bench: add nullable dictionary hash density benchmarks by lakshaydevoo-arch · Pull Request #27008 · apache/datafusion

lakshaydevoo-arch · 2026-11-02T07:28:00Z

Important

Fork CI setup: For pull requests from a fork to main, enable workflows in your fork's Actions tab before you push.
See the fork CI setup guide.

Which issue does this PR close?

Part of #26848.

Related to #26966.

Rationale for this change

This PR separates the additional dictionary hashing benchmarks from the optimization in #26966, as requested during review.

Having the benchmarks in main first will allow the benchmark bot to compare the same workloads before and after the optimization.

What changes are included in this PR?

Add Int32 dictionary hashing benchmarks with 50% and 97% null keys.
Cover both single-column and multi-column hashing.
Keep the existing 0% and 3% null-key benchmarks unchanged.
Reuse deterministic null-key generation for reproducible workloads.

The benchmarks use 8,192 rows and the existing dictionary value pool.

No hashing implementation changes are included.

What is the testing strategy for this PR?

The new benchmarks are registered in the existing with_hashes Criterion benchmark suite.

The benchmark target was previously compiled successfully using:

cargo bench -p datafusion-common --bench with_hashes --no-run

The additional cases were also executed locally and by the benchmark bot as part of #26966.

This PR adds benchmark coverage only. Performance comparisons will be made against these benchmarks once they are available in the baseline.

Are there any user-facing changes?

No. This PR only adds benchmarks.

Rich-T-kid

this looks good, we should simplify it to make it look like the other benchmarks in the file

Rich-T-kid · 2026-11-03T03:51:19Z

+fn dictionary_benchmark(c: &mut Criterion) {
+    let pool = StringPool::new(100, 64);
+    let dict = pool.typed_dictionary_array::<Int32Type>(BATCH_SIZE);
+    let name = "dictionary_utf8_int32";
+    for null_percent in [50, 97] {
+        let array = with_null_keys(&dict, null_percent as f32 / 100.0);
+        c.bench_function(&format!("{name}: single, {null_percent}% null keys"), |b| {
+            do_hash_test(b, std::slice::from_ref(&array));
+        });
+        c.bench_function(
+            &format!("{name}: multiple, {null_percent}% null keys"),
+            |b| {
+                let arrays = vec![array.clone(), array.clone(), array.clone()];
+                do_hash_test(b, &arrays);
+            },
+        );
+    }
+}


I think we should add this to existing benchmarks

makes sense. i will move these cases into the existing dictionary benchmarks to keep everything together.

Updated. I've moved the 50% and 97% null-key cases into the existing criterion_benchmark() loop and removed the separate benchmark function, the existing benchmark cases remain unchanged.

Co-authored-by: RIchard Baah <137434454+Rich-T-kid@users.noreply.github.com>

Add comments for future null-density cases in benchmarks.

Rich-T-kid

thank you @lakshaydevoo-arch

lakshaydevoo-arch · 2026-11-03T14:35:56Z

i have found another issue also , it's a sorting bug that when adding another column it distorts the global sorting order, should i start investigation on it in another branch
or wait for my these remaining optimization pr's to merge?

Rich-T-kid · 2026-11-03T14:41:20Z

I think its best to do that in a separate branch

lakshaydevoo-arch · 2026-11-03T14:45:59Z

I was mainly hesitant about opening another PR while my current optimization PR is still under review, as I don't want to add unnecessary review overhead for the maintainers, so will be
opening another focused PR for that issue and investigate more to that until these gets merged.

lakshaydevoo-arch · 2026-11-03T18:16:24Z

@Rich-T-kid when will it merge?

Rich-T-kid · 2026-11-03T19:08:03Z

@Rich-T-kid when will it merge?

@lakshaydevoo-arch 😅 i'm not a committer. i think @kumarUjjawal would have the most context to review these changes, if he has time

lakshaydevoo-arch · 2026-11-03T19:21:07Z

@kumarUjjawal could you please review it

kumarUjjawal

Thank you @lakshaydevoo-arch

Left few comments please take a look

lakshaydevoo-arch · 2026-11-04T16:11:13Z

@kumarUjjawal i just wanted to ask that when will it be merged because my another main performance optimization PR #26966 depends on these benchmarks

