Skip to content

fix(l2): drop provably-absent outer group keys (#53); pin + document the #27 nesting contract - #101

Merged
zzylol merged 3 commits into
mainfrom
fix/53-nested-agg-absent-group-key
Jul 6, 2026
Merged

zzylol merged 3 commits into
mainfrom
fix/53-nested-agg-absent-group-key

Conversation

@zzylol

@zzylol zzylol commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Closes #53. Also finishes the remaining in-scope checklist items of #27 (details below).

#53 — decision: fix, not WAI-rejection

sum(sum by (group)(http_requests{job="api-server"})) by (job) is valid PromQL: aggregating by a label that no input series carries puts every series in one group and omits the (empty) label from the output. Prometheus evaluates it; we rejected it with column job not found in schema (have: ["group", "sum"]).

The fix is the new resolve_group_keys_promql in asap-l2::column_resolution: a group key that fails to resolve against a closed schema is dropped rather than rejected. Closure is what makes this exact rather than permissive — the inner cross-series aggregate enumerated its output columns, so the label's absence is provable at plan time, grouping by it is the identity partition, and dropping it reproduces Prometheus's output labels precisely. Two guardrails:

  • Open schemas keep the strict error. The Binder seeds every referenced label onto PromQL leaves, so an unresolved key over an open schema is a resolution bug, not an absent label — it must keep surfacing.
  • SQL is untouched. The helper is wired only into the PromQL single-stat aggregate branch of the shared converter; SQL GROUP BY still resolves through the strict resolve_column_refs (and DataFusion has validated the columns before we ever see them).

Pinned at three levels: resolver unit tests (closed drops / open errors / present keys keep positions), conformance tests (the repro lowers with the key dropped; a key that IS present in the nested aggregate's output still resolves positionally), and an exact-tree e2e pin of the repro query (q53_outer_group_key_absent_from_nested_aggregate).

#27 — the two remaining in-scope checklist items

  1. SQL scalar / IN / EXISTS subqueries in predicates — the open question is now decided and pinned: reject cleanly in v1, for the whole family. Scalar (x > (SELECT …)) and IN (SELECT …) were already rejected with pinned tests; this adds exists_subquery_in_predicate_is_rejected covering correlated EXISTS, NOT EXISTS, and NOT IN, so no shape in the family can start silently mislowering. (Representing them needs a subquery node in the L2 expression IR + a correlated-vs-uncorrelated choice; derived tables in FROM remain the supported SQL nesting shape.)
  2. Document the nesting contractdocs/promql-lowering.md gains a "The nesting contract (issue Support nested query functions for PromQL and SQL (testing + implementation) #27)" section: a lowers-vs-rejected table of composite argument shapes (aggregate-over-aggregate, aggregate-over-binary-op, generic vs heavy-hitter top-k, range functions over sub-queries, the L3: nested aggregate drops outer group key (sum(sum by (k)(...)) by (j)) — decide WAI vs fix #53 absent-key semantics, SQL derived tables; vs unary negation Unary negation rejected in PromQL lowering (no scalar/negate node) #36, without(...) PromQL without(...) grouping unsupported — usage-derived schema can't enumerate the complement #39, predicate subqueries), every row citing the test that pins it.

After this, every #27 checklist item is either done or tracked by its own dedicated issue (#36 unary negation — the last one standing, since #35 landed), so #27 can be closed as a tracking issue if you agree.

Note: this PR does not touch asap-plan/asap-sketch, so it is independent of open PR #100.

Verified: cargo test --workspace — 0 failures (334 tests; 8 new); cargo clippy --all-targets clean; cargo fmt --check diff count identical to main's pre-existing baseline (113 — no new formatting drift).

🤖 Generated with Claude Code

zz_y and others added 2 commits July 5, 2026 17:16
)

Decision on #53: fix, not WAI-rejection. Prometheus accepts aggregating
`by` a label absent from every input series — all series land in one
group and the empty label is omitted from the output — so
`sum(sum by (group)(m{job="api-server"})) by (job)` is a valid query
that must lower, not a resolution error.

The new `resolve_group_keys_promql` drops a group key that fails to
resolve against a **closed** schema: closure makes the label's absence
provable (the inner cross-series aggregate enumerated its output), so
grouping by it is the identity partition and dropping it is
semantically exact. Against an open schema an unresolved key stays an
error — the Binder seeds every referenced label on PromQL leaves, so an
open-schema miss is a resolution bug, not an absent label. Wired only
into the PromQL single-stat aggregate branch of the converter; SQL
GROUP BY keeps strict resolution.

Pinned at three levels: unit tests on the resolver (closed drops /
open errors), conformance tests (absent key drops; a present key after
a nested aggregate still resolves positionally), and an exact-tree e2e
pin of the issue's repro query.

Closes #53

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nesting contract (#27)

Finishes the two remaining in-scope #27 checklist items:

- SQL scalar / IN / EXISTS subqueries in predicates: the v1 decision is
  reject cleanly (they need a subquery node in the L2 expression IR and
  a correlated-vs-uncorrelated representation choice; derived tables in
  FROM are the supported nesting shape). The rejection already existed
  for scalar and IN; this pins the rest of the family — EXISTS,
  NOT EXISTS, NOT IN, correlated or not — so nothing can start
  silently mislowering.

- docs/promql-lowering.md gains "The nesting contract (issue #27)":
  which composite arguments lower (aggregate-over-aggregate, aggregate-
  over-binary-op, ranking over nested aggregates, range functions over
  sub-queries, absent outer group keys per #53, SQL derived tables) vs.
  which are cleanly rejected (unary negation #36, without() #39,
  predicate subqueries) — every row cites the test that pins it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`max_over_time(deriv(rate(distance_covered_total[5s])[30s:5s])[10m:])` —
the nested sub-query example from the PromQL docs (two stacked
sub-queries feeding range functions; the outer `[10m:]` uses the
default resolution). It already lowers correctly on the recursive
`walk`/converter path; this locks the exact shape so a regression in
sub-query nesting or default-resolution handling can't slip through.

- Conformance test walks the full spine node by node
  (Max ∘ Subquery{10m, res:None} ∘ Deriv ∘ Subquery{30s, res:5s}
   ∘ Rate ∘ TimeRange{5s} ∘ Scan) and checks the output schema stays
  the label-preserving open `[ts, value]`.
- e2e test pins the whole tree with a single exact `assert_eq!`.
- Added as a row to the nesting-contract table in
  docs/promql-lowering.md.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@zzylol
zzylol merged commit 5e9e280 into main Jul 6, 2026
1 check passed
@zzylol
zzylol deleted the fix/53-nested-agg-absent-group-key branch July 6, 2026 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

L3: nested aggregate drops outer group key (sum(sum by (k)(...)) by (j)) — decide WAI vs fix

1 participant