Skip to content

fix(logical-optimizer): move whole-expression top-k partitions onto the heap input - #640

Draft
zzylol wants to merge 1 commit into
stack/viewer-vertical-lanesfrom
stack/fix-nested-topk-keys
Draft

zzylol wants to merge 1 commit into
stack/viewer-vertical-lanesfrom
stack/fix-nested-topk-keys

Conversation

@zzylol

@zzylol zzylol commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Stack: #574 → #620 → #618 → #621 → #627 → #625 → #628 → #632 → #634 → #616 → #617 → #622 → #624 → #629 → #630 → #631 → #633 → #635 → #636 → #637 → #638 → #640 → #641

Rebased onto #638. At the chain tip (#641) the full gate passes.

Fixes #639

Problem

For topk by(job)(k, sum by(service, job)(rate(x[5m]))), Stage 1 builds a whole-expression heap that reads the raw rate rows and absorbs the inner sum. That heap kept the outer top-k's Reduction, whose keys are indices into the inner aggregate's output. Read against the rate rows, by(job) became by(ts), so the result lost job. The legacy search, removed in #635, remapped these keys.

Changes

  • pass1/logical_candidates.rs: whole_expression_input now also returns the outer partitions mapped onto the heap's input. A new helper, partitions_over, matches each key by column (column_ref). It returns None, so the heap is not offered, when a key is missing or ambiguous in the input or the keys are given by without. realize builds the SummaryAgg with these partitions.
  • Un-ignored planner_weighted_topk_binds_at_either_deployment_phase. It now asserts (job, value) pairs at both phases: api: 0.3125, 0.375 and batch: 80, 100.
  • Added the Pass 1 unit test whole_expression_topk_partitions_index_the_heap_input. Every heap in an absorbing candidate partitions by job in its child's schema. Without the fix, it fails with ["ts"].

Test plan

  • cargo fmt --all -- --check: clean
  • cargo clippy --workspace --all-targets -- -D warnings: clean
  • cargo test --workspace: 1282 passed, 0 failed, 13 ignored
  • Example 1 is unaffected: example1_document_is_valid_and_committed_fixture_is_current passes, and tools/dag-viewer/examples/planner-layering-example1.json is unchanged.

🤖 Generated with Claude Code

…he heap input

The whole-expression heap reads the absorbed aggregate's input, but kept
the outer top-k's partition keys, which index that aggregate's output:
`topk by(job)(k, sum by(service, job)(rate(x[5m])))` partitioned by `ts`
and lost `job`. Resolve the keys by column onto the heap's input, and do
not offer the heap when they cannot be resolved.

Fixes #639

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zzylol
zzylol force-pushed the stack/fix-nested-topk-keys branch from 1f9a8f1 to d889c14 Compare October 5, 2026 13:11
@zzylol
zzylol changed the base branch from stack/cleanup-11-docs to stack/viewer-vertical-lanes October 5, 2026 13:11
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.

1 participant