comphead commented on code in PR #7095:
URL: https://github.com/apache/datafusion-comet/pull/7095#discussion_r4329264844
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -1007,110 +1010,109 @@ object QueryPlanSerde extends Logging with
CometExprShim with CometTypeShim {
def exprToProtoInternal(
expr: Expression,
inputs: Seq[Attribute],
- binding: Boolean): Option[Expr] = {
-
- def convert[T <: Expression](expr: T, handler: CometExpressionSerde[T]):
Option[Expr] = {
- val exprConfName = handler.getExprConfigName(expr)
- if (!CometConf.isExprEnabled(exprConfName)) {
- withFallbackReason(
- expr,
- "Expression support is disabled. Set " +
- s"${CometConf.getExprEnabledConfigKey(exprConfName)}=true to
enable it.")
- return None
- }
- handler.getSupportLevel(expr) match {
- case Unsupported(notes) =>
- // `CodegenDispatchFallback` serdes have no native path for these
cases either, but the
- // dispatcher can still run Spark's own `doGenCode` inside the Comet
pipeline. Try that
- // before falling the projection back to Spark. No `[COMET-INFO]`
hint here: unlike
- // `Incompatible`, there is no native opt-in for the user to flip,
so log at debug level
- // to give anyone tracing why a projection stayed native something
to go on.
- dispatchIfFallback(handler, expr, inputs, binding)
- .map { case (_, proto) =>
- logDebug(
- s"$expr has no native path (${notes.getOrElse("unsupported")})
but is routed " +
- "through the JVM codegen dispatcher to stay in the Comet
pipeline.")
- proto
- }
- .orElse {
- withFallbackReason(expr, notes.getOrElse(""))
- None
+ binding: Boolean): Option[Expr] =
+ CometExplainInfo.withExpressionFallbackReasonScope {
Review Comment:
`exprToProto` already opens or reuses the scope at line 939 before it calls
this method, and every caller of `exprToProtoInternal` that I found is a serde
handler running inside that call (`CometCast`, `CometSortOrder`,
`CometScalarFunction`, `CometNativeUdfCall`, `CometScalaUDF`,
`CometBloomFilterMightContain`). If that holds, this second wrapper does
nothing, but it costs a by-name closure and a `ThreadLocal.get` for every node
of the recursion and re-indents the whole body of this method in the diff. Is
there a direct entry into `exprToProtoInternal` outside `exprToProto` that
needs it? I did not trace every handler.
##########
spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala:
##########
@@ -336,8 +340,10 @@ object CometSparkSessionExtensions extends Logging {
logWarning(s"Comet cannot accelerate ${node.getClass.getSimpleName}
because: $reason")
}
}
- val existingNodeInfos =
+ val recorded = CometExplainInfo.recordFallbackReasons(node, info)
+ val existingNodeInfos = recorded.filter(_ =>
node.isInstanceOf[Expression]).getOrElse {
node.getTagValue(CometExplainInfo.FALLBACK_REASONS).getOrElse(Set.empty[String])
+ }
node.setTagValue(CometExplainInfo.FALLBACK_REASONS, existingNodeInfos ++
info)
Review Comment:
`recordFallbackReasons` runs for every `TreeNode`, but the only journal
readers I found (`rollUpFallbackReasons`, `liftFallbackReasons`) look up
expressions, so the entries written for operators are never read and the
`filter(_ => node.isInstanceOf[Expression])` throws the returned set away for
them. For expressions, `existingNodeInfos ++ info` is also redundant because
`recorded` already contains `info`. Could the journal and its two accessors
take `Expression` only, with a match here that reads the journal set for an
`Expression` (or the tag when no scope is active) and the tag for anything
else? The invariant would then show in the types, and `recordFallbackReasons`
would no longer conflate "no active scope" with "recorded".
--
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]
