Conversation
Neither IR could give one aggregate function its own row predicate, so `count(CASE WHEN p THEN 1 END)` next to a plain `sum(x)` was rejected at lowering. `QueryExpr::Aggregate` and `ExactOperation::Aggregate` gain `filters: Vec<Option<Predicate>>`, parallel to `measures` and positional against the child; `SummaryAgg` and its wire payload gain `filter`, and the executable DAG wire version goes to 6. The SQL front end fills the field from an explicit `FILTER (WHERE …)` (parsed through a GenericDialect wrapper that enables the clause), from `count(CASE WHEN p THEN x END)`, and from `count(expr)` over a nullable `expr`. Resolution, canonicalization, CSE, dependency collection and DAG export carry it. No binding rule applies a filter yet: every recognizer in asap-aware-mapping keeps a filtered Aggregate as `KeepPreAsap`, and the heavy-hitter promotion skips filtered rankings. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
milindsrivastava1997
left a comment
Collaborator
There was a problem hiding this comment.
@Selvomega Thanks this is useful. Few questions:
- What is the behavior when multiple aggregations share the same filter? Is there any logic to deduplicate filters?
- What is the IR when there is only a single aggregate with a filter statement and what is the IR when there is only a single aggregate with an equivalent case statement? Do these equivalent SQL queries produce the same or different IRs?
If the above case produces different IRs, we may add a canonicalization pass that normalizes them to the same IR. I believe you will find such existing canonicalization logic in the codebase.
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.
Why
Closes #466.
Neither the pre- nor the post-ASAP IR could give one aggregate function its own row predicate. A predicate could only sit on the relation (
Scan.predicates,Filter), where it applies to every measure. So a query that is one scan and one grouping, with one conditional count next to a plain sum, could not be planned at all:AggIntent::Countcounts rows and never consults its argument, so lowering had no place to put the condition and rejected the query rather than silently drop it. The only alternatives were a hand-written split into twoAggregates joined back, or encoding the condition into a derived column (which onlycountIfdoes, and which erases the predicate from the IR).What
A measure can carry a predicate with SQL
FILTER (WHERE …)semantics: only rows where it isTRUEupdate that measure; groups are still formed from every row.QueryExpr::AggregateandExactOperation::Aggregategainfilters: Vec<Option<Predicate>>, parallel tomeasures, positional against the child's output (likeFilter.pred, unlikehaving). Empty means unfiltered, andresolve_rootnormalizes an all-Nonevector to empty so equality and CSE see one shape.SummaryExpr::SummaryAggand its executable payload gainfilter: Option<Predicate>.POST_ASAP_DAG_WIRE_VERSIONgoes 5 → 6 so an older reader fails loudly instead of taking a filtered aggregate as unfiltered.filtersfrom three shapes: an explicitFILTER (WHERE p);count(CASE WHEN p THEN x END)→p [AND x IS NOT NULL]; andcount(expr)over any other nullableexpr→expr IS NOT NULL. TheCOUNT/corr … FILTERrejections are gone.asap-aware-mappingkeeps a filteredAggregateasKeepPreAsap, and canonicalization does not promote a filtered count ranking to a heavy-hitterTopK.Docs:
docs/develop_docs/pre-asap-ir.md(thefiltersfield, with the example above) anddocs/design_docs/architecture/physical-plan-integration.md(wire version 6).Before this PR
The query above parses, and lowering rejects it:
FILTER (WHERE …)did not even reach the planner: sqlparser'sGenericDialecthassupports_filter_during_aggregationoff, and DataFusion only selects a dialect by name.After this PR
The same query lowers to one
Aggregateover oneScan, noJoin, no derived column. TheCASEcondition became the Count's filter; the Sum is unfiltered:keep_pre_asapon that tree yields an exact plan and compiles to an executable DAG.SELECT sum(bytes) FILTER (WHERE service = 'a'), count(*) …lowers the same way, withfilters = [Some(service == 'a'), None].How
asap-typesfiltersonQueryExpr::Aggregate/ExactOperation::Aggregate;filteronSummaryAggandExecutableOperatorPayload::SummaryAgg; wire version 6.resolve.rsbinds each filter against the child schema and normalizes all-Noneto empty.cse.rs(structural hash and rebuild),schema_resolver.rs(usage-derived catalog),dag_export.rsand the post-ASAP CSE identity carry the field.canonicalize.rsskips filtered rankings.execution_data_state.rscounts filter columns as referenced when checking exact-operator inputs.any_measure_filtered()is the shared gate.asap-frontend-sqlsql/dialect.rs:GenericDialectwith onlysupports_filter_during_aggregationflipped;lowerparses throughDFParser::parse_sql_with_dialect+statement_to_plan, as the ClickHouse path already did.lower_aggregatecomputes onemeasure_filterper aggregate call, passes the filter's columns through any derived-columnProject, and emitsfilters. Temporal aggregates andROLLUP/GROUPING SETScannot carry a filter and now reject one explicitly instead of dropping it.asap-aware-mappingbindable_intent, exact composition, rollup, accuracy reconciliation, maintained population, the avg and composed rewrites, exact top-k,query_time_nested_sumand the analytical cost lowering all decline a filteredAggregate.SummaryAggrelinking carriesfilter.sql-function-catalogcountIfstill lowers tosum(CASE …); its doc now says moving the-Iffamily ontofiltersis a follow-up.Design choices, as in the issue: a parallel vector rather than a
Measurestruct, so existing readers ofmeasureskeep compiling; a field onSummaryAggrather than aFilterchild, so summaries that differ only in predicate can still share one child.Tests
filtersround-trips, and anAggregateserialized before the field existed still reads as unfiltered (aggregate_filters_serde_round_trip_and_default).filtered_and_unfiltered_aggregates_do_not_merge).Aggregatewith noJoin(conditional_count_lowers_to_a_filtered_measure);FILTER (WHERE …)lands on the measure it annotates;count(nullable)becomesIS NOT NULL; the filter's columns survive a derived-columnProject; a filter insideROLLUPis rejected;corr … FILTERnow lowers.Nonenormalizes to empty.Sort + Limit.KeepPreAsap(filtered_measure_stays_logical).GenericDialectswitch and parses theFILTERclause the generic dialect rejects.corr_rejects_unrepresented_formsno longer listsFILTER;count_preserves_non_null_inputs_and_rejects_erased_null_semanticsis nowcount_null_semantics_become_a_measure_filter.cargo test --workspace,cargo clippy --workspace --testsandcargo fmtare clean.Not in this PR
SummaryAgg.filteris never set by a rule yet).sum(CASE WHEN p THEN x END)still lowers to a derived column and relies on undefined NULL-skipping.countIf/sumIf/avgIf/… onfilters.🤖 Generated with Claude Code