zhuqi-lucas commented on code in PR #23344:
URL: https://github.com/apache/datafusion/pull/23344#discussion_r3643366046
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -215,6 +215,11 @@ pub struct GroupValuesColumn<const STREAMING: bool> {
/// [`GroupValuesRows`]: crate::aggregates::group_values::GroupValuesRows
group_values: Vec<Box<dyn GroupColumn>>,
+ /// Indices into `group_values` ordered cheapest to most expensive
comparison
+ /// cost. Built once in `try_new` from the schema so that
`vectorized_equal_to`
+ /// eliminates rows with cheap columns before paying the cost of expensive
ones.
+ compare_order: Vec<usize>,
Review Comment:
Only `vectorized_equal_to_all` consumes this. The scalarized paths at lines
400 / 861 / 878 still iterate schema order — see the review body for the
corresponding fix in each.
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -293,6 +300,12 @@ impl<const STREAMING: bool> GroupValuesColumn<STREAMING> {
/// `clear_shrink`). Centralising it keeps the post-condition that
/// `self.group_values` always contains exactly one builder per schema
/// field outside of those transient drain points.
+ fn build_group_compare_order(schema: &Schema) -> Vec<usize> {
+ let mut order: Vec<usize> = (0..schema.fields().len()).collect();
+ order.sort_by_key(|&i| compare_tier(schema.field(i).data_type()));
Review Comment:
`slice::sort_by_key` is stable — within a tier, columns keep schema order.
Non-obvious; one-line comment would keep someone later from switching to
`sort_unstable_by_key` and silently breaking determinism.
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -651,8 +664,8 @@ impl<const STREAMING: bool> GroupValuesColumn<STREAMING> {
equal_to_results.truncate(0);
equal_to_results.append_n(n, true);
- for (col_idx, group_col) in self.group_values.iter().enumerate() {
- group_col.vectorized_equal_to(
+ for &col_idx in &self.compare_order {
Review Comment:
Vectorized path good ✅ — same pattern needs applying to the three scalarized
loops (see review body for the fixes at lines 400 / 861 / 878; both `STREAMING
= true` and the non-streaming fallback via `scalarized_intern_remaining` at
line 495 hit them).
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -1076,6 +1089,40 @@ fn make_group_column(field: &Field) -> Result<Box<dyn
GroupColumn>> {
);
Ok(v.into_iter().next().unwrap())
}
+/// Returns a comparison cost tier for `data_type` (1 = cheapest, 3 = most
expensive).
+/// Used to order columns in [`GroupValuesColumn::compare_order`] so cheap
comparisons
+/// eliminate rows before expensive ones are evaluated.
+/// see <https://github.com/apache/datafusion/issues/23342>
+fn compare_tier(data_type: &DataType) -> u8 {
Review Comment:
- PR body says tiers `1 → 5` but this returns `1 / 2 / 3` — please align.
- `_ => 3` is unreachable: `try_new` calls `build_group_columns?` before
`build_group_compare_order`, and `make_group_column` rejects any type outside
tier 1 / tier 2 upstream. Worth a one-line comment saying so — today it reads
like the branch might fire.
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -293,6 +300,12 @@ impl<const STREAMING: bool> GroupValuesColumn<STREAMING> {
/// `clear_shrink`). Centralising it keeps the post-condition that
/// `self.group_values` always contains exactly one builder per schema
/// field outside of those transient drain points.
+ fn build_group_compare_order(schema: &Schema) -> Vec<usize> {
Review Comment:
The doc block just above (`Build one fresh GroupColumn per field ...`)
describes `build_group_columns`, not this fn — it got orphaned when the new fn
was inserted. Please move this fn after `build_group_columns`, or give it its
own doc.
--
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]
