feat: Support sort merge join #178

viirya · 2024-03-09T08:19:52Z

Which issue does this PR close?

Closes #177.

Rationale for this change

What changes are included in this PR?

How are these changes tested?

codecov-commenter · 2024-03-09T17:29:59Z

Codecov Report

Attention: Patch coverage is 69.73684% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 33.40%. Comparing base (81a641f) to head (637ff19).
Report is 4 commits behind head on main.

Files	Patch %	Lines
.../scala/org/apache/comet/serde/QueryPlanSerde.scala	66.66%	4 Missing and 4 partials ⚠️
...n/scala/org/apache/spark/sql/comet/operators.scala	66.66%	0 Missing and 8 partials ⚠️
...org/apache/comet/CometSparkSessionExtensions.scala	84.00%	0 Missing and 4 partials ⚠️
...java/org/apache/comet/vector/CometPlainVector.java	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #178      +/-   ##
============================================
+ Coverage     33.32%   33.40%   +0.08%     
+ Complexity      769      768       -1     
============================================
  Files           107      107              
  Lines         35395    36331     +936     
  Branches       7669     7936     +267     
============================================
+ Hits          11795    12137     +342     
- Misses        21146    21645     +499     
- Partials       2454     2549      +95

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

viirya · 2024-03-09T17:38:29Z

cc @sunchao @kazuyukitanimura

viirya · 2024-03-09T17:39:49Z

spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala

+    val operatorDisabledFlag = s"$COMET_EXEC_CONFIG_PREFIX.$operator.disabled"
+    conf.getConfString(operatorFlag, "false").toBoolean || isCometAllOperatorEnabled(conf) &&
+    !conf.getConfString(operatorDisabledFlag, "false").toBoolean


This "disable" flag is useful to disable a particular operator in unit test. For example, I disable sort merge join in one existing test below.

viirya · 2024-03-09T17:40:15Z

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

+        // TODO: Support SortMergeJoin with join condition after new DataFusion release
+        if (join.condition.isDefined) {
+          return None
+        }


I've not added join filter support in this PR. I will do it in follow up.

viirya · 2024-03-09T17:44:35Z

spark/src/test/scala/org/apache/comet/exec/CometExecSuite.scala

