bind: a SQL aggregate root never binds — DataFusion's identity Project wrapper defeats implement_tree's Aggregate check #236

Summary

Found while implementing #191's SQL counterpart to l4_binding.rs (PR #234, crates/integration-tests/tests/l4_binding_sql.rs).

asap_aware_mapping::implement_tree only binds a node that is itself a QueryExpr::Aggregate — anything else falls back to SummaryExpr::Logical for the whole subtree (the "logical parent subsumes bindable child" rule in crates/asap-aware-mapping/src/bind.rs).

lower_promql returns a bare QueryExpr::Aggregate at the root for a top-level aggregation (sum by (job) (m)), so implement_tree binds it directly — this is exactly what the existing PromQL l4_binding.rs tests exercise.

lower_sql does not: DataFusion's own planner inserts an identity Project even for the simplest case, SELECT SUM(x) FROM t. So lower_sql's root is always Project { child: Aggregate { .. } }, never a bare Aggregate — which means implement_tree(lower_sql(...)) always yields SummaryExpr::Logical for the entire tree, for every SQL aggregate query, full stop. The aggregate never gets a sketch or a mergeable exact accumulator; it's treated as opaque logical work every time.

PR #234 pins this explicitly as its own test (sql_root_is_wrapped_in_identity_project_so_binding_falls_back_to_logical, naming may differ slightly — check the actual PR) and works around it in its other tests by manually unwrapping to the inner Aggregate node before calling implement_tree — the same way frontend-sql/tests/sql_lowering.rs's existing find_aggregate/find_aggregate_node helpers already have to. That's fine for a test that wants to assert on the binding shape directly, but it means every real SQL aggregate query, run through the actual pipeline as a caller would use it (not hand-unwrapped), currently never binds to a sketch or exact accumulator at all.

Where the actual bug likely is

Two candidate fixes, not evaluated yet:

1. implement_tree should see through an identity Project (one whose cols are a pure passthrough of the child's columns, no renaming/re-projection beyond what the aggregate itself already produces) when deciding whether its child is bindable, rather than only checking whether the immediate root is itself an Aggregate.
2. The SQL front end should not emit the identity Project in the first place when it adds nothing (DataFusion inserts it, but lower_plan's LogicalPlan::Projection arm could recognize and elide a no-op wrapper before returning Unresolved::Project).

Direction (1) seems more robust — an identity Project can legitimately appear from other sources too (e.g. a derived-table alias per lower_plan's SubqueryAlias handling), and implement_tree is the one place that actually needs to reason about "is there real bindable work under this passthrough," not every producer of Project nodes. But this needs someone who knows bind.rs's actual binding-decision logic to weigh in — flagging as an open question, not a decided fix.

Related

• asap-e2e (integration-tests) has no SQL-path coverage — PromQL only #191 — the coverage gap whose test-writing surfaced this
• PR test: add SQL-path L3->L4 binding coverage to integration-tests #234 — crates/integration-tests/tests/l4_binding_sql.rs, where this is pinned as a test
• crates/asap-aware-mapping/src/bind.rs — implement_tree, the "logical parent subsumes bindable child" fallback
• crates/frontend-sql/src/sql/mod.rs — lower_plan's LogicalPlan::Projection arm, SubqueryAlias handling