Repository navigation
bench: introduce create_hash benchmarks - #25952
Rich-T-kid wants to merge 2 commits into
Conversation
|
@jayzhan211 could you take a look at this 🙇♂️ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25952 +/- ##
==========================================
+ Coverage 82.55% 82.73% +0.17%
==========================================
Files 1141 1147 +6
Lines 440209 449389 +9180
Branches 440209 449389 +9180
==========================================
+ Hits 363403 371787 +8384
- Misses 54834 54946 +112
- Partials 21972 22656 +684 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
@jayzhan211 sorry for the double ping, PR is relatively small could you take a peak 🙏 . this is related to some optimization for repartionExec |
jayzhan211
left a comment
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @Rich-T-kid , there are some request changes
This duplicates benches/with_hashes.rs. 9 of the 12 single-type cases are already there (and also run with nulls, with 3 columns, and sliced), and make_rng/primitive_array/list_array/map_array are near-copies. Two benches of one function with different data give conflicting baselines (utf8: 22.9 µs here vs 18.6 µs there). with_hashes does the same clear+resize that RepartitionExec does per batch (repartition/mod.rs:1253). The only new coverage is null / list_view / fixed_size_list / mixed-type keys, so please add those to with_hashes.rs and drop the new file. I checked that this compiles, passes clippy, and passes --test:
use arrow::array::{
- Array, ArrayRef, ArrowPrimitiveType, DictionaryArray, GenericStringArray, Int32Array,
- Int64Array, ListArray, MapArray, NullBufferBuilder, OffsetSizeTrait, PrimitiveArray,
- RunArray, StringViewArray, StructArray, UnionArray, make_array,
+ Array, ArrayRef, ArrowPrimitiveType, DictionaryArray, FixedSizeListArray,
+ GenericStringArray, Int32Array, Int64Array, ListArray, ListViewArray, MapArray,
+ NullArray, NullBufferBuilder, OffsetSizeTrait, PrimitiveArray, RunArray,
+ StringViewArray, StructArray, UnionArray, make_array,
};
@@ fn criterion_benchmark(c: &mut Criterion) {
array: create_run_array::<Int32Type>(BATCH_SIZE),
supports_nulls: true,
},
+ BenchData {
+ name: "null",
+ array: Arc::new(NullArray::new(BATCH_SIZE)),
+ supports_nulls: false,
+ },
+ BenchData {
+ name: "list_view_array",
+ array: list_view_array(BATCH_SIZE),
+ supports_nulls: true,
+ },
+ BenchData {
+ name: "fixed_size_list_array",
+ array: fixed_size_list_array(BATCH_SIZE),
+ supports_nulls: true,
+ },
];
@@
-criterion_group!(benches, criterion_benchmark, sliced_array_benchmark);
+fn list_view_array(num_rows: usize) -> ArrayRef {
+ let elements_per_row = 5;
+ let values = primitive_array::<Int64Type>(num_rows * elements_per_row);
+ let offsets: ScalarBuffer<i32> = (0..num_rows)
+ .map(|i| (i * elements_per_row) as i32)
+ .collect();
+ let sizes: ScalarBuffer<i32> =
+ (0..num_rows).map(|_| elements_per_row as i32).collect();
+ Arc::new(ListViewArray::new(
+ Arc::new(Field::new("item", DataType::Int64, true)),
+ offsets,
+ sizes,
+ values,
+ None,
+ ))
+}
+
+fn fixed_size_list_array(num_rows: usize) -> ArrayRef {
+ let list_size = 4;
+ Arc::new(FixedSizeListArray::new(
+ Arc::new(Field::new("item", DataType::Int64, true)),
+ list_size as i32,
+ primitive_array::<Int64Type>(num_rows * list_size),
+ None,
+ ))
+}
+
+/// Heterogeneous key columns, as hashed by `RepartitionExec` / joins / aggregates
+fn mixed_columns_benchmark(c: &mut Criterion) {
+ let pool = StringPool::new(100, 64);
+ let int64 = primitive_array::<Int64Type>(BATCH_SIZE);
+ let utf8 = pool.string_array::<i32>(BATCH_SIZE);
+ let utf8_view = pool.string_view_array(BATCH_SIZE);
+ let dict = pool.dictionary_array::<Int32Type>(BATCH_SIZE);
+ let cases = [
+ (
+ "3 columns",
+ vec![int64.clone(), utf8.clone(), utf8_view.clone()],
+ ),
+ (
+ "5 columns",
+ vec![int64.clone(), utf8, utf8_view, dict, int64],
+ ),
+ ];
+ for (name, arrays) in cases {
+ c.bench_function(&format!("mixed: {name}"), |b| do_hash_test(b, &arrays));
+ }
+}
+
+criterion_group!(
+ benches,
+ criterion_benchmark,
+ sliced_array_benchmark,
+ mixed_columns_benchmark
+);| )) | ||
| } | ||
|
|
||
| fn fixed_size_list_array() -> ArrayRef { |
There was a problem hiding this comment.
primitive_array() returns 8192 values, and with list_size = 4 that is a 2048-row FixedSizeListArray hashed into an 8192-slot buffer. This case therefore measures a quarter of the rows of every other case. It only runs because hash_fixed_list_array skips the buffer-length check that hash_array_primitive has (hash_utils.rs:316). Running the same shape through with_hashes's length check fails with left: 2048, right: 8192.
fn fixed_size_list_array() -> ArrayRef {
- let list_size = 4i32;
+ let list_size = 4;
+ let mut rng = make_rng();
+ let values: Int64Array = (0..BATCH_SIZE * list_size)
+ .map(|_| Some(rng.random::<i64>()))
+ .collect();
Arc::new(FixedSizeListArray::new(
Arc::new(Field::new("item", DataType::Int64, true)),
- list_size,
- primitive_array::<Int64Type>(),
+ list_size as i32,
+ Arc::new(values),
None,
))
}There was a problem hiding this comment.
thanks for catching this. updated
|
nice that makes this simpler. I also added 1 case that benchmarks creating multi-key hash as that isn't already covered. @jayzhan211 what did u use for green/red highlighting? looks very nice |
3 participants
Which issue does this PR close?
Rationale for this change
create_hashes()has no dedicated benchmarks, making it hard to measure the impact of changes to hashing logic. covers all existing datatypesWhat changes are included in this PR?
adds benchmarks for every datatype thats covered by
create_hashes()What is the testing strategy for this PR?
n/a
Are there any user-facing changes?
no