kazuyukitanimura commented on code in PR #6879:
URL: https://github.com/apache/datafusion-comet/pull/6879#discussion_r4276542679
##########
spark/src/main/scala/org/apache/spark/sql/comet/CometNativeScanExec.scala:
##########
@@ -75,6 +75,15 @@ case class CometNativeScanExec(
with ShimStreamSourceAwareSparkPlan
with CometScanWithPlanData {
+ override def sparkFallback(newChildren: Seq[SparkPlan]): SparkPlan = {
Review Comment:
Do any of the new tests cover a reverted stage where AQE has rewritten the
DPP placeholders on the live scan? I couldn't find one in the test diff. If the
main-branch PR had such a test, could it be included here?
##########
docs/source/contributor-guide/adding_a_new_operator.md:
##########
@@ -573,6 +573,34 @@ Use `QueryPlanSerde.exprToProto` to convert Spark
expressions to protobuf:
val protoExpr = exprToProto(sparkExpr, inputSchema)
```
+### Restoring the Spark operator (`sparkFallback`)
+
+`CometExec.originalPlan` is the Spark operator this node replaced.
`CometExecRule` copies
+`originalPlan.logicalLink` onto the Comet node, which is how AQE finds the
node again when it
+re-plans a stage. `RevertNativeForTransitionHeavyStages` calls
`sparkFallback(newChildren)` to
+rebuild that Spark operator with the children of the reverted stage.
+
+The default implementation is `originalPlan.withNewChildren(newChildren)`. It
refuses a null
+`originalPlan`, an `originalPlan` that is one of the node's own children, or a
different number of
+children than the Spark operator has.
+
+Override `sparkFallback` when conversion changes the plan shape, so the
restored node is not that
+Spark operator with the same children. `CometNativeWriteExec` replaces a
`DataWritingCommandExec`
+and drops the `WriteFilesExec` under it; its override puts that wrapper back
around the restored
+input. `CometIcebergWriteExec` keeps the same shape as `IcebergWriteExec`, so
the default is
+enough.
+
+Also override `sparkFallback` when the operator's live state differs from
`originalPlan`.
+`CometNativeScanExec` restores its current partition and data filters, and
+`CometIcebergNativeScanExec` restores its current runtime filters, so AQE's
executable DPP
+subqueries survive reversion. `CometLocalTopKExec` returns the restored child
directly: Comet
Review Comment:
Thanks for adapting the docs along with the code. This sentence mentions
CometLocalTopKExec, but that operator isn't on branch-1.1 (the PR description
says the same). Could you drop it or reword it so the guide only describes
operators in this release?
--
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]
