kumarUjjawal commented on code in PR #27008:
URL: https://github.com/apache/datafusion/pull/27008#discussion_r4463265558
##########
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:
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.
##########
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`, but its only caller
passes `DictionaryArray<Int32Type>`.
##########
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 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).
##########
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 and the same `< null_density` test, so the
null positions are nested: 3% inside 50% inside 97%.
##########
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 named '{name}: single, nulls'.
--
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]