kumarUjjawal

@lakshaydevoo-arch Please don't resolve the open threads. I will do it once I am satisfied with the code changes.

kumarUjjawal · 2026-11-05T03:56:02Z

-    let null_density = 0.03;
    let mut builder = NullBufferBuilder::new(len);
    for _ in 0..len {
        if rng.random::<f32>() < null_density {


Every density uses the same seed (42) and the same rng.random::<f32>() < null_density test. So the null positions are nested: the 3% mask from add_nulls is inside the 50% mask, which is inside the 97% mask. raised this in threads/r4463265560.md; the thread is resolved but the code is not changed.

kumarUjjawal · 2026-11-05T03:56:02Z

+/// Returns a random null mask where each slot is null with probability
+/// `null_density`, or `None` if no slot ended up null
+fn create_null_mask_with_density(len: usize, null_density: f32) -> Option<NullBuffer> {
    let mut rng = make_rng();


create_null_mask_with_density calls make_rng(), which uses seed 42. iter_strings uses the same seed to pick each dictionary key, so the draws that decide which keys become null come from the same random stream that chose the strings. kumarUjjawal raised this in threads/r4463265558.md; it is resolved but not changed.

Failure scenario: At 97% nulls only about 250 keys stay non-null, and which ones stay depends on the same random draws that chose the strings. The values that are left can be skewed toward a few dictionary entries instead of a uniform sample. That changes how much value-hash work is done and makes the high-density numbers unrepresentative.

kumarUjjawal · 2026-11-05T03:56:02Z

                do_hash_test(b, &arrays);
            });
        }
+        // Apply additional null-density cases to all Int32 dictionary


The comment says the density cases apply to dictionary benchmarks 'including any added in the future', but the downcast only matches DictionaryArray<Int32Type>.**

Failure scenario: Someone adds a dictionary_utf8_int8 or int64 key case to cases and expects the 50%/97% density cases from this comment. The downcast returns None and those cases are silently never registered, and nothing warns about it.

kumarUjjawal · 2026-11-05T03:56:02Z

+                let nullable_array = with_null_keys(dict, null_percent as f32 / 100.0);
+
+                c.bench_function(
+                    &format!("{name}: single, {null_percent}% null keys"),


The new cases are named '{name}: single, 50% null keys' while the existing 3% case for the same array is still '{name}: single, nulls', so one density sweep is split across two naming schemes. raised this in threads/r4463265565.md; it is resolved but not changed.

Failure scenario: When you filter or compare the dictionary null-density results with critcmp or the benchmark bot (for example --bench with_hashes '% null keys'), the 3% data point is left out, and the results don't show that 'nulls' means 3%.

kumarUjjawal · 2026-11-05T03:56:02Z

        }
