sunchao commented on code in PR #5471:
URL: https://github.com/apache/datafusion-comet/pull/5471#discussion_r3873879881
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -855,14 +855,10 @@ object QueryPlanSerde extends Logging with CometExprShim
with CometTypeShim {
}
private def liftCoverageTags(from: Expression, to: Expression): Unit = {
Review Comment:
Updated in 95c744c4c63af1d8a778245bd34b080d12c0904b. Added reciprocal
comments on `isStructuralExpr` and `isNeverTagged`: the read filter is the
write-side structural set minus `Alias`, because rewritten-child coverage is
lifted onto the original alias. I also narrowed the wording to coverage/info
tags and explicitly excluded `FALLBACK_REASONS` from that invariant; literals
can legitimately carry those reasons.
##########
spark/src/test/scala/org/apache/comet/CometCodegenSuite.scala:
##########
@@ -278,7 +279,20 @@ class CometCodegenSuite
val planted = Literal.TrueLiteral
planted.setTagValue(CometExplainInfo.EXTENSION_INFO, Set("PLANTED_INFO"))
planted.setTagValue(CometExplainInfo.NATIVE_EXPRS, Set("plantedexpr"))
+ planted.setTagValue(CometExplainInfo.CODEGEN_DISPATCH_EXPRS,
Set("planteddispatch"))
try {
+ // Decimal promotion rebuilds this projection. Its coverage lift must
not copy the
Review Comment:
Updated in 95c744c4c63af1d8a778245bd34b080d12c0904b. I kept the
seeded-singleton probe with its plan-level check so the two phases share one
setup and guaranteed cleanup. The direct call pins the exact
`Literal.TrueLiteral` identity and the original `Alias`; normal SQL
optimization can change that shape. The neighboring decimal test already covers
plan-level coverage, and the second half of this test covers DPP roll-up and
extended explain. There are also existing direct Catalyst probes in this suite
for `BoundReference` and map/array expressions.
##########
spark/src/test/scala/org/apache/comet/CometCodegenSuite.scala:
##########
@@ -278,7 +279,20 @@ class CometCodegenSuite
val planted = Literal.TrueLiteral
planted.setTagValue(CometExplainInfo.EXTENSION_INFO, Set("PLANTED_INFO"))
planted.setTagValue(CometExplainInfo.NATIVE_EXPRS, Set("plantedexpr"))
+ planted.setTagValue(CometExplainInfo.CODEGEN_DISPATCH_EXPRS,
Set("planteddispatch"))
try {
+ // Decimal promotion rebuilds this projection. Its coverage lift must
not copy the
+ // singleton's stale tags onto the Alias, which is a legitimate coverage
owner.
+ val decimal = AttributeReference("amount", DecimalType(10, 2), nullable
= false)()
+ val projection = Alias(
+ CreateNamedStruct(Seq(Literal("flag"), planted, Literal("sum"),
Add(decimal, decimal))),
+ "value")()
+ assert(QueryPlanSerde.exprToProto(projection, Seq(decimal)).isDefined)
+ val native =
projection.getTagValue(CometExplainInfo.NATIVE_EXPRS).getOrElse(Set.empty)
+ assert(native.contains("checkoverflow"), s"expected lifted decimal
coverage, got: $native")
+ assert(!native.contains("plantedexpr"))
+
assert(projection.getTagValue(CometExplainInfo.CODEGEN_DISPATCH_EXPRS).isEmpty)
Review Comment:
Updated in 95c744c4c63af1d8a778245bd34b080d12c0904b. Added `codegen dispatch
coverage survives the decimal promotion rewrite`. It places `Hypot` above a
cast of decimal addition, so promotion rebuilds both `Hypot` and its alias. The
test checks the emitted `JvmScalarUdf` dispatcher class, verifies that the
original `Hypot` has neither the dispatch marker nor its coverage tag, and
requires the original alias to receive `hypot`. This passed locally on Spark
4.1.3. As a mutation check, removing only `liftCoverageTags(newExpr, expr)`
made this new assertion fail with `None did not contain Set("hypot")`.
--
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]
