[I] Add a GroupsAccumulator for the native regr_* aggregates [datafusion-comet]

["andygrove (via GitHub)" <gi...@apache.org>]

andygrove opened a new issue, #6886:
URL: https://github.com/apache/datafusion-comet/issues/6886

   ### What is the problem the feature request solves?
   
   `Regr` in `native/spark-expr/src/agg_funcs/regr.rs` only implements `Accumulator`, so grouped `regr_slope`, `regr_intercept`, `regr_r2`, `regr_sxx`, `regr_syy` and `regr_sxy` run through DataFusion's `GroupsAccumulatorAdapter`. The adapter keeps one boxed accumulator per group and calls it once for each group present in a batch. In DataFusion 55.2 it also loops over every group's state on every batch (`invoke_per_accumulator`), so it falls further behind as the number of groups grows.
   
   #4254 measured that path for the same building blocks. With 1M rows in 1000 groups in `CometAggregateExpressionBenchmark`, `covar_samp` took 93 ms and `corr` 105 ms through the adapter, against 56 ms and 64 ms with the aggregate left in Spark (the "Comet (Scan)" case). With `GroupsAccumulator`s they took 26 ms and 32 ms.
   
   The `regr_*` aggregates fall back to Spark in 1.1.0 (#6451). #6076 removes that fallback, so until `Regr` gets a `GroupsAccumulator`, grouped `regr_*` queries are likely to be slower than in 1.1.0.
   
   ### Describe the potential solution
   
   Implement `groups_accumulator_supported` and `create_groups_accumulator` for `Regr`, composing the existing grouped accumulators the way `CorrelationGroupsAccumulator` does:
   
   - `regr_sxx` and `regr_syy`: `VarianceGroupsAccumulator` with the central-moment update, evaluating to `m2`.
   - `regr_sxy`: `CovarianceGroupsAccumulator`, evaluating to the co-moment.
   - `regr_slope` and `regr_intercept`: `CovarianceGroupsAccumulator` plus a `VarianceGroupsAccumulator` over x, which only sees rows where both inputs are non-null when `filter_var_by_pair_nulls` is set (SPARK-48719).
   - `regr_r2`: `CovarianceGroupsAccumulator` plus two `VarianceGroupsAccumulator`s with `with_pearson_update()`, keeping the SPARK-55969 flag and the captured evaluation mode.
   
   The partial buffer layouts need to stay as they are. Adding the six functions to the statistical list in `CometAggregateExpressionBenchmark` would show where they land against Spark before and after.
   
   ### Additional context
   
   This should land before 1.2.0 (#6550). Found while reviewing #6076.
   


-- 
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@datafusion.apache.org.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [I] Add a GroupsAccumulator for the native regr_* aggregates [datafusion-comet]

["rich7420 (via GitHub)" <gi...@apache.org>]

rich7420 commented on issue #6886:
URL: https://github.com/apache/datafusion-comet/issues/6886#issuecomment-6249780262

   take


-- 
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@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org