+        // Apply additional null-density cases to all Int32 dictionary
+        // benchmarks, including any added in the future.
+        if let Some(dict) = array.as_any().downcast_ref::<DictionaryArray<Int32Type>>() {


The extra density cases are chosen by downcasting inside the shared loop, not declared on the BenchData entry (for example a null_densities: &'static [u8] field like the existing supports_nulls). kumarUjjawal raised this in threads/r4463265572.md; it is resolved but not changed.

Failure scenario: The type check hides which cases get extra benchmarks. Adding density cases to another array type means adding another downcast branch to the shared loop, not setting a field on its entry. This also leads directly to the misleading 'future' comment above.

kumarUjjawal · 2026-11-05T03:56:02Z

+/// Returns `dict` with a new random null mask on its keys, where each key is null
+/// with probability `null_density`. Any existing key nulls are replaced, and the
+/// key and value buffers are shared rather than copied
+fn with_null_keys<K: ArrowDictionaryKeyType>(


with_null_keys repeats what add_nulls already does. For a dictionary, into_data().into_builder().nulls(...) sets the key nulls in the same way. Giving add_nulls (or create_null_mask) a density parameter would remove the new helper and the Option/expect split.

Failure scenario: Now two code paths build nullable dictionaries: add_nulls for 3% and with_null_keys for 50%/97%. They can drift apart in how they handle buffers, and they are a second place to update if null generation changes, for example to fix the seed problem above.

kumarUjjawal · 2026-11-05T03:56:02Z

+/// Returns `dict` with a new random null mask on its keys, where each key is null
+/// with probability `null_density`. Any existing key nulls are replaced, and the
+/// key and value buffers are shared rather than copied
+fn with_null_keys<K: ArrowDictionaryKeyType>(


with_null_keys is generic over the key type K: ArrowDictionaryKeyType, but its only caller passes DictionaryArray<Int32Type>. kumarUjjawal raised this in threads/r4463265575.md; it is resolved but not changed.

Failure scenario: A generic that is never used with more than one type adds complexity. Because the caller is limited to Int32, the generic also suggests coverage of other key types that does not exist.

kumarUjjawal · 2026-11-05T03:56:05Z

datafusion/datafusion/common/benches/with_hashes.rs

Line 202 in f44d5e4

fn create_null_mask(len: usize) -> NullBuffer

create_null_mask keeps the bound where StandardUniform: Distribution<bool>. It is always true and has no purpose, now that the function is a one-line wrapper around create_null_mask_with_density.

Failure scenario: The leftover bound is noise in a function the diff touches, and it makes readers wonder why the wrapper needs it when the real implementation does not.

bench: add nullable dictionary hash density benchmarks

9b2ee2e

github-actions Bot added the common Related to common crate label Nov 2, 2026

lakshaydevoo-arch mentioned this pull request Nov 2, 2026

perf: skip null keys when scattering nullable dictionary hashes #26966

Open

lakshaydevoo-arch added 2 commits November 3, 2026 01:55

Merge branch 'main' into bench/26848-dictionary-null-density

5e23729

Merge branch 'main' into bench/26848-dictionary-null-density

274f249

Rich-T-kid reviewed Nov 3, 2026

View reviewed changes

lakshaydevoo-arch added 3 commits November 3, 2026 10:21

Merge branch 'main' into bench/26848-dictionary-null-density

5de2c9c

refactor: integrate null-density cases into existing benchmarks

5cb5ed5

Merge branch 'main' into bench/26848-dictionary-null-density

f9de53e

Rich-T-kid reviewed Nov 3, 2026

View reviewed changes

Comment thread datafusion/common/benches/with_hashes.rs Resolved

lakshaydevoo-arch and others added 3 commits November 3, 2026 19:52

Update datafusion/common/benches/with_hashes.rs

190f145

Co-authored-by: RIchard Baah <137434454+Rich-T-kid@users.noreply.github.com>

Merge branch 'main' into bench/26848-dictionary-null-density

7099810

Enhance comments in with_hashes.rs for clarity

0792ce8

Add comments for future null-density cases in benchmarks.

Rich-T-kid approved these changes Nov 3, 2026

View reviewed changes

lakshaydevoo-arch added 2 commits November 3, 2026 15:02

style: format dictionary benchmark changes

d4f90cd

Merge branch 'main' into bench/26848-dictionary-null-density

7712e31

kumarUjjawal reviewed Nov 4, 2026

View reviewed changes

lakshaydevoo-arch added 2 commits November 4, 2026 14:30

Merge branch 'main' into bench/26848-dictionary-null-density

8433dcf

Merge branch 'main' into bench/26848-dictionary-null-density

f44d5e4

kumarUjjawal reviewed Nov 5, 2026

View reviewed changes

Conversation

lakshaydevoo-arch commented Nov 2, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

What is the testing strategy for this PR?

Are there any user-facing changes?

Uh oh!

Rich-T-kid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Rich-T-kid left a comment

Choose a reason for hiding this comment

Uh oh!

lakshaydevoo-arch commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rich-T-kid commented Nov 3, 2026

Uh oh!

lakshaydevoo-arch commented Nov 3, 2026

Uh oh!

lakshaydevoo-arch commented Nov 3, 2026

Uh oh!

Rich-T-kid commented Nov 3, 2026

Uh oh!

lakshaydevoo-arch commented Nov 3, 2026

Uh oh!

kumarUjjawal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lakshaydevoo-arch commented Nov 4, 2026

Uh oh!

kumarUjjawal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lakshaydevoo-arch commented Nov 3, 2026 •

edited

Loading