andygrove commented on code in PR #5471:
URL: https://github.com/apache/datafusion-comet/pull/5471#discussion_r3873068651
##########
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:
What makes this filter safe is that `isStructuralExpr` at line 1002 never
lets the serde tag an `Attribute`, `BoundReference`, or `Literal` in the first
place, so `CometExplainInfo.isNeverTagged` can only ever discard copied tags.
That invariant is now load-bearing for this fix, but the two lists live in
different files under different names and neither comment mentions the other.
Could `isNeverTagged` be derived from `isStructuralExpr` minus `Alias`, or
failing that, could each comment name the other and state the subset
relationship? Otherwise someone adding a node type to just one of them either
reopens this path or quietly deletes real coverage.
##########
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:
Would it be better to split this out into its own test? It asserts something
quite different from the rest of the test it is in, which is about the
dynamic-pruning plan not reporting the planted name, and the test name and
comment only describe that second half. A separate test named for the lift,
something like "the coverage lift ignores stale tags on the shared
TrueLiteral", would point straight at the mechanism when it fails, and it would
sit next to "expression coverage stats survive the decimal promotion rewrite",
which is the positive case for the same code path.
Related question. Is there a way to drive this through a real plan the way
the neighbouring tests do, or does planting on the singleton force the direct
`exprToProto` call? I ask because this is the only place in these suites that
builds a Catalyst tree by hand, so if a plan-level version is workable it would
be more consistent and would cover the roll-up and the explain rendering too.
##########
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:
This change routes `CODEGEN_DISPATCH_EXPRS` through the structural filter
for the first time, and both of the new assertions for it are negative. Would
it be worth adding a positive case, where an expression inside a promoted
decimal tree really is routed through the JVM codegen dispatcher, checking that
its name still reaches the original owner? Without one, a future change to
`isNeverTagged` could drop genuine dispatch coverage across a decimal rewrite
and these tests would still pass.
--
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]
