There seem to be 2 ways to express a GROUP BY + Aggregate

[milindsrivastava1997]

Aggregate node has a by field to express columns to group by
We also have a Partition node which splits a table into multiple based on a key, effectively a group by

We should canonicalize the L3 IR so that there is only one way to express GROUP BY + Aggregate.

[zzylol]

Reframing: this is two different operations, not two ways to do one — and the duplication is already mostly resolved

Digging into current main, the "two ways to express GROUP BY + Aggregate" framing doesn't quite match the code anymore. Two findings:

1. The group-by-reduce duplication is already gone. After the #8 convergence + f9458d6 (route tabular GROUP BY through positional Aggregate.by), every reducing GROUP BY — SQL and PromQL — lowers to Aggregate.by. No query produces a Partition wrapping a reducing aggregate. The remaining fallback at lower.rs:162-176 only ever wraps a per-series (label-preserving) reduction with by: [], so Partition is never standing in for group-by-reduce today.

2. The surviving Partition is a genuinely different concept — split without reduce. It's row-preserving (its output_schema_in arm is pass-through, query_expr.rs:457), whereas Aggregate collapses N rows → 1 and rewrites the schema to by ++ aggs (closed: true). They only share the "set of keys" shape; what they do with the keys is opposite:

	Aggregate { by }	Partition { keys }
effect	group and reduce → 1 row/group	split/route, rows preserved
schema	new (by ++ aggs, closed)	child schema unchanged (pass-through)
meaning	GROUP BY	PARTITION BY / sharding

So Partition is really PARTITION BY, not GROUP BY. Its only live use today is recording per-group structure over a per-series reduction — e.g. topk by (host) (avg_over_time(cpu[5m])), where you keep each series' windowed value and then rank within each host group.

Where it should live

Per the layering contract (design.md §3: L3/L4 are symbolic, "describe operations without committing to where anything runs"; L5 produces the stage-assigned plan), a split-for-parallelism is a physical decision and does not belong in the symbolic L3 IR. Concretely the end-state is:

• L3: Aggregate.by is the only grouping construct. The standalone Partition node leaves QueryExpr. (WindowFunc.partition_by stays — that partition is analytic semantics, not placement.)
• Per-group ranking (topk by (host)): express the partition on the ranking operator as a partition_by, mirroring WindowFunc.partition_by — not via a generic node.
• Parallel/sharding split: an L5 stage-allocation artifact. design.md §3 already says the StageAllocator colours the L4 DAG by StageId and inserts shard/merge edges; it pairs with the existing L4 SketchExpr::SummaryMerge. L4 contributes the precondition (mergeability) + cost signal; L5 materializes the split. It should be derived by the allocator, not authored as a node.

Blocker for a naive removal

You can't just move the windowed-reduction's group keys into Aggregate.by: per-series schema detection keys off Aggregate.by.is_empty() (query_expr.rs:393). Putting keys there flips the node to a cross-series reduce and breaks the label-preserving (open, time-indexed) schema that topk by/*_over_time rely on. So removal needs the per-group-structure home (rank partition_by + explicit grouped-windowed handling) built first.

Doc inconsistency feeding the confusion

design.md describes Partition two contradictory ways: line 305 — "Partition the stream by key tuple (GROUP BY / PromQL by (dims))" — vs the schema-flow table at line 444 — "logical-only marker; carries a sharding hint for L5's stage allocator." The stale line 305 is a source of the "is Partition a group-by?" confusion.

Proposed PR (scoped)

Opening a PR that takes the safe first step and encodes this decision without the risky node removal:

• reconcile design.md (kill the stale GROUP BY framing of Partition),
• re-document the L3 Partition node + the lower.rs fallback as transitional split-without-reduce, slated to move to a rank partition_by (L3) / L5-physical, referencing this issue and the per-series-detection blocker,
• add a regression test pinning the resolved half: every group-by-reduce lowers to Aggregate.by and produces no Partition.

Full node removal tracked as the follow-up (needs the rank-partition_by work).

[zzylol]

Implemented in PR #18 — the actual removal landed in commit 32b315c ("feat(core)!: remove L3 Partition node; per-group ranking → Sort.partition_by").

The L3 Partition node (and PartitionKeys) are gone. Grouping now has exactly one home per concept:

• reducing GROUP BY → Aggregate.by
• per-group ranking (topk by (host)) → Sort.partition_by (new field, row-preserving)
• parallel / sharding split → L5 stage allocator (physical, not in the symbolic IR)

The generic topk by/bottomk grouping — the sole producer of the old node — now routes to Sort.partition_by, so the windowed reduction beneath stays label-preserving (by: []). docs/design.md is reconciled and the change is verified green (cargo test across core/lower/e2e, 17 suites).

Will close once #18 merges.

[zzylol]

On the follow-up "should we drop Aggregate.by and use a group/partition node instead?" — no, and here's why.

A standalone group/partition node is exactly the Partition node this issue just removed: with nothing consuming it, it's a row-preserving pass-through whose meaning (semantic grouping vs physical sharding) is ambiguous. The deeper reason it can't live upstream as a node: grouping's semantics are defined by the consumer, not the grouping —

• group by host + reduce (sum) → 1 row/group (collapses)
• group by host + rank (topk) → many rows/group (ranks within group)
• group by host + window (rate) → per-series scan per group

Same keys, opposite row behavior. So the keys belong on the operator that defines what the grouping does — which is also why a node boundary is wrong (a Filter/Project could wedge between a GroupBy node and its aggregate, forcing the aggregate to "look down the tree" for its grouping; and "no grouping" becomes ambiguous: absent node vs empty node). It also diverges from the relational/DataFusion substrate L3 is built on, where Aggregate{group_expr, aggr_expr} is atomic.

The real observation underneath the question is valid, though: grouping keys currently live in three places — Aggregate.by, Sort.partition_by, WindowFunc.partition_by. The right unification is a shared parameter type, not a node:

struct GroupKeys(Vec<ColumnId>);   // one spelling of "per-group keys"
Aggregate  { by: GroupKeys, .. }            // reduce per group
Sort       { partition_by: GroupKeys, .. }  // rank per group (incl. generic topk/bottomk)
WindowFunc { partition_by: GroupKeys, .. }  // window per group

This gives the DRY win (one home to evolve grouping — e.g. qualified keys, a future without(...)) without re-introducing the standalone-node ambiguity, and keeps "one canonical form per query" intact.

This also ties off #13: heavy-hitter topk carries its grouping on Aggregate.by (the AggIntent::TopK rides there), generic topk/bottomk on Sort.partition_by — both become GroupKeys, so every grouping concept (reduce / rank / window) shares one type.

(The one model where grouping-as-a-node fits is streaming keyBy (Flink/Kafka Streams), where a KeyedStream is consumed by keyed operators. ASAP is streaming-ish, so it's not a crazy thought — but L3 deliberately chose the relational/DataFusion model, and even Flink couples keyBy tightly to the next operator. Switching models is a far larger change than this issue, and would reopen the ambiguity #12 closed.)

Starting the GroupKeys refactor in PR #18.