feat(query-expr): add opt-in Concat discriminator key - #291
Merged
Merged
Conversation
zzylol
changed the title
zzylol
force-pushed
the
feat/concat-discriminator-unique-keys-228
branch
from
September 4, 2026 04:00
acfd2fe to
8672389
Compare
zzylol
changed the title
…_keys override Investigated whether any current Concat call site (PromQL histogram_quantiles, SQL ROLLUP/CUBE/GROUPING SETS lowering) pays for a redundant Dedup that a discriminator-based unique_keys override could prove unnecessary. Finding: neither call site emits a Dedup (or equivalent) after its Concat today, and no consumer of Schema::unique_keys (CSE's share_common_subtrees, asap_aware_mapping::rollup's is_legal_rollup_source) is exercised by either Concat in any current test or workload. SQL grouping-set lowering also actively discards the one natural discriminator (__grouping_id) today, since this front end rejects GROUPING(). Recommendation: defer Option 1 (producer-supplied unique_keys override) until a real consumer exists; building it now would be speculative surface area with no call site to justify or exercise it. No behavior change — Concat still drops unique_keys unconditionally, per merge_drops_the_branches_unique_keys / merge_and_setop_agree_on_unique_keys (both still passing, unchanged). Ref #228 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per explicit user direction overriding the investigation's own defer recommendation: build Option 1 (producer-supplied unique_keys override) ahead of a proven call-site win, as a deliberate 'ship the extension point now' decision. - QueryExpr::Concat gains an opt-in discriminator_unique_key: Option< ConcatDiscriminatorKey<C>> field. ConcatDiscriminatorKey's fields (discriminator, inner_key) are private; the only constructor, ConcatDiscriminatorKey::new, requires the discriminator column to be named explicitly by the caller -- nothing infers or defaults one. - QueryExpr::concat(children) is the new ordinary constructor (discriminator_unique_key: None), replacing the bare struct literal at every call site in the tree. - QueryExpr::concat_with_discriminator(children, discriminator, inner_key) is the override constructor. - output_schema()'s Concat arm: unchanged default (drop unique_keys unconditionally) when the field is None; when Some, asserts (discriminator, inner_key) as the sole unique key, trusting the caller's claim without verifying it. - resolve.rs resolves a pre-bind discriminator key into its post-bind ColumnId equivalent against the first resolved branch's schema, so the feature is correct end-to-end for a future caller upstream of resolve_root. - Every other Concat match/construction site across the tree (canonicalize, cse, binder, dag_export, asap-aware-mapping's replacement/explanation, and test/tooling AST walkers) updated mechanically; cse.rs's rebuild_children (CSE interning) threads the field through unchanged rather than dropping it. Not wired into any real lowering call site, per instruction: neither histogram_quantiles nor SQL ROLLUP/CUBE/GROUPING SETS lowering calls concat_with_discriminator. Both still call the plain concat() builder -- byte-for-byte the same output_schema() behavior as before. SQL's natural discriminator (__grouping_id) is still discarded because this front end rejects GROUPING(); PromQL's phi discriminator is structurally available but nothing downstream needs the resulting key yet. Both noted as future work. Tests (crates/types/src/pre_asap/query_expr.rs): - merge_drops_the_branches_unique_keys / merge_and_setop_agree_on_unique_keys: unchanged, still pass. - discriminator_override_produces_a_compound_unique_key: new, concat_with_discriminator on branches individually deduplicated on the same column yields unique_keys == [[discriminator, inner_key]]. - ordinary_concat_struct_literal_still_drops_unique_keys_by_default: new, the bare struct literal with discriminator_unique_key: None still drops unique_keys. - no_way_to_fabricate_a_unique_key_without_naming_a_discriminator: new misuse check -- neither the ordinary builder nor an explicit None literal can produce a unique key without a call site literally naming a discriminator column via ConcatDiscriminatorKey::new. docs/design_docs/concat-unique-keys-decision.md updated to record the investigation (unchanged), the reversal, and the safety argument for shipping the override unused. cargo build --workspace, cargo test --workspace, and cargo clippy --workspace --tests all clean. Ref #228 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…c caveat (#228) Review fixes on the Concat discriminator_unique_key override: 1. binder.rs's collect_referenced_columns didn't walk discriminator_unique_key's ColumnRefs -- the Concat arm was updated with ', ..' only, unlike the analogous Dedup.cols case (which is walked). This function seeds every name a query references into the Binder's usage-derived fallback schema; a discriminator column not otherwise referenced anywhere else in the tree, over a schema-less leaf Scan, would be absent from that fallback schema, and resolve.rs's later resolve_column_ref call would fail NotFound for a column the caller correctly named. Fixed: the Concat arm now pushes key.discriminator() and every key.inner_key() column into the walk, mirroring Dedup.cols exactly. New regression tests: binder.rs's concat_discriminator_key_is_seeded_into_the_binder_schema and resolve.rs's resolve_root_seeds_and_resolves_an_otherwise_unreferenced_discriminator_column (end-to-end through resolve_root). 2. Resolved ColumnIds in discriminator_unique_key could go stale after canonicalize() runs. resolve() resolves the key's ColumnRefs against children.first()'s pre-canonicalize output schema; canonicalize() then runs afterward and can restructure that same branch (try_promote_heavy_hitter, try_rewrite_rownumber_topk both replace a Limit{Sort{Aggregate}}/Filter{...} shape with a differently-shaped Aggregate, anywhere within the branch), changing its column count/order with no consistency check downstream. Fixed in canon() (canonicalize.rs): snapshot the first branch's output schema before recursing into a Concat's children (exactly what resolve.rs resolved against), and after recursing, drop the key (never re-derive it by guessing) if the schema differs at all -- erring conservatively, since a wrong unique_keys claim is a wrong query answer, not a missed optimization. Two new tests in canonicalize.rs pin both outcomes: the key survives an untouched branch, and is dropped when the branch matches the heavy-hitter promotion trigger. 3. Documentation accuracy: the doc comment on ConcatDiscriminatorKey and the design doc's safety argument overclaimed "there is no path to a non-empty unique_keys claim that doesn't go through a call site literally naming the discriminator" -- true for other Rust code, but #[derive(Deserialize)] is same-module generated code that builds the struct directly from arbitrary field values, bypassing new() entirely. Currently unreachable (the only whole-QueryExpr deserialization call site in the repo is a same-file unit test), but the claim was factually incomplete as stated. Fixed by scoping both claims to 'other Rust code' and stating the Deserialize caveat explicitly, rather than adding speculative runtime hardening for a currently-unreachable path. docs/design_docs/concat-unique-keys-decision.md updated with a 'Review fixes' section covering all three, and both doc-accuracy fixes applied inline. cargo build --workspace, cargo test --workspace (115 passed, 0 failed), cargo clippy --workspace --all-targets -- -D warnings, and cargo fmt --all -- --check all clean. Ref #228 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
force-pushed
the
feat/concat-discriminator-unique-keys-228
branch
from
September 4, 2026 11:35
8672389 to
5a31247
Compare
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
1 participant
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.
Why
Concatnormally must drop branch-localunique_keys: the same key value can appear in multiple branches. Some producers can prove branch disjointness with an explicit discriminator, however, and need a safe way to preserve the compound(discriminator, inner_key)fact.What
ConcatDiscriminatorKeyandQueryExpr::concat_with_discriminatoras an opt-in assertion.QueryExpr::concatbehavior unchanged: no discriminator assertion means no output unique key.concatfor now.Correctness contract
The assertion is sound only when both facts hold:
inner_keyis unique within every branch.The type cannot prove either data-level fact. Rust callers must opt in explicitly. Deserialization is also only a caller assertion:
ConcatDiscriminatorKeyrejects unknown fields, and any externalQueryExprboundary must reject or validate the assertion before using it as uniqueness evidence.The binder walks every referenced discriminator/key column so schema-less inputs can resolve them. Canonicalization never guesses a replacement key after a schema rewrite; it fails closed by removing the assertion.
Scope
This PR provides the general IR mechanism. It does not enable it in
histogram_quantilesor SQL grouping-set lowering because those paths do not yet establish and consume the full proof contract end to end.Verification
After rebasing onto the fully merged analytical-cost/window/viewer stack:
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkasap-typestests, including discriminator construction, binding, resolution, canonicalization, legacy behavior, and unknown-field rejection.Closes #228.