parthchandra commented on code in PR #5236:
URL: https://github.com/apache/datafusion-comet/pull/5236#discussion_r3714571500
##########
spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala:
##########
@@ -698,6 +702,20 @@ case class CometExecRule(session: SparkSession)
/** Convert a Spark plan to a Comet plan using the specified serde handler */
private def convertToComet(op: SparkPlan, handler: CometOperatorSerde[_]):
Option[SparkPlan] = {
+ val converted = tryConvertToComet(op, handler)
+ if (converted.isEmpty) {
+ // Comet looked at this operator and declined it, so it stays in the
Spark plan. Lift any
+ // reasons recorded on its expressions onto the operator itself - see
+ // `rollUpFallbackReasons` for why this is needed - and make sure
something was recorded.
+ rollUpFallbackReasons(op)
+ reportUnexplainedFallback(op)
Review Comment:
Is it correct to say that `reportUnexplainedFallback` relies on
`rollUpFallbackReasons` to have rolled up the expression tags first? Should we
add a comment to make sure a future change from separating them?
##########
spark/src/test/scala/org/apache/comet/rules/CometExecRuleSuite.scala:
##########
@@ -77,6 +77,64 @@ class CometExecRuleSuite extends CometTestBase {
}.sum
}
+ test("expression-level fallback reasons are rolled up onto the operator that
falls back") {
Review Comment:
Should we add a unit test that forces a handled operator to return None with
no reason and asserts that an exception is thrown in strict mode?
--
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]
