[SPARK-19766][SQL] Constant alias columns in INNER JOIN should not be folded by FoldablePropagation rule #17099

stanzhai · 2017-02-28T13:15:12Z

What changes were proposed in this pull request?

This PR fixes the code in Optimizer phase where the constant alias columns of a INNER JOIN query are folded in Rule FoldablePropagation.

For the following query():

val sqlA =
  """
    |create temporary view ta as
    |select a, 'a' as tag from t1 union all
    |select a, 'b' as tag from t2
  """.stripMargin

val sqlB =
  """
    |create temporary view tb as
    |select a, 'a' as tag from t3 union all
    |select a, 'b' as tag from t4
  """.stripMargin

val sql =
  """
    |select tb.* from ta inner join tb on
    |ta.a = tb.a and
    |ta.tag = tb.tag
  """.stripMargin

The tag column is an constant alias column, it's folded by FoldablePropagation like this:

TRACE SparkOptimizer: 
=== Applying Rule org.apache.spark.sql.catalyst.optimizer.FoldablePropagation ===
 Project [a#4, tag#14]                              Project [a#4, tag#14]
!+- Join Inner, ((a#0 = a#4) && (tag#8 = tag#14))   +- Join Inner, ((a#0 = a#4) && (a = a))
    :- Union                                           :- Union
    :  :- Project [a#0, a AS tag#8]                    :  :- Project [a#0, a AS tag#8]
    :  :  +- LocalRelation [a#0]                       :  :  +- LocalRelation [a#0]
    :  +- Project [a#2, b AS tag#9]                    :  +- Project [a#2, b AS tag#9]
    :     +- LocalRelation [a#2]                       :     +- LocalRelation [a#2]
    +- Union                                           +- Union
       :- Project [a#4, a AS tag#14]                      :- Project [a#4, a AS tag#14]
       :  +- LocalRelation [a#4]                          :  +- LocalRelation [a#4]
       +- Project [a#6, b AS tag#15]                      +- Project [a#6, b AS tag#15]
          +- LocalRelation [a#6]                             +- LocalRelation [a#6]

Finally the Result of Batch Operator Optimizations is:

Project [a#4, tag#14]                              Project [a#4, tag#14]
!+- Join Inner, ((a#0 = a#4) && (tag#8 = tag#14))   +- Join Inner, (a#0 = a#4)
!   :- SubqueryAlias ta, `ta`                          :- Union
!   :  +- Union                                        :  :- LocalRelation [a#0]
!   :     :- Project [a#0, a AS tag#8]                 :  +- LocalRelation [a#2]
!   :     :  +- SubqueryAlias t1, `t1`                 +- Union
!   :     :     +- Project [a#0]                          :- LocalRelation [a#4, tag#14]
!   :     :        +- SubqueryAlias grouping              +- LocalRelation [a#6, tag#15]
!   :     :           +- LocalRelation [a#0]        
!   :     +- Project [a#2, b AS tag#9]              
!   :        +- SubqueryAlias t2, `t2`              
!   :           +- Project [a#2]                    
!   :              +- SubqueryAlias grouping        
!   :                 +- LocalRelation [a#2]        
!   +- SubqueryAlias tb, `tb`                       
!      +- Union                                     
!         :- Project [a#4, a AS tag#14]             
!         :  +- SubqueryAlias t3, `t3`              
!         :     +- Project [a#4]                    
!         :        +- SubqueryAlias grouping        
!         :           +- LocalRelation [a#4]        
!         +- Project [a#6, b AS tag#15]             
!            +- SubqueryAlias t4, `t4`              
!               +- Project [a#6]                    
!                  +- SubqueryAlias grouping        
!                     +- LocalRelation [a#6]

The condition tag#8 = tag#14 of INNER JOIN has been removed. This leads to the data of inner join being wrong.

After fix:

=== Result of Batch LocalRelation ===
 GlobalLimit 21                                           GlobalLimit 21
 +- LocalLimit 21                                         +- LocalLimit 21
    +- Project [a#4, tag#11]                                 +- Project [a#4, tag#11]
       +- Join Inner, ((a#0 = a#4) && (tag#8 = tag#11))         +- Join Inner, ((a#0 = a#4) && (tag#8 = tag#11))
!         :- SubqueryAlias ta                                      :- Union
!         :  +- Union                                              :  :- LocalRelation [a#0, tag#8]
!         :     :- Project [a#0, a AS tag#8]                       :  +- LocalRelation [a#2, tag#9]
!         :     :  +- SubqueryAlias t1                             +- Union
!         :     :     +- Project [a#0]                                :- LocalRelation [a#4, tag#11]
!         :     :        +- SubqueryAlias grouping                    +- LocalRelation [a#6, tag#12]
!         :     :           +- LocalRelation [a#0]        
!         :     +- Project [a#2, b AS tag#9]              
!         :        +- SubqueryAlias t2                    
!         :           +- Project [a#2]                    
!         :              +- SubqueryAlias grouping        
!         :                 +- LocalRelation [a#2]        
!         +- SubqueryAlias tb                             
!            +- Union                                     
!               :- Project [a#4, a AS tag#11]             
!               :  +- SubqueryAlias t3                    
!               :     +- Project [a#4]                    
!               :        +- SubqueryAlias grouping        
!               :           +- LocalRelation [a#4]        
!               +- Project [a#6, b AS tag#12]             
!                  +- SubqueryAlias t4                    
!                     +- Project [a#6]                    
!                        +- SubqueryAlias grouping        
!                           +- LocalRelation [a#6]

How was this patch tested?

add sql-tests/inputs/inner-join.sql
All tests passed.

stanzhai · 2017-02-28T13:21:53Z

@hvanhovell

hvanhovell · 2017-02-28T13:32:49Z

ok to test

hvanhovell · 2017-02-28T13:57:53Z

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

-        // join is not always picked from its children, but can also be null.
-        // TODO(cloud-fan): It seems more reasonable to use new attributes as the output attributes
-        // of outer join.
-        case j @ Join(_, _, Inner, _) =>


We forgot to check stop here. Can you just change this line into:

case j @ Join(_, _, Inner, _) if !stop =>

SparkQA · 2017-02-28T15:01:33Z

Test build #73588 has finished for PR 17099 at commit 4463648.

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

…er-join

SparkQA · 2017-02-28T16:40:32Z

Test build #73592 has finished for PR 17099 at commit fc819f8.

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

rxin · 2017-02-28T16:56:15Z

sql/core/src/test/resources/sql-tests/inputs/inner-join.sql

+SELECT tb.* FROM ta INNER JOIN tb ON ta.a = tb.a AND ta.tag = tb.tag;
+
+-- Clean up
+DROP VIEW IF EXISTS t1;


i don't think you need these drop views, since TEMPORARY VIEW are destroyed immediately after this file.

SparkQA · 2017-02-28T17:44:59Z

Test build #73594 has finished for PR 17099 at commit c197b20.

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

gatorsmile · 2017-02-28T18:05:01Z

Could you add a test case to FoldablePropagationSuite? Thanks!

gatorsmile · 2017-03-01T04:58:57Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/FoldablePropagationSuite.scala

+      .union(testRelation.select('a, Literal("b").as('tag)))
+      .subquery('tb)
+    val query = ta.join(tb, Inner,
+      Some("ta.a".attr === "tb.a".attr && "ta.tag".attr === "tb.tag"))


This is wrong. What you are doing is to compare the column ta.tag with a string constant "tb.tag"

-> Some("ta.a".attr === "tb.a".attr && "ta.tag".attr === "tb.tag".attr))

Then add the rule ConstantFolding into the test suite.

SparkQA · 2017-03-01T05:36:26Z

Test build #73659 has finished for PR 17099 at commit 15fae50.

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

…er-join

stanzhai · 2017-03-01T06:15:06Z

Thanks for @gatorsmile 's help.

ConstantFolding will affect other test cases in FoldablePropagationSuite.

It's fine without adding ConstantFolding.

Before fix:

[info]   !'Join Inner, ((a#0 = a#0) && (1 = 1))        'Join Inner, (('tb.a = 'ta.a) && ('tb.tag = 'ta.tag))
[info]   !:- Union                                     :- 'SubqueryAlias ta
[info]   !:  :- Project [a#0, 1 AS tag#0]              :  +- 'Union
[info]   !:  :  +- LocalRelation <empty>, [a#0, b#0]   :     :- 'Project ['a, 1 AS tag#0]
[info]   !:  +- Project [a#0, 2 AS tag#0]              :     :  +- LocalRelation <empty>, [a#0, b#0]
[info]   !:     +- LocalRelation <empty>, [a#0, b#0]   :     +- 'Project ['a, 2 AS tag#0]
[info]   !+- Union                                     :        +- LocalRelation <empty>, [a#0, b#0]
[info]   !   :- Project [a#0, 1 AS tag#0]              +- 'SubqueryAlias tb
[info]   !   :  +- LocalRelation <empty>, [a#0, b#0]      +- 'Union
[info]   !   +- Project [a#0, 2 AS tag#0]                    :- 'Project ['a, 1 AS tag#0]
[info]   !      +- LocalRelation <empty>, [a#0, b#0]         :  +- LocalRelation <empty>, [a#0, b#0]
[info]   !                                                   +- 'Project ['a, 2 AS tag#0]
[info]   !                                                      +- LocalRelation <empty>, [a#0, b#0] (PlanTest.scala:99)

After fix:

[info]   !'Join Inner, ((a#0 = a#0) && (tag#0 = tag#0))   'Join Inner, (('tb.a = 'ta.a) && ('tb.tag = 'ta.tag))
[info]   !:- Union                                        :- 'SubqueryAlias ta
[info]   !:  :- Project [a#0, 1 AS tag#0]                 :  +- 'Union
[info]   !:  :  +- LocalRelation <empty>, [a#0, b#0]      :     :- 'Project ['a, 1 AS tag#0]
[info]   !:  +- Project [a#0, 2 AS tag#0]                 :     :  +- LocalRelation <empty>, [a#0, b#0]
[info]   !:     +- LocalRelation <empty>, [a#0, b#0]      :     +- 'Project ['a, 2 AS tag#0]
[info]   !+- Union                                        :        +- LocalRelation <empty>, [a#0, b#0]
[info]   !   :- Project [a#0, 1 AS tag#0]                 +- 'SubqueryAlias tb
[info]   !   :  +- LocalRelation <empty>, [a#0, b#0]         +- 'Union
[info]   !   +- Project [a#0, 2 AS tag#0]                       :- 'Project ['a, 1 AS tag#0]
[info]   !      +- LocalRelation <empty>, [a#0, b#0]            :  +- LocalRelation <empty>, [a#0, b#0]
[info]   !                                                      +- 'Project ['a, 2 AS tag#0]
[info]   !                                                         +- LocalRelation <empty>, [a#0, b#0] (PlanTest.scala:99)

I just fix the test case("tb.tag" -> "tb.tag".attr).

gatorsmile · 2017-03-01T06:21:26Z

LGTM

SparkQA · 2017-03-01T08:20:48Z

Test build #73669 has finished for PR 17099 at commit df11cc4.

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

… folded by FoldablePropagation rule ## What changes were proposed in this pull request? This PR fixes the code in Optimizer phase where the constant alias columns of a `INNER JOIN` query are folded in Rule `FoldablePropagation`. For the following query(): ``` val sqlA = """ |create temporary view ta as |select a, 'a' as tag from t1 union all |select a, 'b' as tag from t2 """.stripMargin val sqlB = """ |create temporary view tb as |select a, 'a' as tag from t3 union all |select a, 'b' as tag from t4 """.stripMargin val sql = """ |select tb.* from ta inner join tb on |ta.a = tb.a and |ta.tag = tb.tag """.stripMargin ``` The tag column is an constant alias column, it's folded by `FoldablePropagation` like this: ``` TRACE SparkOptimizer: === Applying Rule org.apache.spark.sql.catalyst.optimizer.FoldablePropagation === Project [a#4, tag#14] Project [a#4, tag#14] !+- Join Inner, ((a#0 = a#4) && (tag#8 = tag#14)) +- Join Inner, ((a#0 = a#4) && (a = a)) :- Union :- Union : :- Project [a#0, a AS tag#8] : :- Project [a#0, a AS tag#8] : : +- LocalRelation [a#0] : : +- LocalRelation [a#0] : +- Project [a#2, b AS tag#9] : +- Project [a#2, b AS tag#9] : +- LocalRelation [a#2] : +- LocalRelation [a#2] +- Union +- Union :- Project [a#4, a AS tag#14] :- Project [a#4, a AS tag#14] : +- LocalRelation [a#4] : +- LocalRelation [a#4] +- Project [a#6, b AS tag#15] +- Project [a#6, b AS tag#15] +- LocalRelation [a#6] +- LocalRelation [a#6] ``` Finally the Result of Batch Operator Optimizations is: ``` Project [a#4, tag#14] Project [a#4, tag#14] !+- Join Inner, ((a#0 = a#4) && (tag#8 = tag#14)) +- Join Inner, (a#0 = a#4) ! :- SubqueryAlias ta, `ta` :- Union ! : +- Union : :- LocalRelation [a#0] ! : :- Project [a#0, a AS tag#8] : +- LocalRelation [a#2] ! : : +- SubqueryAlias t1, `t1` +- Union ! : : +- Project [a#0] :- LocalRelation [a#4, tag#14] ! : : +- SubqueryAlias grouping +- LocalRelation [a#6, tag#15] ! : : +- LocalRelation [a#0] ! : +- Project [a#2, b AS tag#9] ! : +- SubqueryAlias t2, `t2` ! : +- Project [a#2] ! : +- SubqueryAlias grouping ! : +- LocalRelation [a#2] ! +- SubqueryAlias tb, `tb` ! +- Union ! :- Project [a#4, a AS tag#14] ! : +- SubqueryAlias t3, `t3` ! : +- Project [a#4] ! : +- SubqueryAlias grouping ! : +- LocalRelation [a#4] ! +- Project [a#6, b AS tag#15] ! +- SubqueryAlias t4, `t4` ! +- Project [a#6] ! +- SubqueryAlias grouping ! +- LocalRelation [a#6] ``` The condition `tag#8 = tag#14` of INNER JOIN has been removed. This leads to the data of inner join being wrong. After fix: ``` === Result of Batch LocalRelation === GlobalLimit 21 GlobalLimit 21 +- LocalLimit 21 +- LocalLimit 21 +- Project [a#4, tag#11] +- Project [a#4, tag#11] +- Join Inner, ((a#0 = a#4) && (tag#8 = tag#11)) +- Join Inner, ((a#0 = a#4) && (tag#8 = tag#11)) ! :- SubqueryAlias ta :- Union ! : +- Union : :- LocalRelation [a#0, tag#8] ! : :- Project [a#0, a AS tag#8] : +- LocalRelation [a#2, tag#9] ! : : +- SubqueryAlias t1 +- Union ! : : +- Project [a#0] :- LocalRelation [a#4, tag#11] ! : : +- SubqueryAlias grouping +- LocalRelation [a#6, tag#12] ! : : +- LocalRelation [a#0] ! : +- Project [a#2, b AS tag#9] ! : +- SubqueryAlias t2 ! : +- Project [a#2] ! : +- SubqueryAlias grouping ! : +- LocalRelation [a#2] ! +- SubqueryAlias tb ! +- Union ! :- Project [a#4, a AS tag#11] ! : +- SubqueryAlias t3 ! : +- Project [a#4] ! : +- SubqueryAlias grouping ! : +- LocalRelation [a#4] ! +- Project [a#6, b AS tag#12] ! +- SubqueryAlias t4 ! +- Project [a#6] ! +- SubqueryAlias grouping ! +- LocalRelation [a#6] ``` ## How was this patch tested? add sql-tests/inputs/inner-join.sql All tests passed. Author: Stan Zhai <zhaishidan@haizhi.com> Closes #17099 from stanzhai/fix-inner-join. (cherry picked from commit 5502a9c) Signed-off-by: Xiao Li <gatorsmile@gmail.com>

gatorsmile · 2017-03-01T15:55:53Z

Thanks! Merging to master/2.1

gatorsmile · 2017-03-01T15:57:16Z

@stanzhai Could you submit another PR to backport it to Spark 2.0?

stanzhai · 2017-03-02T01:34:34Z

ok

gatorsmile · 2017-03-02T03:27:35Z

sql/core/src/test/resources/sql-tests/results/inner-join.sql.out

@@ -0,0 +1,68 @@
+-- Automatically generated by SQLQueryTestSuite
+-- Number of queries: 13


Actually, this number is wrong. Next time, please do not manually change this file. You should run the command to generate the file. @stanzhai

Thanks!
I will pay attention to this next time.

…hould not be folded by FoldablePropagation rule This PR fix for branch-2.0 Refer #17099 gatorsmile Author: Stan Zhai <zhaishidan@haizhi.com> Closes #17131 from stanzhai/fix-inner-join-2.0.

stanzhai added 3 commits February 28, 2017 20:44

fix inner join

02b9dd6

fix inner-join.sql.out

112dd23

update test

4463648

stanzhai changed the title ~~Constant alias columns in INNER JOIN should not be folded by FoldablePropagation rule~~ [SPARK-19766][SQL] Constant alias columns in INNER JOIN should not be folded by FoldablePropagation rule Feb 28, 2017

hvanhovell reviewed Feb 28, 2017

View reviewed changes

revert and add check stop

fc819f8

Merge branch 'master' of https://github.com/apache/spark into fix-inn…

c197b20

…er-join

rxin reviewed Feb 28, 2017

View reviewed changes

add Propagate in inner join test case into FoldablePropagationSuite

15fae50

gatorsmile reviewed Mar 1, 2017

View reviewed changes

stanzhai added 2 commits March 1, 2017 14:12

fix test case

5611861

Merge branch 'master' of https://github.com/apache/spark into fix-inn…

df11cc4

…er-join

asfgit closed this in 5502a9c Mar 1, 2017

gatorsmile reviewed Mar 2, 2017

View reviewed changes

stanzhai mentioned this pull request Mar 2, 2017

[SPARK-19766][SQL][BRANCH-2.0] Constant alias columns in INNER JOIN should not be folded by FoldablePropagation rule #17131

Closed

		@@ -0,0 +1,68 @@
		-- Automatically generated by SQLQueryTestSuite
		-- Number of queries: 13

[SPARK-19766][SQL] Constant alias columns in INNER JOIN should not be folded by FoldablePropagation rule #17099

[SPARK-19766][SQL] Constant alias columns in INNER JOIN should not be folded by FoldablePropagation rule #17099

Uh oh!

Conversation

stanzhai commented Feb 28, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

stanzhai commented Feb 28, 2017

Uh oh!

hvanhovell commented Feb 28, 2017

Uh oh!

hvanhovell Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 28, 2017

Uh oh!

SparkQA commented Feb 28, 2017

Uh oh!

rxin Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 28, 2017

Uh oh!

gatorsmile commented Feb 28, 2017

Uh oh!

gatorsmile Mar 1, 2017

Choose a reason for hiding this comment

Uh oh!

gatorsmile Mar 1, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 1, 2017

Uh oh!

stanzhai commented Mar 1, 2017

Uh oh!

gatorsmile commented Mar 1, 2017

Uh oh!

SparkQA commented Mar 1, 2017

Uh oh!

gatorsmile commented Mar 1, 2017

Uh oh!

gatorsmile commented Mar 1, 2017

Uh oh!

stanzhai commented Mar 2, 2017

Uh oh!

gatorsmile Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

stanzhai Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hvanhovell Feb 28, 2017 •

edited

Loading