[SPARK-31705][SQL][FOLLOWUP] Avoid the unnecessary CNF computation for full-outer joins by maropu · Pull Request #28810 · apache/spark

maropu · 2020-06-12T08:10:36Z

What changes were proposed in this pull request?

To avoid the unnecessary CNF computation for full-outer joins, this PR fixes code for filtering out full-outer joins at the entrance of the rule.

Why are the changes needed?

To mitigate optimizer overhead.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests.

maropu · 2020-06-12T08:10:49Z

cc: @gengliangwang @cloud-fan

gengliangwang · 2020-06-12T08:17:01Z

@maropu Thanks for the improvement. Shall we update PushPredicateThroughJoin as well?

maropu · 2020-06-12T09:44:53Z

Yea, sure.

cloud-fan · 2020-06-12T09:55:14Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

-        case UsingJoin(_, _) => sys.error("Untransformed Using join node")
+
+        case jt =>
+          sys.error(s"Unexpected join type: $jt")


since we are here, can we throw an exception instead? sys.error will exit the JVM IIRC

How about IllegalStateException? Throwing an exception here looks okay to me, but I personally think we need consistent handling for unexpected code pathes. I've checed how to handle this unexpected behaivour in the other optimzier rules, then I found there are some rules to use sys.error;

$ grep -nr "sys.error" . ./Optimizer.scala:1345: sys.error(s"Unexpected join type: $jt") ./Optimizer.scala:1381: sys.error(s"Unexpected join type: $jt") ./PushCNFPredicateThroughJoin.scala:64: sys.error(s"Unexpected join type: $jt") ./subquery.scala:458: sys.error(s"Unexpected operator in scalar subquery: $lp") ./subquery.scala:496: sys.error(s"Correlated subquery has unexpected operator $op below filter") ./subquery.scala:498: case op @ _ => sys.error(s"Unexpected operator $op in correlated subquery") ./subquery.scala:502: sys.error("This line should be unreachable") ./subquery.scala:564: case op => sys.error(s"Unexpected operator $op in corelated subquery")

Is it better to change them, too?

How about AnalysisException?

AnalysisException is mainly used for an analyzing phase? https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/AnalysisException.scala#L24

yea, I know some rules in the optimizer throw AnalysisException though...

(base) maropu@~/Repositories/spark/spark-master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer: (SPARK-31705 $)$grep -nr "throw" . ./joins.scala:187: // `requires attributes from more than one child`, we throw firstly here for better ./joins.scala:189: throw new AnalysisException("Using PythonUDF in join condition of join type" + ./joins.scala:206: throw new AnalysisException("Using PythonUDF in join condition of join type" + ./NormalizeFloatingNumbers.scala:100: throw new IllegalStateException("grouping/join/window partition keys cannot be map type.") ./NormalizeFloatingNumbers.scala:134: case _ => throw new IllegalStateException(s"fail to normalize $expr") ./objects.scala:250: throw new IllegalStateException("LambdaVariable should never has 0 as its ID.") ./objects.scala:255: throw new IllegalStateException( ./objects.scala:264: throw new IllegalStateException( ./Optimizer.scala:1345: throw new IllegalStateException(s"Unexpected join type: $jt") ./Optimizer.scala:1381: throw new IllegalStateException(s"Unexpected join type: $jt") ./Optimizer.scala:1436: throw new AnalysisException( ./PushCNFPredicateThroughJoin.scala:64: throw new IllegalStateException(s"Unexpected join type: $jt") ./ReplaceExceptWithFilter.scala:106: throw new IllegalStateException("Leaf node is expected") ./ReplaceNullWithFalseInPredicate.scala:105: throw new IllegalArgumentException(message) ./subquery.scala:79: throw new AnalysisException("Found conflicting attributes " +

SparkQA · 2020-06-12T13:58:42Z

Test build #123908 has finished for PR 28810 at commit 6cad6c6.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-06-12T14:00:28Z

retest this please

SparkQA · 2020-06-12T16:29:25Z

Test build #123917 has finished for PR 28810 at commit 7850c2b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-12T16:45:23Z

