peterxcli commented on code in PR #5736:
URL: https://github.com/apache/datafusion-comet/pull/5736#discussion_r3944619302
##########
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:
Updated in d3c44512f. This is now a Scaladoc block that states directly that
aggExprToProto does not promote the aggregate tree, and that its arguments and
filters are independent roots serialized through exprToProto.
##########
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:
Keeping the testing-only checked/unchecked entry points as a follow-up, as
you suggested, to handle the legitimate CometCheckOverflow exception
separately. This revision adds value-level regression coverage with codegen
dispatch disabled: 46 expression cases per ANSI mode over native Parquet scans.
Temporarily bypassing promotion in exprToProto fails the nested-shape test and
both value tests, including wrong LEGACY decimal-division results and a missing
ANSI division error. With promotion restored, all 64 focused tests pass.
##########
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:
Swept all eight remaining calls in arrays.scala in d3c44512f: both Slice
casts, ArrayJoin's IsNull guard, and the five literal calls now use
exprToProtoInternal. No public exprToProto calls remain in that file. The value
regressions exercise decimal arithmetic under the Slice start/length casts and
ArrayJoin null-replacement guard.
##########
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:
Agreed. These Sequence changes are consistency-only. The benchmark covers
Reverse/ArrayContains nesting and BitwiseNot nesting; it does not attribute a
performance gain to Sequence.
##########
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:
Addressed in d3c44512f. The original test now deliberately passes
DecimalPrecision.promote(expression) into the public exprToProto entry point,
so it exercises re-promotion explicitly without depending on recursive calls
that this PR removes. The binding loop now asserts the bound index or unbound
name. Added 46 value cases per ANSI mode using overflowing DECIMAL(38,0)
multiply and DECIMAL(38,6) divide over native Parquet scans, with codegen
dispatch disabled. Disabling promotion makes both value tests fail on decimal
division; restoring it gives 64 passing focused tests. The ArrayJoin
replacement case uses a nullable column-based delimiter to avoid a separate
Spark codegen null-flag issue reproduced on the unchanged serializers.
--
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]