@@ -859,6 +906,7 @@ class CometExecSuite extends CometTestBase {
        .saveAsTable("bucketed_table2")

      withSQLConf(
+        "spark.comet.exec.sort_merge_join.disabled" -> "true",


This test explicitly checks for Spark sort merge join. I'd like to keep what it proposes to test so disable Comet sort merge join here.

common/src/main/java/org/apache/comet/vector/CometPlainVector.java

viirya · 2024-03-12T22:33:30Z

I will update the diff for failed Spark tests.

core/src/execution/datafusion/planner.rs

spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala

kazuyukitanimura · 2024-03-13T07:27:19Z

spark/src/test/scala/org/apache/comet/exec/CometExecSuite.scala

+      withParquetTable((0 until 10).map(i => (i, i % 5)), "tbl_a") {
+        withParquetTable((0 until 10).map(i => (i % 10, i + 2)), "tbl_b") {
+          val df1 = sql("SELECT * FROM tbl_a JOIN tbl_b ON tbl_a._2 = tbl_b._1")
+          checkSparkAnswerAndOperator(df1)


Should we add checks to make sure the plan includes the Comet SMJ, i.e. stripAQEPlan(df.queryExecution.executedPlan).collectFirst...

checkSparkAnswerAndOperator already does the check. If there is Spark SMJ or other join operators, it will report error.

I see. How does checkSparkAnswerAndOperator checks whether it is Comet SMJ?
Unless we provide includeClasses, it only checks classes below?
https://github.com/apache/arrow-datafusion-comet/blob/main/spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala#L156

case _: CometScanExec | _: CometBatchScanExec => true case _: CometSinkPlaceHolder | _: CometScanWrapper => false case _: CometExec | _: CometShuffleExchangeExec => true case _: CometBroadcastExchangeExec => true case _: WholeStageCodegenExec | _: ColumnarToRowExec | _: InputAdapter => true

CometSortMergeJoinExec along with other native operators are CometExec so it is listed in checkCometOperators. We don't white list all native operators but the common base class.

Got it, thanks. I think it is good for now. Ideal if we can check CometSortMergeJoinExec specifically, as we will add other joins. In that way, we can make sure that we are not testing different join accidentally.

Ah, that makes sense. I can add something to verify join type (sort merge join or hash join or others) in a follow up. Thanks for the suggestion.

kazuyukitanimura

LGTM

kazuyukitanimura · 2024-03-14T16:35:27Z

spark/src/test/scala/org/apache/comet/exec/CometExecSuite.scala

+      withParquetTable((0 until 10).map(i => (i, i % 5)), "tbl_a") {
+        withParquetTable((0 until 10).map(i => (i % 10, i + 2)), "tbl_b") {
+          val df1 = sql("SELECT * FROM tbl_a JOIN tbl_b ON tbl_a._2 = tbl_b._1")
+          checkSparkAnswerAndOperator(df1)


Got it, thanks. I think it is good for now. Ideal if we can check CometSortMergeJoinExec specifically, as we will add other joins. In that way, we can make sure that we are not testing different join accidentally.

common/src/main/java/org/apache/comet/vector/CometPlainVector.java

core/src/execution/datafusion/planner.rs

dev/diffs/3.4.2.diff

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

spark/src/test/scala/org/apache/comet/exec/CometExecSuite.scala

sunchao

LGTM (pending CI)

viirya · 2024-03-18T21:22:26Z

Merged. Thanks.

viirya force-pushed the sort_merge_join branch from 70fc8c7 to 0b7f600 Compare March 9, 2024 08:56

viirya commented Mar 9, 2024

View reviewed changes

common/src/main/java/org/apache/comet/vector/CometPlainVector.java Outdated Show resolved Hide resolved

viirya mentioned this pull request Mar 10, 2024

Seeing WARN messages indicating native execution is disabled #180

Closed

singhpk234 mentioned this pull request Mar 12, 2024

Support BroadcastNestedLoopJoinExec #198

Open

viirya force-pushed the sort_merge_join branch from c24429b to 8f6ee36 Compare March 12, 2024 20:50

kazuyukitanimura reviewed Mar 13, 2024

View reviewed changes

viirya force-pushed the sort_merge_join branch from fd66320 to 17dd39f Compare March 14, 2024 06:14

kazuyukitanimura approved these changes Mar 14, 2024

View reviewed changes

sunchao reviewed Mar 15, 2024

View reviewed changes

viirya added 8 commits March 18, 2024 11:21

feat: Support sort merge join

9b4b87d

Update PlanStability

04d928e

Update Spark diff

4b78e4f

For review

15eb659

Fix format

68418ac

For review

08db759

Add CometJoinSuite

80e58ea

Remove uuid

fc03003

viirya force-pushed the sort_merge_join branch from 637ff19 to fc03003 Compare March 18, 2024 18:22

sunchao approved these changes Mar 18, 2024

View reviewed changes

Fix diff

1e5465f

viirya merged commit 8aab44c into apache:main Mar 18, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support sort merge join #178

feat: Support sort merge join #178

viirya commented Mar 9, 2024

codecov-commenter commented Mar 9, 2024 •

edited

Loading

viirya commented Mar 9, 2024

viirya Mar 9, 2024

viirya Mar 9, 2024

viirya Mar 9, 2024

viirya commented Mar 12, 2024

kazuyukitanimura Mar 13, 2024

viirya Mar 14, 2024

kazuyukitanimura Mar 14, 2024

viirya Mar 14, 2024

kazuyukitanimura Mar 14, 2024

viirya Mar 14, 2024

kazuyukitanimura left a comment

kazuyukitanimura Mar 14, 2024

sunchao left a comment

viirya commented Mar 18, 2024

feat: Support sort merge join #178

feat: Support sort merge join #178

Conversation

viirya commented Mar 9, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

codecov-commenter commented Mar 9, 2024 • edited Loading

Codecov Report

viirya commented Mar 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Mar 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazuyukitanimura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunchao left a comment

Choose a reason for hiding this comment

viirya commented Mar 18, 2024

codecov-commenter commented Mar 9, 2024 •

edited

Loading