andygrove commented on code in PR #6830:
URL: https://github.com/apache/datafusion-comet/pull/6830#discussion_r4248065175
##########
native/spark-expr/src/agg_funcs/correlation.rs:
##########
@@ -55,10 +87,41 @@ impl Correlation {
Volatility::Immutable,
),
null_on_divide_by_zero,
+ zero_divisor: ZeroDivisor {
+ eval_mode,
+ expr_id,
+ registry,
+ },
}
}
}
+/// How the `Divide` in Spark's `ck / sqrt(xMk * yMk)` treats a zero
denominator.
+/// It returns null in LEGACY mode and raises DIVIDE_BY_ZERO, carrying the
`corr`
+/// call's query context, in ANSI mode.
+#[derive(Debug, Clone)]
+pub(crate) struct ZeroDivisor {
Review Comment:
`regr_r2` raises the same `DIVIDE_BY_ZERO` from the same kind of `Divide`,
but `regr.rs:397` returns a bare `divide_by_zero_error()`, so its error has no
query context while Spark's points at the `regr_r2(...)` call. I checked by
adding the fragment comparison from the new `corr` test to `statistical
aggregates regr_r2 underflow respects ANSI mode`, and on Spark 3.4 it fails
with `Array() did not equal Array("regr_r2(v, v)")`. Since this PR adds
`ZeroDivisor` and already shares `CapturedEvalMode.ofDivide` between the two
serdes, could `regr_r2` go through `ZeroDivisor` as well? That would mean
passing `spark_expr.expr_id` and the registry to `Regr::new` in the planner,
plus the same fragment assertion in the `regr_r2` test.
--
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]
