kumarUjjawal commented on code in PR #27008:
URL: https://github.com/apache/datafusion/pull/27008#discussion_r4500449856
##########
datafusion/common/benches/with_hashes.rs:
##########
@@ -242,6 +291,13 @@ pub fn make_rng() -> StdRng {
StdRng::seed_from_u64(42)
}
+/// RNG for null masks. Deliberately not `make_rng()`: sharing its stream makes
+/// null positions a function of the generated values (e.g. which pool string
+/// was picked for a row).
+fn make_null_rng() -> StdRng {
+ StdRng::seed_from_u64(43)
Review Comment:
The null mask seed moved from 42 to 43 for every type, so all existing
'{name}: single, nulls' and '{name}: multiple, nulls' benchmarks now hash
different data under the same names.
Failure scenario: Someone compares main against this branch (critcmp, or the
benchmark bot) for int64, utf8, utf8_view, list_array, struct_array,
run_array_int32 and the others. The 3% 'nulls' cases now put nulls in different
slots and, for strings and dictionaries, keep a different set of non-null
values. Any change in those numbers comes from new input data, not from code.
That mixes noise into a PR that should only add dictionary cases, and it breaks
continuity with older baselines. One fix is to keep seed 42 for the default 3%
case and use the separate stream only for the new densities. The other is to
call out the baseline reset in the PR.
##########
datafusion/common/benches/with_hashes.rs:
##########
@@ -159,6 +180,26 @@ fn criterion_benchmark(c: &mut Criterion) {
];
do_hash_test(b, &arrays);
});
+ for &null_percent in additional_null_densities {
+ let nullable_array = add_nulls(&array, null_percent as f32 /
100.0);
+ c.bench_function(
+ &format!("{name}: single, {null_percent}% nulls"),
+ |b| {
+ do_hash_test(b, std::slice::from_ref(&nullable_array));
+ },
+ );
+ c.bench_function(
+ &format!("{name}: multiple, {null_percent}% nulls"),
+ |b| {
+ let arrays = vec![
Review Comment:
The new 'multiple, 50%/97% nulls' cases hash three clones of the same array,
so all three columns have their nulls on exactly the same rows.
Failure scenario: At 97% density about 97% of rows are null in all three
columns at once, and the other rows are non-null in all three. Multi-column
hashing never sees a row where one column is null and another is not. That
mixed case is what high null density looks like in real data, and it drives the
rehash and combine path. So the 'multiple' high-density numbers mostly repeat
the 'single' result instead of testing the multi-column null path. The existing
3% cases have the same weakness, but the new sweep is where it matters most.
##########
datafusion/common/benches/with_hashes.rs:
##########
@@ -188,12 +236,13 @@ where
builder.append_non_null();
}
}
- builder.finish().expect("should be nulls in buffer")
+ builder.finish()
Review Comment:
The `expect("should be nulls in buffer")` guard is gone, so a case labelled
'nulls' can now quietly run with no nulls at all.**
Failure scenario: The old code panicked if the mask ended up with zero
nulls, which meant a 'nulls' benchmark always had nulls. Now `create_null_mask`
returns None and `add_nulls` builds an array with no null buffer. Take a short
input: the run array values (about 320 entries, so about 10 expected nulls at
3%), a smaller BATCH_SIZE, or a low density added to
`additional_null_densities`. If no slot is drawn null, the 'nulls' case
silently measures the no-nulls fast path and reports it under the wrong name.
Nothing warns. Keeping an assert or expect at the call sites where nulls are
required would keep the old guarantee.
##########
datafusion/common/benches/with_hashes.rs:
##########
@@ -159,6 +180,26 @@ fn criterion_benchmark(c: &mut Criterion) {
];
do_hash_test(b, &arrays);
});
+ for &null_percent in additional_null_densities {
Review Comment:
The density loop copies the single and multiple bench bodies of the default
3% block just above it, almost line for line.
Failure scenario: Two copies of the same 'single' and 'multiple' bench code
now sit inside one `if`. A change to the bench shape, such as how many columns
'multiple' uses, must be made in both places, and the copies can drift apart.
Running one loop over `[DEFAULT_NULL_DENSITY].iter().chain(extra)`, with a
small helper that turns 3% into the old 'nulls' label, would remove the copy
and keep the existing names.
##########
datafusion/common/benches/with_hashes.rs:
##########
@@ -41,46 +44,55 @@ fn criterion_benchmark(c: &mut Criterion) {
name: "int64",
array: primitive_array::<Int64Type>(BATCH_SIZE),
supports_nulls: true,
+ additional_null_densities: &[],
Review Comment:
Each of the 17 `BenchData` entries now repeats `additional_null_densities:
&[]`, but only one entry uses the field.
Failure scenario: The field adds 16 lines that do nothing, and every future
entry must spell it out too. Small constructors would avoid this. For example,
`BenchData::new(name, array)` and `BenchData::no_nulls(name, array)`, plus a
`.with_null_densities(&[50, 97])` call on the dictionary entry. Then only the
case that uses the field mentions it.
##########
datafusion/common/benches/with_hashes.rs:
##########
@@ -30,6 +30,9 @@ struct BenchData {
array: ArrayRef,
/// Union arrays can't have null bitmasks added
supports_nulls: bool,
+ /// Extra null percentages to benchmark, in addition to the default 3%
+ /// ("nulls") case
+ additional_null_densities: &'static [u8],
Review Comment:
`additional_null_densities` is silently ignored when `supports_nulls` is
false, because two fields now describe one thing.
Failure scenario: Someone sets `additional_null_densities: &[50]` on a union
or `null` entry, where `supports_nulls: false`. The loop sits inside `if
supports_nulls`, so no cases are registered and nothing warns. A single
`null_densities: &'static [u8]` field (empty means no null cases) would remove
the overlap. Another option is a debug_assert that the densities list is empty
whenever `supports_nulls` is false.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
