[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2375: Grouped Aggregate in row format

[GitBox <gi...@apache.org>]

alamb commented on code in PR #2375:
URL: https://github.com/apache/arrow-datafusion/pull/2375#discussion_r866068228


##########
datafusion/core/Cargo.toml:
##########
@@ -66,7 +64,7 @@ datafusion-data-access = { path = "../../data-access", version = "1.0.0" }
 datafusion-expr = { path = "../expr", version = "7.0.0" }
 datafusion-jit = { path = "../jit", version = "7.0.0", optional = true }
 datafusion-physical-expr = { path = "../physical-expr", version = "7.0.0" }
-datafusion-row = { path = "../row", version = "7.0.0", optional = true }
+datafusion-row = { path = "../row", version = "7.0.0" }

Review Comment:
   👍 



##########
datafusion/core/src/physical_plan/hash_utils.rs:
##########
@@ -265,7 +265,42 @@ pub fn create_hashes<'a>(
     for hash in hashes_buffer.iter_mut() {
         *hash = 0
     }
-    return Ok(hashes_buffer);
+    Ok(hashes_buffer)
+}
+
+/// Test version of `create_row_hashes` that produces the same value for
+/// all hashes (to test collisions)
+///
+/// See comments on `hashes_buffer` for more details
+#[cfg(feature = "force_hash_collisions")]
+pub fn create_row_hashes<'a>(
+    _rows: &[Vec<u8>],
+    _random_state: &RandomState,
+    hashes_buffer: &'a mut Vec<u64>,
+) -> Result<&'a mut Vec<u64>> {
+    for hash in hashes_buffer.iter_mut() {
+        *hash = 0
+    }
+    Ok(hashes_buffer)
+}
+
+/// Test version of `create_row_hashes` that produces the same value for
+/// all hashes (to test collisions)
+///
+/// See comments on `hashes_buffer` for more details

Review Comment:
   this comment does not seem to be accurate -- it is the *non*test version



##########
datafusion/physical-expr/src/aggregate/mod.rs:
##########
@@ -77,6 +79,18 @@ pub trait AggregateExpr: Send + Sync + Debug {
     fn name(&self) -> &str {
         "AggregateExpr: default name"
     }
+
+    /// If the aggregate expression is supported by row format
+    fn accumulator_v2_supported(&self) -> bool {
+        false
+    }
+
+    fn create_accumulator_v2(

Review Comment:
   ```suggestion
       /// the accumulator used to accumulate values from the expressions that uses Row format. 
       /// the accumulator expects the same number of arguments as `expressions` and must
       /// return states with the same description as `state_fields`
       fn create_accumulator_v2(
   ```



##########
datafusion/physical-expr/src/aggregate/accumulator_v2.rs:
##########
@@ -0,0 +1,42 @@
+// 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.
+
+//! Accumulator over row format
+
+use arrow::array::ArrayRef;
+use datafusion_common::{Result, ScalarValue};
+use datafusion_row::accessor::RowAccessor;
+use std::fmt::Debug;
+
+pub trait AccumulatorV2: Send + Sync + Debug {

Review Comment:
   What about calling this `RowAccumulator` or `AccumulatorRow` so the name hints at the difference between it and `Accumlator`
   
   I also think it would help to add some comments here:
   
   1. This is a row based accumulator where the internal aggregate state is stored using row format.



##########
datafusion/physical-expr/src/aggregate/mod.rs:
##########
@@ -77,6 +79,18 @@ pub trait AggregateExpr: Send + Sync + Debug {
     fn name(&self) -> &str {
         "AggregateExpr: default name"
     }
+
+    /// If the aggregate expression is supported by row format

Review Comment:
   This approach of adding two new functions to the `Aggregate` trait has the very nice property that it is backwards compatible. However, I wonder if we can clarify:
   1. Do aggregates need to implement `accumulator` if they also implement `create_accumulator_v2`?
   2. What is the benefit of creating accumulator v2 (it is faster?)



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org