feat: Support CartesianProductExec in comet #442
Conversation
|
@viirya help to start CI and review, thanks |
spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/spark/sql/comet/operators.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala
Outdated
Show resolved
Hide resolved
|
@viirya thanks for your review, resolve your commets |
spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala
Outdated
Show resolved
Hide resolved
| override def hashCode(): Int = Objects.hashCode(condition, left, right) | ||
|
|
||
| override lazy val metrics: Map[String, SQLMetric] = | ||
| CometMetricNode.baselineMetrics(sparkContext) |
There was a problem hiding this comment.
Could you add more metrics here since Datafusion's cross join could emit additional metrics other than output rows?
|
@leoluan2009 You may need to generate plan stability results to pass the CI pipelines. |
| @@ -251,4 +251,18 @@ class CometJoinSuite extends CometTestBase { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| // TODO: Add a test for CartesianProductExec with join filter after new DataFusion release | |||
There was a problem hiding this comment.
Add a github issue for the followup and refer to it here?
OK, I will update soon, thanks! |
|
@leoluan2009 Would you like to update the plan stability results? Thanks. |
Hi @viirya sorry for delay reply, I think this pr do not change the query plan for tpcds? |
Hmm, last time I saw the plans were changed so CI was failed, but now the CI doesn't fail on plan stability. But CI currently failed on some other tests. |
Which issue does this PR close?
Closes #199 .
TODO: need to support join condition after CrossJoin in Datafusion support join condition
Rationale for this change
What changes are included in this PR?
How are these changes tested?