Repository navigation
bench: add nullable dictionary hash density benchmarks - #27008
lakshaydevoo-arch wants to merge 11 commits into
Conversation
Rich-T-kid
left a comment
Rich-T-kid
left a comment
There was a problem hiding this comment.
this looks good, we should simplify it to make it look like the other benchmarks in the file
| 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); | ||
| }, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we should add this to existing benchmarks
There was a problem hiding this comment.
makes sense. i will move these cases into the existing dictionary benchmarks to keep everything together.
There was a problem hiding this comment.
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
left a comment
Rich-T-kid
left a comment
There was a problem hiding this comment.
thank you @lakshaydevoo-arch
|
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 |
|
I think its best to do that in a separate branch |
|
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 |
|
@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 |
|
@kumarUjjawal could you please review it |
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @lakshaydevoo-arch
Left few comments please take a look
| /// 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(); |
There was a problem hiding this comment.
The null masks and the dictionary key choice (iter_strings) both start a fresh RNG with seed 42, so which keys get masked is tied to the random draws that picked each string.
| let null_density = 0.03; | ||
| let mut builder = NullBufferBuilder::new(len); | ||
| for _ in 0..len { | ||
| if rng.random::<f32>() < null_density { |
There was a problem hiding this comment.
Every density uses the same seed and the same < null_density test, so the null positions are nested: 3% inside 50% inside 97%.
| let nullable_array = with_null_keys(dict, null_percent as f32 / 100.0); | ||
|
|
||
| c.bench_function( | ||
| &format!("{name}: single, {null_percent}% null keys"), |
There was a problem hiding this comment.
The new cases are named '{name}: single, 50% null keys', while the existing 3% case for the same array is still named '{name}: single, nulls'.
| } | ||
| // 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>>() { |
There was a problem hiding this comment.
The extra cases are chosen by downcasting inside the shared loop instead of being declared on the BenchData entry (e.g. a null_densities: &'static [u8] field).
| /// 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>( |
There was a problem hiding this comment.
with_null_keys is generic over the key type K, but its only caller passes DictionaryArray<Int32Type>.
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
mainfirst will allow the benchmark bot to compare the same workloads before and after the optimization.What changes are included in this PR?
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_hashesCriterion benchmark suite.The benchmark target was previously compiled successfully using:
cargo bench -p datafusion-common --bench with_hashes --no-runThe 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.