fix(unparser): unparse a stacked aggregate as a derived table - #192
Open
claudespice wants to merge 1 commit into
Open
fix(unparser): unparse a stacked aggregate as a derived table#192claudespice wants to merge 1 commit into
claudespice wants to merge 1 commit into
Conversation
A SELECT expresses a single grouping, but the `LogicalPlan::Aggregate` arm of `select_to_sql_recursively` recursed straight into its input whenever the select list was already built, so a second aggregate underneath was skipped and its `GROUP BY` never reached the emitted SQL. `single_distinct_to_groupby` produces exactly that shape for `count(DISTINCT c)`: an outer `count(alias1)` over an inner `Aggregate` grouping by `c AS alias1`. With the inner aggregate dropped, `SELECT count(DISTINCT "UserID") FROM hits` unparses to `SELECT count(alias1) FROM hits` — `alias1` does not exist on the base table, so a consumer pushing the optimized plan down to a remote engine gets a binder error, and where a column of that name does exist it counts every row rather than the distinct values. Track on the `SelectBuilder` whether an aggregate has been folded into the current SELECT, and emit a `derived_aggregate` table for the next one, as the `Sort`, `Limit`, `Distinct` and `Projection` arms already do. The existing TPC-H and ClickBench roundtrip suites unparse the plan as the SQL planner produces it, before `single_distinct_to_groupby` runs, which is why they never caught this; the new core test unparses the optimized plan and executes both statements.
2 tasks
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.
Summary (root cause)
A
SELECTexpresses a single grouping, but theLogicalPlan::Aggregatearm ofselect_to_sql_recursivelyrecursed straight into its input whenever the select list hadalready been built (
select.already_projected()), so a second aggregate stackedunderneath was skipped entirely — its
GROUP BYnever reached the emitted SQL. TheSort,Limit,DistinctandProjectionarms all guard the same situation by emittinga derived table;
Aggregatedid not.single_distinct_to_groupbyproduces exactly that shape forcount(DISTINCT c): an outercount(alias1)over an innerAggregategrouping byc AS alias1. Unparsing theoptimized plan for
SELECT count(DISTINCT "UserID") FROM hitstherefore emittedTwo consequences, both bad:
alias1does not exist on the base table, so a consumer that pushes the optimized plandown to a remote engine gets a binder error (
Referenced column "alias1" not found in FROM clause). This is the failure reported downstream in DuckDB agg pushdown: Clickbench/aliasing issues spiceai#8475 againstClickBench q5 on DuckDB.
DISTINCTissilently gone —
count(alias1)counts every row.Changes
SelectBuildertracks whether an aggregate has been folded into the current SELECT(
mark_aggregated()/already_aggregated()).Aggregatearm emits aderived_aggregatetable for a second aggregate instead ofskipping it, matching the existing
derived_sort/derived_limit/derived_distinct/derived_projectionhandling.After the fix:
Why the existing roundtrip suites missed it
datafusion/core/tests/sql/unparser.rsruns the TPC-H and ClickBench queries throughdf.logical_plan()— the plan as the SQL planner produces it, beforesingle_distinct_to_groupbyruns. The rewrite only appears in the optimized plan, whichis what a federation/pushdown consumer unparses. The new test closes that gap.
Test plan
cargo test -p datafusion-sql—stacked_aggregate_is_unparsed_as_a_derived_tablecovers the ungrouped
count(DISTINCT b)shape, the groupeda, count(DISTINCT b) ... GROUP BY ashape, and a lone aggregate (which must still foldinto its SELECT rather than become a derived table).
Result: 498 passed / 22 failed, versus 497 / 22 on a pristine checkout of this
base — the same 22 pre-existing failures, byte-identical output, plus the new test.
cargo test -p datafusion --test core_integration sql::—test_optimized_plan_roundtrip_count_distinctunparses the optimized plan forcount(DISTINCT)with and without aGROUP BY, then executes both the original and theunparsed SQL against the ClickBench fixture and compares the rows.
Result: 71 passed / 29 failed, versus 70 / 30 on the same base — the only delta
is the new test flipping from failing to passing.
plan.rsreverted, both new tests fail, and the core test failswith
column 'alias1' not foundand the emitted SQLSELECT count(alias1) AS c FROM hits_raw AS hits— i.e. it reproduces the reported bug.Downstream issue: spiceai/spiceai#8475