kumarUjjawal commented on code in PR #27008:
URL: https://github.com/apache/datafusion/pull/27008#discussion_r4476432381
##########
datafusion/common/benches/with_hashes.rs:
##########
@@ -178,8 +203,13 @@ fn create_null_mask(len: usize) -> NullBuffer
where
StandardUniform: Distribution<bool>,
{
+ create_null_mask_with_density(len, 0.03).expect("should be nulls in
buffer")
+}
+
+/// 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();
- let null_density = 0.03;
let mut builder = NullBufferBuilder::new(len);
for _ in 0..len {
if rng.random::<f32>() < null_density {
Review Comment:
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.
##########
datafusion/common/benches/with_hashes.rs:
##########
@@ -178,8 +203,13 @@ fn create_null_mask(len: usize) -> NullBuffer
where
StandardUniform: Distribution<bool>,
{
+ create_null_mask_with_density(len, 0.03).expect("should be nulls in
buffer")
+}
+
+/// 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();
Review Comment:
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.
##########
datafusion/common/benches/with_hashes.rs:
##########
@@ -160,6 +160,31 @@ fn criterion_benchmark(c: &mut Criterion) {
do_hash_test(b, &arrays);
});
}
+ // 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>>() {
Review Comment:
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.
##########
datafusion/common/benches/with_hashes.rs:
##########
@@ -160,6 +160,31 @@ fn criterion_benchmark(c: &mut Criterion) {
do_hash_test(b, &arrays);
});
}
+ // Apply additional null-density cases to all Int32 dictionary
Review Comment:
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.
##########
datafusion/common/benches/with_hashes.rs:
##########
@@ -188,7 +218,21 @@ where
builder.append_non_null();
}
}
- builder.finish().expect("should be nulls in buffer")
+ builder.finish()
+}
+
+/// 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>(
Review Comment:
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.
##########
datafusion/common/benches/with_hashes.rs:
##########
Review Comment:
https://github.com/apache/datafusion/blob/f44d5e4d36bd9bdfcd68bda293f10b5669735b9d/datafusion/common/benches/with_hashes.rs#L202
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.
##########
datafusion/common/benches/with_hashes.rs:
##########
@@ -188,7 +218,21 @@ where
builder.append_non_null();
}
}
- builder.finish().expect("should be nulls in buffer")
+ builder.finish()
+}
+
+/// 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>(
Review Comment:
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.
##########
datafusion/common/benches/with_hashes.rs:
##########
@@ -160,6 +160,31 @@ fn criterion_benchmark(c: &mut Criterion) {
do_hash_test(b, &arrays);
});
}
+ // 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>>() {
+ for null_percent in [50, 97] {
+ let nullable_array = with_null_keys(dict, null_percent as f32
/ 100.0);
+
+ c.bench_function(
+ &format!("{name}: single, {null_percent}% null keys"),
Review Comment:
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%.
--
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]
