kosiew commented on code in PR #22322: URL: https://github.com/apache/datafusion/pull/22322#discussion_r3285980151
########## datafusion/physical-plan/src/aggregates/group_values/mod.rs: ########## @@ -30,7 +30,7 @@ use datafusion_expr::EmitTo; pub mod multi_group_by; -mod row; +pub mod row; Review Comment: On the question about `pub mod row`: if the public access is only needed for the benchmark, I think it would be cleaner to keep `mod row;` private and expose only the narrow surface that is needed, for example `pub use row::GroupValuesRows;`. That keeps the internal module layout private instead of publishing the full module path just for benchmark access. ########## datafusion/physical-plan/benches/multi_group_by.rs: ########## @@ -0,0 +1,349 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Benchmarks for multi-column GROUP BY performance comparing vectorized +//! (`GroupValuesColumn`) vs row-based (`GroupValuesRows`) implementations. +//! +//! Motivated by <https://github.com/apache/datafusion/issues/17850> which +//! showed vectorized can regress for low-cardinality, high-row-count scenarios. +//! +//! Uses the direct `GroupValues::intern()` API with identical Int32 data for +//! both implementations — a fair apples-to-apples comparison with the same +//! hashing and data layout. + +use arrow::array::{ArrayRef, Int32Array}; +use arrow::datatypes::{DataType, Field, Schema, SchemaRef}; +use criterion::{BenchmarkId, Criterion, criterion_group, criterion_main}; +use datafusion_physical_plan::aggregates::group_values::GroupValues; +use datafusion_physical_plan::aggregates::group_values::multi_group_by::GroupValuesColumn; +use datafusion_physical_plan::aggregates::group_values::row::GroupValuesRows; +use std::hint::black_box; +use std::sync::Arc; + +const DEFAULT_BATCH_SIZE: usize = 8192; + +fn make_schema(num_cols: usize) -> SchemaRef { + let fields: Vec<Field> = (0..num_cols) + .map(|i| Field::new(format!("col_{i}"), DataType::Int32, false)) + .collect(); + Arc::new(Schema::new(fields)) +} + +fn generate_batches( + num_cols: usize, + num_distinct_groups: usize, + num_rows: usize, + batch_size: usize, +) -> Vec<Vec<ArrayRef>> { + let per_col_card = (num_distinct_groups as f64) + .powf(1.0 / num_cols as f64) + .ceil() as usize; + + let num_batches = num_rows / batch_size; Review Comment: `generate_batches` currently computes `num_batches = num_rows / batch_size`, which drops the remainder rows. With the default `8192` batch size and `num_rows = 1_000_000`, the benchmark generates only `122 * 8192 = 999_424` rows. That means the `grp_1M` / `cols_*_grp_1M` cases can produce at most `999_424` observed groups, not 1,000,000. So while the sequential enumeration change fixes most of the earlier cardinality-label mismatch, the exact 1M threshold cases are still slightly off. Could we either generate the final partial batch, or adjust/report the benchmark labels based on the actual generated row/group counts? -- 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]
