andygrove commented on code in PR #5736:
URL: https://github.com/apache/datafusion-comet/pull/5736#discussion_r3944570485
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -740,6 +740,8 @@ object QueryPlanSerde extends Logging with CometExprShim
with CometTypeShim {
}
}
+ // Aggregate functions bypass exprToProto: their arguments and filters are
independent roots
Review Comment:
This reads as though it contradicts itself, since aggregates both bypass
`exprToProto` and must enter through it. I think the point is that this method
does not promote the aggregate tree, so the arguments and filters are
independent roots. Maybe something like: "This method does not promote the
aggregate tree, so its arguments and filters are independent roots and
`AggExprSerde` implementations must serialize them through `exprToProto`."
Worth making it a scaladoc block too, so it renders next to the two
entry-point docs you add below.
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -913,6 +917,11 @@ object QueryPlanSerde extends Logging with CometExprShim
with CometTypeShim {
* Convert a Spark expression to a protocol-buffer representation of a
native Comet/DataFusion
* expression.
*
+ * The caller owns decimal promotion: this method serializes children of an
already-promoted
Review Comment:
Worth thinking about what backstops this contract now. Before this change
the array and bitwise serdes re-promoted their children, so they were safe
against ever handing unwrapped decimal arithmetic to the native side even if a
later edit synthesized some. After it, that safety is a scaladoc sentence, and
the failure mode is quiet. I patched promotion out of `exprToProto` and
`array_contains(array(c / d), c / d)` on `DECIMAL(38,6)` returns `true` where
Spark returns `null`, because native division's `i128::MAX` overflow sentinel
is only converted to NULL by `CheckOverflow`. No error, no fallback, just a
wrong boolean. Multiply and add at scale 0 do not diverge, so division is the
case that bites.
Nothing violates the contract today. `CometAtan2` is the only serde that
synthesizes arithmetic onto the internal path, and `atan2` arguments are always
double. So this is about the next serde, not about this PR.
Would a testing-only assertion be worth adding? I tried it and there are two
traps worth passing on. `Utils` is package-private to `org.apache.spark`, so
the helper has to live somewhere like `DecimalPrecision` rather than in
`org.apache.comet.serde`. And a plain `assert(promote(expr) == expr)` here
fires on four existing tests, because `CometCheckOverflow` legitimately passes
down the bare arithmetic node it just unwrapped. Exempting decimal
`BinaryArithmetic` silences those but also silences the real thing, which I
confirmed by injecting a violation.
What did work was keeping the strict assertion and giving
`CometCheckOverflow` a `private[serde] exprToProtoInternalUnchecked`: clean
across 80 tests in 5 suites with no trips, and it fires on an injected
`Add(child, child)` in `CometArrayContains`. Happy for that to be a follow-up
rather than something this PR carries.
##########
spark/src/test/scala/org/apache/spark/sql/comet/CometDecimalPromotionSuite.scala:
##########
@@ -62,9 +63,7 @@ class CometDecimalPromotionSuite extends CometTestBase {
DecimalPrecision.promote(promoted) == promoted,
s"$name promotion is not idempotent: $promoted")
- // This proto-shape check relies on CometArrayContains re-entering
exprToProto for its
- // children. If https://github.com/apache/datafusion-comet/issues/5248
changes that,
- // re-point it to another recursively serializing serde.
+ // Recursive child serialization must retain exactly one equivalent
overflow wrapper.
Review Comment:
The comment being removed here was addressed to exactly this change. It said
the proto-shape check relies on `CometArrayContains` re-entering `exprToProto`
for its children, and to re-point it at another recursively serializing serde
if #5248 changed that. Now that `array_contains` promotes once, the "Duplicate
outer CheckOverflow" and "Duplicate inner CheckOverflow" assertions in the new
test cannot trip through that path any more, since reaching them needs the
double promotion that no longer happens there. `CometSlice` and
`CometArrayJoin` still double-promote, so pointing the shape check at one of
those would keep the coverage the comment was guarding.
The other gap is that both tests assert only on proto structure, and the
property that actually breaks is the value. I wrote a value-level suite while
reviewing and it seems worth having: 46 cases putting overflowing
`DECIMAL(38,0)` multiply and overflowing `DECIMAL(38,6)` divide under every
array and bitwise serde this PR touches, read from Parquet so the scan is
native, checked with `checkSparkAnswerAndOperator` for ANSI off and
`checkSparkAnswerMaybeThrows` for ANSI on. It passes on this branch and on its
parent, and fails two of three tests if promotion is removed. Happy to hand it
over if useful.
Smaller thing: the `Seq(true, false).foreach { binding => ... }` loop below
doubles the runtime but no assertion inside it depends on `binding`, so it is
not really covering the bound and unbound cases the description mentions.
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -999,12 +999,12 @@ object CometSequence extends
CometExpressionSerde[Sequence] with CodegenDispatch
expr: Sequence,
inputs: Seq[Attribute],
binding: Boolean): Option[ExprOuterClass.Expr] = {
- val startExprProto = exprToProto(expr.start, inputs, binding)
- val stopExprProto = exprToProto(expr.stop, inputs, binding)
+ val startExprProto = exprToProtoInternal(expr.start, inputs, binding)
Review Comment:
`getSupportLevel` already requires every argument to be a `Literal`,
`Attribute` or `BoundReference` via `argsAreLiteralsOrRefs`, so `convert` never
sees a decimal arithmetic child here and promoting a single leaf is free. These
three lines are fine and consistent, just worth knowing they are not part of
the measured win. My corpus confirms it: every `sequence` case serialized as
`jvm_scalar_udf` through the codegen dispatcher rather than `spark_sequence`,
because the cast-wrapped arguments fail the literals-or-refs test.
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -498,7 +498,7 @@ object CometSlice extends CometExpressionSerde[Slice] {
inputs: Seq[Attribute],
binding: Boolean): Option[ExprOuterClass.Expr] = {
val elementType = expr.x.dataType.asInstanceOf[ArrayType].elementType
- val arrayExprProto = exprToProto(expr.x, inputs, binding)
+ val arrayExprProto = exprToProtoInternal(expr.x, inputs, binding)
val startExprProto = exprToProto(Cast(expr.start, LongType), inputs,
binding)
Review Comment:
The new contract says literals and non-arithmetic wrappers can use
`exprToProtoInternal`, and only synthesized arithmetic needs the public entry
point. Eight calls in this file still take the public path, and two of them
look like the case this PR is fixing.
Here, `expr.x` goes through the internal path but `expr.start` and
`expr.length` get wrapped in synthesized `Cast(..., LongType)` and sent through
`exprToProto`, which re-promotes everything under `expr.start`. Reachable with
something like `slice(arr, CAST(a + b AS INT), 2)` on decimal columns. A `Cast`
is not arithmetic, so by your own rule these belong on the internal path.
`CometArrayJoin` has the same shape in a tighter space:
`nullReplacementExpr` is serialized internally at line 412, then
`IsNull(nullReplacementExpr)` publicly at line 423, so the same subexpression
takes both entry points seven lines apart. The remaining literal calls are at
lines 84, 424, 700, 816 and 887.
`CometArraysZip` at line 886 already routes its synthesized
`IsNotNull(...)`/`And` guard through `exprToProtoInternal`, so the pattern is
right there in the file. Would you sweep these too? The protos come out
identical either way, so it is consistency and saved traversal rather than a
correctness fix.
--
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]
