andygrove commented on code in PR #4818:
URL: https://github.com/apache/datafusion-comet/pull/4818#discussion_r4543708188
##########
spark/src/main/scala/org/apache/comet/serde/aggregates.scala:
##########
@@ -1116,6 +1116,44 @@ object CometApproxCountDistinct extends
CometAggregateExpressionSerde[HyperLogLo
}
}
+object CometKurtosis extends CometAggregateExpressionSerde[Kurtosis] {
+
+ // Not marked safe for mixed partial/final: follows the same policy as
`Variance` / `Stddev`,
+ // whose complex `[n, avg, m2, ...]` buffer is not certified compatible
across engines. The
+ // native accumulator does mirror Spark's `[n, avg, m2, m3, m4]` wire
format, so lifting this
+ // to `true` should be considered together with the other `CentralMomentAgg`
serdes.
+
+ override def convert(
+ aggExpr: AggregateExpression,
+ kurtosis: Kurtosis,
+ inputs: Seq[Attribute],
+ binding: Boolean,
+ conf: SQLConf): Option[ExprOuterClass.AggExpr] = {
+ val child = kurtosis.child
+ val childExpr = exprToProto(child, inputs, binding)
+
+ if (childExpr.isDefined) {
+ val builder = ExprOuterClass.Kurtosis.newBuilder()
+ builder.setChild(childExpr.get)
+ builder.setNullOnDivideByZero(kurtosis.nullOnDivideByZero)
+ // Spark's evaluate expression divides by `m2 * m2`, and that `Divide`
picks up its eval
+ // mode from the session. `m2` can be non-zero while `m2 * m2`
underflows to zero, which
+ // the `m2 === 0` guard above it does not catch, so the native side
needs to know whether
+ // that divisor should raise or return null.
+ builder.setAnsiEnabled(conf.ansiEnabled)
Review Comment:
Fixed. `CometKurtosis` reads the mode of the `Divide` in the aggregate's
evaluate expression through `CapturedEvalMode.ofDivide`, as `corr` does, and
falls back if there is none. `kurtosis.sql` now has your repro, with temporary
views: one analyzed with ANSI off and queried with it on returns `NULL`, and
one analyzed with ANSI on and queried with it off raises `DIVIDE_BY_ZERO`. It
fails when I put the session setting back. I also merged `main`: `Kurtosis` is
planned in `planner/aggregate.rs` now, and its proto field is 30, because 23 is
`Mode`'s (0b918885b5).
--
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]
