mbutrovich commented on code in PR #5201:
URL: https://github.com/apache/datafusion-comet/pull/5201#discussion_r3705141689
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -786,7 +791,26 @@ object QueryPlanSerde extends Logging with CometExprShim
with CometTypeShim {
binding: Boolean = true): Option[Expr] = {
val newExpr = DecimalPrecision.promote(expr)
- exprToProtoInternal(newExpr, inputs, binding)
+ val result = exprToProtoInternal(newExpr, inputs, binding)
+ if (!(newExpr eq expr)) {
+ // `promote` rebuilt the tree, so the coverage tags landed on copies
that the operator does
+ // not hold. Lift them onto `expr` so
`CometExecRule.rollUpInfoMessages`, which walks the
+ // operator's own expressions, still sees them. Skipped in the common
case where `promote`
+ // returned the same tree and there is nothing to lift.
+ liftCoverageTags(newExpr, expr)
+ }
+ result
+ }
+
+ private def liftCoverageTags(from: Expression, to: Expression): Unit = {
Review Comment:
`liftCoverageTags` re-homes the `NATIVE_EXPRS` / `CODEGEN_DISPATCH_EXPRS`
tags onto `expr` when `DecimalPrecision.promote` (called at
`QueryPlanSerde.scala:788`) rebuilds the tree and the tags land on the promoted
copy instead of the tree the operator actually holds. None of the new tests in
`CometCodegenSuite.scala` exercise this path, the only coverage is incidental
to whatever the TPC-DS goldens happen to hit, and if this silently dropped tags
no test would fail, it would just undercount native expressions in any plan
with decimal arithmetic. Could you add a test with a decimal expression that
actually triggers `DecimalPrecision.promote` (for example, adding two `DECIMAL`
columns with different precision/scale in a projection), asserting
`ExtendedExplainInfo.getNativeExpressions` reports the arithmetic expression.
--
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]
