Skip to content

fix: keep grouped integer DISTINCT counts on the native accumulator path - #25988

Open
QinXi-ai wants to merge 2 commits into
apache:mainfrom
QinXi-ai:codex/25936-numeric-distinct-count
Open

QinXi-ai wants to merge 2 commits into
apache:mainfrom
QinXi-ai:codex/25936-numeric-distinct-count

Conversation

@QinXi-ai

@QinXi-ai QinXi-ai commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #25936.

Rationale for this change

Grouped count(DISTINCT x) over integers currently materializes every distinct (group, value) pair and then counts those rows. The existing native GroupsAccumulator can count directly without that additional aggregate.

What changes are included in this PR?

Preserve a grouped, all-DISTINCT aggregate when every aggregate reports native GroupsAccumulator support for its argument types. Unknown support preserves the existing rewrite. The current specialized DISTINCT count supports Int8/16/32/64 and UInt8/16/32/64; strings, floating-point arguments, global aggregation and mixed aggregates keep their existing behavior.

Add SQL regression coverage and update the affected existing plan snapshots in joins.slt, aggregates_simplify.slt, clickbench.slt, aggregate_memory_spill.slt and TPCH q16.slt.part. The spill regression preserves its result and spill-count assertions. Add a six-case SQL benchmark suite spanning signed/unsigned keys, group count and distinct cardinality, with string and floating-point controls. Each case compares its result against independently constructed distinct-pairs-then-count SQL.

Five adjacent per-query baseline/patched rounds on Windows / Intel Core Ultra 7 258V use four million rows, one partition, the same CPU affinity and AboveNormal priority. Each process runs ten iterations and discards the first as warm-up, leaving 45 measurements per version and query:

Case Baseline median Patched median Reduction Paired-round range
Int64, 2,000 groups, high NDV 167.7 ms 139.9 ms 16.6% 12.1–20.7%
Int64, 2,000 groups, low NDV 34.3 ms 24.3 ms 29.2% 28.5–41.5%
Int64, 8 groups, high NDV 166.1 ms 145.6 ms 12.3% −3.6–15.6%
UInt64, 2,000 groups 175.7 ms 147.2 ms 16.2% −8.2–23.0%
VARCHAR control 171.9 ms 179.6 ms −4.5% −8.1–21.3%
Float64 control 173.0 ms 171.9 ms 0.6% −6.9–8.2%

The issue's first case also reduces query-level pool peak from 191.39 to 168.16 MiB (12.1%). The first two cases improve in every paired round. The small-group and UInt64 cases each reverse in one round, and the unchanged controls show timing noise; the measurements describe this local environment. All six independent result assertions pass.

Reproduce the suite from benchmarks/ with cargo run --profile release-nonlto --bin benchmark_runner -- grouped_count_distinct --iterations 10 --partitions 1. Baseline measurements use 416002a5b908d990f0497ad4fc9d4438e513c210 with the same benchmark files.

What is the testing strategy for this PR?

The optimizer crate passes 921 unit and 26 integration tests. The complete default SQL logic workload (527 files, including parquet_encryption coverage) passes with one test thread and RUST_BACKTRACE=0; TPCH and SQLite external datasets are outside that default workload. The focused SQL run also passes all seven files matched by grouped_count_distinct.slt, single_distinct_to_groupby.slt, joins.slt, aggregates_simplify.slt and clickbench.slt. The new SQL cases cover every supported integer width, duplicates, NULL-only groups, empty input, repeated counts, expression grouping and unchanged fallback plans.

The complete extended workspace command passes on Ubuntu 24.04 / Rust 1.98.1 with RUST_BACKTRACE=1, CI opt-level=1, debug=0, four libtest threads (16 for the SQL runner) and a 65,536 process file-descriptor limit. This includes the complete 121-test fuzz target and default 527-file SQL workload. Strict all-target/all-feature Clippy, formatting, the CLI tests (108 tests) and the full ./dev/rust_lint.sh suite pass, including Rust documentation and the HTML documentation build.

The PR's benchmark-plan CI then found one additional expected-plan change in TPCH Q16. Commit 3ae5962 updates that snapshot; its 32 output lines were checked against the CI's actual plan, and local formatting passed. CI validation of this new head is pending. A local Windows full-Clippy attempt stopped in the unchanged snmalloc-sys C++ build (C2220), so that attempt is not counted as passing.

The Windows extended command encountered failures in unchanged URL-listing, exact floating-point and command-path tests, plus incomplete memory-limited SMJ RSS tests; that command is not reported as passing. The Linux extended run completes those targets successfully. The benchmark executables' original two-aggregate plan and direct DISTINCT plan have been independently checked.

AI assistance was used for implementation and test execution.

Are there any user-facing changes?

Eligible grouped integer DISTINCT counts use the native accumulator. Query results and public APIs are unchanged.

@github-actions github-actions Bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Oct 3, 2026
@codecov-commenter

codecov-commenter commented Oct 3, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.64%. Comparing base (416002a) to head (3ae5962).
⚠️ Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
...fusion/optimizer/src/single_distinct_to_groupby.rs 85.71% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25988      +/-   ##
==========================================
+ Coverage   82.59%   82.64%   +0.04%     
==========================================
  Files        1145     1147       +2     
  Lines      444096   445521    +1425     
  Branches   444096   445521    +1425     
==========================================
+ Hits       366806   368189    +1383     
+ Misses      55021    54971      -50     
- Partials    22269    22361      +92     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Grouped count(DISTINCT x) on numbers takes a slower path

2 participants