Test build #123923 has finished for PR 28810 at commit 1251b68.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-12T19:30:03Z

Test build #123931 has finished for PR 28810 at commit 1251b68.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-06-15T02:44:29Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

-        case UsingJoin(_, _) => sys.error("Untransformed Using join node")
+
+        case jt =>
+          throw new IllegalStateException(s"Unexpected join type: $jt")


IllegalStateException means unexpected code path, I think this is the standard in Java, +1

maropu · 2020-06-16T01:24:40Z

retest this please

maropu · 2020-06-16T01:24:50Z

ping @cloud-fan @gengliangwang

SparkQA · 2020-06-16T05:40:27Z

Test build #124081 has finished for PR 28810 at commit 1251b68.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-06-16T06:39:26Z

retest this please

gengliangwang · 2020-06-16T07:04:51Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

-        case NaturalJoin(_) => sys.error("Untransformed NaturalJoin node")
-        case UsingJoin(_, _) => sys.error("Untransformed Using join node")
+
+        case jt =>


Nit: jt => other

gengliangwang · 2020-06-16T07:04:57Z

...yst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/PushCNFPredicateThroughJoin.scala

-          case FullOuter => j
-          case NaturalJoin(_) => sys.error("Untransformed NaturalJoin node")
-          case UsingJoin(_, _) => sys.error("Untransformed Using join node")
+          case jt =>


SparkQA · 2020-06-16T07:05:02Z

Test build #124104 has finished for PR 28810 at commit 1251b68.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

wangyum

Could we make conjunctiveNormalForm to protected in this pr?

maropu · 2020-06-16T10:40:01Z

Thanks, all! Pending, Jenkins.

SparkQA · 2020-06-16T16:03:22Z

Test build #124122 has finished for PR 28810 at commit 6dbff0c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

wangyum · 2020-06-16T16:13:37Z

Merged to master.

SparkQA · 2020-06-16T16:25:32Z

Test build #124120 has finished for PR 28810 at commit f4497a6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Fix

6cad6c6

probot-autolabeler bot added the SQL label Jun 12, 2020

review

7850c2b

cloud-fan reviewed Jun 12, 2020

View reviewed changes

review

1251b68

cloud-fan reviewed Jun 15, 2020

View reviewed changes

cloud-fan approved these changes Jun 16, 2020

View reviewed changes

gengliangwang reviewed Jun 16, 2020

View reviewed changes

gengliangwang approved these changes Jun 16, 2020

View reviewed changes

wangyum reviewed Jun 16, 2020

View reviewed changes

fix

6dbff0c

maropu force-pushed the SPARK-31705 branch from f4497a6 to 6dbff0c Compare June 16, 2020 10:39

wangyum approved these changes Jun 16, 2020

View reviewed changes

dongjoon-hyun approved these changes Jun 16, 2020

View reviewed changes

wangyum closed this in 8d57709 Jun 16, 2020

maropu mentioned this pull request Feb 1, 2021

[SPARK-32985][SQL] Decouple bucket scan and bucket filter pruning for data source v1 #31413

Closed

Conversation

maropu commented Jun 12, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

maropu commented Jun 12, 2020

Uh oh!

gengliangwang commented Jun 12, 2020

Uh oh!

maropu commented Jun 12, 2020

Uh oh!

cloud-fan Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

maropu Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

gengliangwang Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

maropu Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

maropu commented Jun 12, 2020

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

SparkQA commented Jun 12, 2020

Uh oh!

cloud-fan Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

maropu commented Jun 16, 2020

Uh oh!

maropu commented Jun 16, 2020

Uh oh!

SparkQA commented Jun 16, 2020

Uh oh!

maropu commented Jun 16, 2020

Uh oh!

gengliangwang Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

gengliangwang Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 16, 2020

Uh oh!

wangyum left a comment

Choose a reason for hiding this comment

Uh oh!

maropu commented Jun 16, 2020

Uh oh!

SparkQA commented Jun 16, 2020

Uh oh!

wangyum commented Jun 16, 2020

Uh oh!

SparkQA commented Jun 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants