Skip to content

fix: TopK aggregation drops groups whose MIN/MAX value is NULL#23684

Open
u70b3 wants to merge 1 commit into
apache:mainfrom
u70b3:fix/grouped-topk-null-aggregate-values
Open

fix: TopK aggregation drops groups whose MIN/MAX value is NULL#23684
u70b3 wants to merge 1 commit into
apache:mainfrom
u70b3:fix/grouped-topk-null-aggregate-values

Conversation

@u70b3

@u70b3 u70b3 commented Jul 18, 2026

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

When TopKAggregation pushes a LIMIT into a MIN/MAX aggregate, a group whose aggregate inputs are all NULL can disappear instead of being returned with a NULL aggregate value.

The stream previously skipped NULL aggregate inputs without registering the group. This is correct for an individual MIN/MAX input, but not for a group whose inputs are all NULL.

There is also an important correctness boundary: nullable MIN/MAX with NULLS FIRST is not monotonic for a bounded aggregation. A group can start at NULL, later become non-NULL, and thereby move to a worse rank. Keeping only limit NULL candidates can therefore discard a group that belongs in the final result.

What changes are included?

  • Track up to limit all-NULL candidates alongside valued TopK groups and emit them for the parent sort to rank and truncate.
  • Correctly convert a tracked NULL group when its first value arrives, or unregister it when that value cannot enter the valued TopK.
  • Skip TopK pushdown for nullable MIN/MAX with NULLS FIRST; regular aggregation is used for exact results. TopK remains enabled for NULLS LAST, non-nullable MIN/MAX inputs, and GROUP BY-only/DISTINCT queries.
  • Replace the single reusable hash-table slot with a free-slot stack. A NULL-to-value conversion can free both the NULL registration and an evicted valued group, so retaining only one slot caused unbounded backing-store growth under repeated conversions.
  • Select NULL-aware insertion once per batch, keeping NULL bookkeeping off the common no-NULL per-row hot path.
  • Add regression coverage for all-NULL groups, mixed NULL/value batches, bounded NULL candidate backfill, evicted groups, hash-table slot reuse, and optimizer plan selection.

Are these changes tested?

Yes. The following passed on the final commit:

  • cargo fmt --all -- --check
  • cargo clippy --all-targets --all-features -- -D warnings
  • TopK physical-plan unit tests (33 tests)
  • aggregates_topk.slt and affected group_by.slt tests
  • The repository's extended workspace command with avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption, including all 495 sqllogic files and extended/fuzz suites

Performance

The topk_aggregate 10-million-row time-series benchmark exposed an initial ~5.5% regression from checking NULL state on every row. Moving NULL handling to a batch-selected slow path removed the measurable regression.

Final 30-sample 95% intervals on the same machine and settings:

  • main: 25.630–26.436 ms
  • this PR: 25.151–27.061 ms

Are there any user-facing changes?

Queries that previously dropped all-NULL groups under ORDER BY <min/max> ... LIMIT now return correct SQL results. Nullable MIN/MAX queries using NULLS FIRST may use regular aggregation rather than the bounded TopK optimization to guarantee correctness. There are no API or configuration changes.

Grouped TopK aggregation previously dropped groups whose MIN/MAX inputs were all NULL. Track bounded all-NULL candidates alongside valued groups and emit them for the final sort to rank and truncate. Handle NULL-to-value transitions, fully reuse freed hash-table slots, and keep NULL bookkeeping off the common no-NULL hot path.

Nullable MIN/MAX with NULLS FIRST is not monotonic for bounded aggregation: a group can move from NULL to a worse non-NULL rank. Skip the TopK pushdown in that case to preserve exact results while retaining it for NULLS LAST and non-nullable inputs.

Closes apache#23440, closes apache#22190.
@u70b3
u70b3 force-pushed the fix/grouped-topk-null-aggregate-values branch from 9238ac1 to a7f380c Compare July 20, 2026 06:58
@github-actions github-actions Bot added the optimizer Optimizer rules label Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizer Optimizer rules physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

1 participant