Skip to content

bench: expand dictionary group-by coverage - #25303

Merged
kosiew merged 3 commits into
apache:mainfrom
yinli-systems:feature/automatic-dictionary-encoding
Oct 14, 2026
Merged

kosiew merged 3 commits into
apache:mainfrom
yinli-systems:feature/automatic-dictionary-encoding

Conversation

@yinli-systems

@yinli-systems yinli-systems commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

The existing dictionary benchmark only measures pre-encoded dictionaries with cardinality expressed as a fraction of row count. That is not enough to derive a safe automatic encoding policy: native dictionaries, per-batch dictionaries, Utf8View, plain strings, and runtime encoding have materially different costs, and the crossover also depends on absolute NDV, value width, and batch layout.

I rebased this PR onto current main after #25185 merged and reran the relevant matrix, so the evidence includes the new cached dictionary-value-hash path. The crossover remains clear; the cache optimization does not make dictionary grouping universally profitable.

Current-main results (700ad9cf2, release/LTO, Apple M5 Max, Rust 1.98.1; 10M rows, 8,192-row input batches, one target partition; median of iterations 2-6 after two warmups; output normalized back to Utf8):

value bytes NDV Utf8View dict/shared vs view dict/per-batch vs view
16 1,000 50.29 ms 43.48 ms -13.5% 43.52 ms -13.5%
16 10,000 54.24 ms 91.61 ms +68.9% 94.03 ms +73.3%
64 1,000 72.65 ms 47.02 ms -35.3% 47.69 ms -34.4%
64 10,000 79.69 ms 115.85 ms +45.4% 115.04 ms +44.4%

These are local measurements, not a proposed universal threshold. They show why #24117 needs a cost decision based on absolute NDV and value width rather than a blanket dictionary rewrite. Runtime encoding also lost in every earlier measured case, which is why the planned optimizer work targets native Parquet dictionary reads rather than inserting a runtime cast.

What changes are included in this PR?

  • Compare pre-encoded dictionaries, Utf8View, plain strings, and runtime dictionary casts.
  • Configure absolute NDV, value width, and input batch size.
  • Model shared dictionary values and independent per-batch values.
  • Optionally normalize dictionary group keys back to Utf8 in the output so output-type preservation is included in the measurement.
  • Preserve the existing default benchmark behavior and case names.
  • Reject invalid benchmark configurations instead of silently changing the requested workload.
  • Verify that all encodings produce identical grouped counts.

This PR only establishes the benchmark surface needed to evaluate #24117; it does not add an optimizer rule or select a universal threshold.

What is the testing strategy for this PR?

On rebased head 5fb605471:

  • cargo test -p datafusion-benchmarks --lib (153 passed)
  • cargo test -p datafusion-benchmarks --lib --no-default-features (153 passed)
  • cargo clippy -p datafusion-benchmarks --all-targets --no-deps -- -D warnings
  • cargo clippy -p datafusion-benchmarks --all-targets --all-features --no-deps -- -D warnings
  • cargo fmt --all -- --check
  • Current-main CLI benchmark matrix above; every case returned exactly the requested NDV

Are there any user-facing changes?

No DataFusion runtime or public API behavior changes. This only adds benchmark CLI controls.

@codecov-commenter

codecov-commenter commented Sep 14, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.18868% with 63 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.58%. Comparing base (4a5d580) to head (338eaa4).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
benchmarks/src/dict.rs 80.18% 56 Missing and 7 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #25303    +/-   ##
========================================
  Coverage   82.58%   82.58%            
========================================
  Files        1142     1142            
  Lines      440930   441236   +306     
  Branches   440930   441236   +306     
========================================
+ Hits       364126   364397   +271     
- Misses      54801    54825    +24     
- Partials    22003    22014    +11     

☔ 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.

@yinli-systems
yinli-systems force-pushed the feature/automatic-dictionary-encoding branch from 67df706 to 5fb6054 Compare September 23, 2026 18:40
@yinli-systems

Copy link
Copy Markdown
Contributor Author

Rebased onto current main after #25185 and reran the key Utf8View/dictionary matrix. The new hash-cache path is included, but the profitability boundary remains material: dictionaries are 13–35% faster at NDV 1,000 and 44–73% slower at NDV 10,000 in the tested 16B/64B cases. I updated the PR description with the full method and current numbers; both 153-test configurations, fmt, and default/all-feature package clippy pass. @Rich-T-kid @LiaCastaneda, could you check whether this benchmark surface captures the dimensions needed before the #24117 cost rule is reviewed?

@Rich-T-kid

Copy link
Copy Markdown
Contributor

I will try and take a look sometime this week/weekend

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yinli-systems,

Thanks for expanding the benchmark coverage. The new inputs cover the main representation, NDV, value-width, batch-layout, and dictionary-reuse dimensions needed to gather data for a future cost-based policy, and I did not find any blocking issues in this change.

@Rich-T-kid

Copy link
Copy Markdown
Contributor

@yinli-systems you may be interested in this PR #26250, its not in a merge-able state but the results will be very useful when creating #24117

@kosiew
kosiew added this pull request to the merge queue Oct 14, 2026
Merged via the queue into apache:main with commit 1814c6c Oct 14, 2026
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants