Conversation
…UNT(DISTINCT) Two changes, in dependency order. The second is only expressible because of the first. `AggIntent` asserted that an aggregate reads one column: `input_col() -> Option<ColumnId>` was how every consumer reached an input. #421 added `PearsonCorr { left, right }` and had to make it return `None` from that accessor *defensively*, so no single-column consumer could pick up half of the pair. The accessor had become a trap each new multi-column intent must remember to opt out of, and the cost of forgetting is a silently wrong number rather than a compile error. That is also why `COUNT(DISTINCT a, b)` had nowhere to land. It counts distinct *tuples*; SQL lowering dropped every argument after the first, reporting single-column cardinality where the query asked for tuple cardinality. #419 stopped the miscount by rejecting the call outright, which left TPC-H/Deequ U-P2b and nine synthetic-packet-trace flow counts permanently unlowerable. Before this PR: SELECT count(DISTINCT l_orderkey, l_linenumber) * 1.0 / count(*) FROM lineitem -> lowering failed: unsupported aggregate: multi-column COUNT(DISTINCT) After this PR: Aggregate { measures: [Cardinality { cols: [0, 1], .. }], .. } What changed: - `input_col()` is removed. `input_cols() -> Vec<C>` is the only column accessor, so no consumer can ask for "the" input column of an aggregate that reads two. An intent may now declare whatever arity its semantics need without a defensive opt-out, and `PearsonCorr`'s opt-out is gone. The genuinely unary reducers keep `col: Option<C>` — this relaxes what consumers assume, it does not widen every variant. - `AggIntent::Cardinality` takes `cols: Vec<C>`. One entry is `COUNT(DISTINCT col)`, several count distinct tuples, empty keeps the PromQL "the sample value" convention. It is the first variable-arity intent, and was not expressible while the accessor split existed. - Realization is the single-column one with a wider item: a tuple becomes a `SummaryInputExpr::Tuple`, which HLL/Theta/KMV hash as one value. UnivMon is withheld from a tuple — it estimates frequency moments over a single value stream — and the value-frequency rule refuses a multi-column intent directly, so that invariant does not rest on the candidate table alone. - SQL lowering resolves every `COUNT(DISTINCT ...)` argument as a grouping key, so a qualifier survives a join; an expression argument is rejected rather than reduced over a probe column. Serialization of `Cardinality` changes from `col` to `cols`; a payload that omits the field still reads as the implicit input. Both corpus ratchets move in this commit: tpch_deequ 49 -> 50 lowered with no rejections, synthetic_packet_trace 61 -> 70. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment on lines
+125
to
+144
| `Cardinality { cols, accuracy }` carries a list, not one column. One entry is | ||
| SQL `COUNT(DISTINCT col)`; several count distinct *tuples* | ||
| (`COUNT(DISTINCT a, b)`), which is not the distinct count of any one of them. | ||
| Empty is the PromQL convention "the sample value" (`count_values`, | ||
| `distinct_over_time`). | ||
|
|
||
| `input_cols()` is the only column accessor on `AggIntent` — an intent's arity is | ||
| its own business, so no consumer can ask for "the" input column of an aggregate | ||
| that reads two and silently receive one leg of it. That mattered concretely: | ||
| before `Cardinality` took a list, SQL lowering dropped every argument after the | ||
| first, reporting single-column cardinality as tuple cardinality. | ||
|
|
||
| Realization is the single-column one with a wider item: a tuple becomes a | ||
| `SummaryInputExpr::Tuple`, which the distinct-count sketches (HLL, Theta, KMV) | ||
| hash as one value. UnivMon is withheld from a tuple — it estimates frequency | ||
| moments over a single value stream. At `AccuracyTarget::Exact` the node stays a | ||
| logical pass-through at any arity. Each SQL argument must be a bare column, | ||
| qualifier preserved so a tuple over a join resolves to the correct side; an | ||
| expression argument is rejected rather than reduced over a probe column. | ||
|
|
Collaborator
There was a problem hiding this comment.
i dont understand any of this.
Collaborator
Author
There was a problem hiding this comment.
lololololololol Will double check. I indeed did not check doc part
Collaborator
|
@Selvomega Apart from the stuff added to design doc, rest LGTM. |
Collaborator
Author
lolol I will double check that later |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two changes, in dependency order. The second is only expressible because of the
first.
Closes #425
Closes #424
Why
The IR asserted that an aggregate reads one column.
AggIntentexposedinput_col() -> Option<C>, and every consumer that needed an input column wentthrough it. That is fine for
SUM(x), but it is a claim about the wholevocabulary, and the vocabulary had already outgrown it: #421 added
PearsonCorr { left, right }and had to make it returnNonefrominput_col()defensively, so that no single-column consumer could pick uphalf of the pair. The accessor had become a trap that each new multi-column
intent must remember to opt out of — and the cost of forgetting is a silently
wrong number, not a compile error.
A concrete query had nowhere to land.
COUNT(DISTINCT a, b)counts distincttuples. SQL lowering used to drop every argument after the first, reporting
single-column cardinality where the query asked for tuple cardinality. #419
stopped the silent miscount by rejecting the call outright
(
UnsupportedAggregate("multi-column COUNT(DISTINCT)")).These are the same problem at two levels. The rejection in #419 was not a
missing feature so much as a vocabulary that could not express the intent.
What
IR — inputs are arity-agnostic.
AggIntent::input_col()is removed.input_cols() -> Vec<C>is the onlycolumn accessor.
PearsonCorr's opt-out is gone; it is now ordinary rather than a specialcase that has to neutralize an accessor.
Query — multi-column
COUNT(DISTINCT).AggIntent::Cardinalitycarriescols: Vec<C>instead ofcol: Option<C>.One entry is
COUNT(DISTINCT col); several count distinct tuples; empty keepsthe PromQL "the sample value" convention (
count_values,distinct_over_time).COUNT(DISTINCT a, b, ...)lowers, plans, and realizes end to end.Cardinalitychanges fieldcol→cols. Breaking for apayload that spells the field out; a payload that omits it still reads as the
implicit input.
Before this PR
IR contract. Two accessors, with a documented trap:
A new multi-column intent had to remember to fall through
input_col()'s_arm. Nothing enforced it, and getting it wrong yields a wrong number.
Query behavior.
U-P2b is the check that
(l_orderkey, l_linenumber)is a key:Corpus ratchets on
main:tpch_deequ[("P2b", "multi-column COUNT(DISTINCT)")]synthetic_packet_traceAfter this PR
IR contract. One accessor, no opt-out to remember:
Adding a two-column
covaror a three-columnregr_*is now a variant plus alowering arm. No consumer needs to learn about it, and no variant needs to
defend itself against an accessor.
Query behavior.
U-P2b lowers to one measure carrying both columns, in argument order:
tpch_deequsynthetic_packet_traceA data-quality workload can express a composite-key uniqueness check and have
it planned like any other aggregate — sketched under an ε target, exact
pass-through at
AccuracyTarget::Exact.Verification
cargo test --workspace: 1157 passed, 0 failed (62 test binaries).cargo clippy --workspace --all-targets: 0 warnings.cargo fmt --all --check:clean. All four also ran as pre-commit hooks.
Tests added:
input_cols_tracks_only_reducers(types)distinct_tuple_cardinality_contract(types)input_cols, is mergeable, is not exact, and reports the same output shape as the one-column formresolve_distinct_tuple_columns(types)sketch_realizes_over_the_intents_input_columns(mapping)Tuple, not a single legcomposite_distinct_counts_tuples(sql)COUNT(DISTINCT a,b)→cols == [0,1];COUNT(DISTINCT a)→cols == [0]composite_distinct_rejects_expression_arguments(sql)COUNT(DISTINCT a, b+1)is rejected, not reduced over a probe columnmulti_arg_count_distinct_flow_counts_the_whole_tuple(sql)[0,1,2,3,4]The existing
pearson_corr_contractkeeps passing with itsinput_col()assertion dropped — the property it asserted is now structural.
agg_intent_to_summary_kind_coverage_matrixpins the new decisions (ε → HLL,Exact→ pass-through) at both arities. Both corpus ratchets moved in thiscommit, as the sidra workload and this test corpus are coupled.
End-to-end evidence is the
run_dag_export.pyoutput above. Performancemeasurement: not applicable — no hot path changed, and planner time is
unchanged at ~310 ms for this workload. Screenshots: not applicable.
Limitations and follow-ups
Sum,Avg,Quantileand the reststill carry
col: Option<C>, which is right for their semantics. What this PRremoves is the consumer-side assumption, not per-variant arity. A future
multi-column reducer adds its own field shape.
Cardinalityreadout is L0 of the item-frequency vector, so a tuple item is plausibly
correct — but its sizing and cost model were written for a single value
stream, and I did not verify them at tuple arity. Withholding is the
conservative choice; admitting it is a separate change with its own evidence.
AggIntentthat spells out"col": <id>nolonger deserializes. Following feat: support corr with a general bivariate aggregate type #421's precedent (
kind: pearson_corr, droppedop), no migration is provided.COUNT(DISTINCT t.a)now lowers to aqualified
ColumnRef. Called out above; flag it if this PR should not carryit.