Rich-T-kid commented on code in PR #25198:
URL: https://github.com/apache/datafusion/pull/25198#discussion_r3991811733
##########
datafusion/physical-plan/benches/dictionary_group_values.rs:
##########
@@ -247,11 +247,81 @@ fn bench_take_n(c: &mut Criterion) {
group.finish();
}
+/// Batches that share one dictionary values array, as produced downstream of
+/// a repartition or filter: `take`/`filter` clone the values `Arc` and rewrite
+/// only the keys, so the values array is never compacted and its cardinality
+/// reflects the whole upstream stream rather than a single batch.
+///
+/// The other benchmarks here always allocate a fresh values array per batch
+/// and cap cardinality at the batch size, so neither the `Arc` reuse nor the
+/// `cardinality >> rows` regime is covered by them.
+fn bench_shared_values_arc(c: &mut Criterion) {
+ // More batches than `N_BATCHES`: the cost this exercises is paid once per
+ // batch, so a longer run per values array is what a real partition looks
+ // like (thousands of batches sharing one dictionary).
+ const BATCHES: usize = 32;
+
+ let mut group = c.benchmark_group("dict_shared_values_arc");
+ let size = SIZES[0];
+ let mut rng = StdRng::seed_from_u64(SEED);
Review Comment:
I think we can re-use the seed at the top of the file
##########
datafusion/physical-plan/benches/dictionary_group_values.rs:
##########
@@ -247,11 +247,81 @@ fn bench_take_n(c: &mut Criterion) {
group.finish();
}
+/// Batches that share one dictionary values array, as produced downstream of
+/// a repartition or filter: `take`/`filter` clone the values `Arc` and rewrite
+/// only the keys, so the values array is never compacted and its cardinality
+/// reflects the whole upstream stream rather than a single batch.
+///
+/// The other benchmarks here always allocate a fresh values array per batch
+/// and cap cardinality at the batch size, so neither the `Arc` reuse nor the
+/// `cardinality >> rows` regime is covered by them.
+fn bench_shared_values_arc(c: &mut Criterion) {
+ // More batches than `N_BATCHES`: the cost this exercises is paid once per
+ // batch, so a longer run per values array is what a real partition looks
+ // like (thousands of batches sharing one dictionary).
+ const BATCHES: usize = 32;
+
+ let mut group = c.benchmark_group("dict_shared_values_arc");
+ let size = SIZES[0];
+ let mut rng = StdRng::seed_from_u64(SEED);
+
+ for &cardinality in &[size, 100_000, 500_000] {
Review Comment:
nit : I think the jump from 8k -> 100k is a bit steep. could we introduce
32k or 64k as an intermediate cardinality size?
--
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]
