sunchao commented on code in PR #7357:
URL: https://github.com/apache/datafusion-comet/pull/7357#discussion_r4433059470
##########
dev/diffs/3.5.9.diff:
##########
@@ -1706,20 +1706,51 @@ index 418ca3430bb..eb8267192f8 100644
withTempPath { path =>
val dir = path.getCanonicalPath
diff --git
a/sql/core/src/test/scala/org/apache/spark/sql/execution/LogicalPlanTagInSparkPlanSuite.scala
b/sql/core/src/test/scala/org/apache/spark/sql/execution/LogicalPlanTagInSparkPlanSuite.scala
-index 743ec41dbe7..9f30d6c8e04 100644
+index 743ec41dbe7..aa549210fae 100644
---
a/sql/core/src/test/scala/org/apache/spark/sql/execution/LogicalPlanTagInSparkPlanSuite.scala
+++
b/sql/core/src/test/scala/org/apache/spark/sql/execution/LogicalPlanTagInSparkPlanSuite.scala
-@@ -53,6 +53,10 @@ class LogicalPlanTagInSparkPlanSuite extends
TPCDSQuerySuite with DisableAdaptiv
+@@ -22,6 +22,7 @@ import scala.reflect.ClassTag
+ import org.apache.spark.sql.TPCDSQuerySuite
+ import
org.apache.spark.sql.catalyst.expressions.aggregate.{AggregateExpression,
Complete, Final}
+ import org.apache.spark.sql.catalyst.plans.logical.{Aggregate, Generate,
Join, LocalRelation, LogicalPlan, Range, Sample, Union, Window, WithCTE}
++import org.apache.spark.sql.comet.{CometColumnarToRowExec, CometFilterExec,
CometProjectExec}
+ import org.apache.spark.sql.execution.adaptive.DisableAdaptiveExecutionSuite
+ import org.apache.spark.sql.execution.aggregate.{HashAggregateExec,
ObjectHashAggregateExec, SortAggregateExec}
+ import org.apache.spark.sql.execution.columnar.{InMemoryRelation,
InMemoryTableScanExec}
+@@ -47,12 +48,19 @@ class LogicalPlanTagInSparkPlanSuite extends
TPCDSQuerySuite with DisableAdaptiv
+ }
+
+ // A scan plan tree is a plan tree that has a leaf node under zero or more
Project/Filter nodes.
+- // We may add `ColumnarToRowExec` and `InputAdapter` above the scan node
after planning.
++ // A columnar-to-row transition and `InputAdapter` may be added above the
scan after planning.
+ @scala.annotation.tailrec
+ private def isScanPlanTree(plan: SparkPlan): Boolean = plan match {
case ColumnarToRowExec(i: InputAdapter) => isScanPlanTree(i.child)
++ case CometColumnarToRowExec(i: InputAdapter) => isScanPlanTree(i.child)
Review Comment:
I tried removing both cases. It works when falling through reaches the same
project, but q9 also has a Spark project above a Comet row transition and a
native filter. Without looking through the transition, the matcher walks past
that project and asks the filter for a direct tag. The filter correctly has
only an inherited tag. This reproduced on Spark 3.5.9, 4.0.4, and 4.1.3.
I also tried keeping just the scan-tree check. A bare filtered Parquet scan
exposed the other case: the transition has no direct tag, and its child has the
direct `Filter` link. The assertion needs to look through the transition there
too.
The five patches now use Spark’s `ColumnarToRowTransition` type in the two
existing matches. That covers Spark and Comet without adding either
Comet-specific transition case, and keeps both original Spark comments. The
revised matcher passes both q9 and the bare-scan check.
[b39bd7e331](https://github.com/apache/datafusion-comet/pull/7357/commits/b39bd7e3318fdf8b532585b7ff9863799bc29a8e)
--
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]
