[SPARK-29145][SQL][FOLLOW-UP] Move tests from `SubquerySuite` to `subquery/in-subquery/in-joins.sql` #26406

AngersZhuuuu · 2019-11-06T03:08:11Z

What changes were proposed in this pull request?

Follow comment of #25854 (comment)

Why are the changes needed?

NO

Does this PR introduce any user-facing change?

NO

How was this patch tested?

ADD TEST CASE

AngersZhuuuu · 2019-11-06T05:02:30Z

@gatorsmile @maropu
Currently, use exists in join condition with subquery have problem with LEFT OUTER JOIN & FULL OUTER JOIN.
Ok to explain but can't execute.

AngersZhuuuu · 2019-11-06T06:06:27Z


create temporary view s1 as select * from values
    (1), (3), (5), (7), (9)
  as s1(id);

create temporary view s2 as select * from values
    (1), (3), (4), (6), (9)
  as s2(id);

create temporary view s3 as select * from values
    (3), (4), (6), (9)
  as s3(id);

 explain extended SELECT s1.id, s2.id as id2 FROM s1
 LEFT OUTER JOIN s2 ON s1.id = s2.id
 AND EXISTS (SELECT * FROM s3 WHERE s3.id > 6)

LogicalPlan

      == Parsed Logical Plan ==
'Project ['s1.id, 's2.id AS id2#291]
+- 'Join LeftOuter, (('s1.id = 's2.id) AND exists#290 [])
   :  +- 'Project [*]
   :     +- 'Filter ('s3.id > 6)
   :        +- 'UnresolvedRelation [s3]
   :- 'UnresolvedRelation [s1]
   +- 'UnresolvedRelation [s2]

== Analyzed Logical Plan ==
id: int, id2: int
Project [id#244, id#250 AS id2#291]
+- Join LeftOuter, ((id#244 = id#250) AND exists#290 [])
   :  +- Project [id#256]
   :     +- Filter (id#256 > 6)
   :        +- SubqueryAlias `s3`
   :           +- Project [value#253 AS id#256]
   :              +- LocalRelation [value#253]
   :- SubqueryAlias `s1`
   :  +- Project [value#241 AS id#244]
   :     +- LocalRelation [value#241]
   +- SubqueryAlias `s2`
      +- Project [value#247 AS id#250]
         +- LocalRelation [value#247]

== Optimized Logical Plan ==
Project [id#244, id#250 AS id2#291]
+- Join LeftOuter, (exists#290 [] AND (id#244 = id#250))
   :  +- Project [value#253 AS id#256]
   :     +- Filter (value#253 > 6)
   :        +- LocalRelation [value#253]
   :- Project [value#241 AS id#244]
   :  +- LocalRelation [value#241]
   +- Project [value#247 AS id#250]
      +- LocalRelation [value#247]

== Physical Plan ==
*(2) Project [id#244, id#250 AS id2#291]
+- *(2) BroadcastHashJoin [id#244], [id#250], LeftOuter, BuildRight, exists#290 []
   :  +- Project [value#253 AS id#256]
   :     +- Filter (value#253 > 6)
   :        +- LocalRelation [value#253]
   :- *(2) Project [value#241 AS id#244]
   :  +- *(2) LocalTableScan [value#241]
   +- BroadcastExchange HashedRelationBroadcastMode(List(cast(input[0, int, false] as bigint))), [id=#670]
      +- *(1) Project [value#247 AS id#250]
         +- *(1) LocalTableScan [value#247]

Exist can't be change to other form of the join, and exists 's child plan was not change to Physical Plan in the whole Physic plan.

+- *(2) BroadcastHashJoin [id#244], [id#250], LeftOuter, BuildRight, exists#290 []
   :  +- Project [value#253 AS id#256]
   :     +- Filter (value#253 > 6)
   :        +- LocalRelation [value#253]

maropu · 2019-11-06T07:21:51Z

How about moving these tests into subquery/in-subquery/in-joins.sql instead of making a new file? Also, I think we need to execute multiple runs for these tests with different configurations;

--SET spark.sql.autoBroadcastJoinThreshold=10485760
--SET spark.sql.autoBroadcastJoinThreshold=-1,spark.sql.join.preferSortMergeJoin=true
--SET spark.sql.autoBroadcastJoinThreshold=-1,spark.sql.join.preferSortMergeJoin=false

maropu · 2019-11-06T07:24:52Z

Currently, use exists in join condition with subquery have problem with LEFT OUTER JOIN & FULL OUTER JOIN.
Ok to explain but can't execute.

sorry, but I miss your point. You meant that this test could run correctly on SubquerySuite.scala but it couldn't run on SQLQueryTestSuite? I think its ok just to move the tests you added in the previous PR into SQLQueryTestSuite.

AngersZhuuuu · 2019-11-06T07:33:43Z

sorry, but I miss your point. You meant that this test could run correctly on SubquerySuite.scala but it couldn't run on SQLQueryTestSuite? I think its ok just to move the tests you added in the previous PR into SQLQueryTestSuite.

No, current catalyst can't support well for EXISTS when use this as join's on condition and join type is LeftOuter/FullOuter like I have show in #26406 (comment)

In the end of above comment, you can see that some part of Physical Plan is not a Physical Plan , There is a LocalRelation, we can't doCodeGen for the whole physical plan.

maropu · 2019-11-06T08:07:58Z

Ur.... I see. You found the bug caused by your previous pr, right? If so, could you file a new jira for that and investigate the root cause of the bug there?

AngersZhuuuu · 2019-11-06T08:14:55Z

Ur.... I see. You found the bug caused by your previous pr, right? If so, could you file a new jira for that and investigate the root cause of the bug there?

I don't think it is caused by my pr. My pr focus on IN/NOT IN, behavior between IN/NOT IN and Exists/NOT EXISTS is different.
Run this example in 2.4.0

== Parsed Logical Plan ==
'Project ['s1.id, 's2.id AS id2#4]
+- 'Join LeftOuter, (('s1.id = 's2.id) && exists#3 [])
   :  +- 'Project [*]
   :     +- 'Filter ('s3.id > 6)
   :        +- 'UnresolvedRelation `s3`
   :- 'UnresolvedRelation `s1`
   +- 'UnresolvedRelation `s2`

== Analyzed Logical Plan ==
org.apache.spark.sql.AnalysisException: Table or view not found: `s3`; line 3 pos 27;
'Project ['s1.id, 's2.id AS id2#4]
+- 'Join LeftOuter, ((id#0 = id#1) && exists#3 [])
   :  +- 'Project [*]
   :     +- 'Filter ('s3.id > 6)
   :        +- 'UnresolvedRelation `s3`
   :- SubqueryAlias `s1`
   :  +- Project [id#0]
   :     +- SubqueryAlias `s1`
   :        +- LocalRelation [id#0]
   +- SubqueryAlias `s2`
      +- Project [id#1]
         +- SubqueryAlias `s2`
            +- LocalRelation [id#1]

org.apache.spark.sql.AnalysisException: Table or view not found: `s3`; line 3 pos 27;
'Project ['s1.id, 's2.id AS id2#4]
+- 'Join LeftOuter, ((id#0 = id#1) && exists#3 [])
   :  +- 'Project [*]
   :     +- 'Filter ('s3.id > 6)
   :        +- 'UnresolvedRelation `s3`
   :- SubqueryAlias `s1`
   :  +- Project [id#0]
   :     +- SubqueryAlias `s1`
   :        +- LocalRelation [id#0]
   +- SubqueryAlias `s2`
      +- Project [id#1]
         +- SubqueryAlias `s2`
            +- LocalRelation [id#1]

== Optimized Logical Plan ==
org.apache.spark.sql.AnalysisException: Table or view not found: `s3`; line 3 pos 27;
'Project ['s1.id, 's2.id AS id2#4]
+- 'Join LeftOuter, ((id#0 = id#1) && exists#3 [])
   :  +- 'Project [*]
   :     +- 'Filter ('s3.id > 6)
   :        +- 'UnresolvedRelation `s3`
   :- SubqueryAlias `s1`
   :  +- Project [id#0]
   :     +- SubqueryAlias `s1`
   :        +- LocalRelation [id#0]
   +- SubqueryAlias `s2`
      +- Project [id#1]
         +- SubqueryAlias `s2`
            +- LocalRelation [id#1]

== Physical Plan ==
org.apache.spark.sql.AnalysisException: Table or view not found: `s3`; line 3 pos 27;
'Project ['s1.id, 's2.id AS id2#4]
+- 'Join LeftOuter, ((id#0 = id#1) && exists#3 [])
   :  +- 'Project [*]
   :     +- 'Filter ('s3.id > 6)
   :        +- 'UnresolvedRelation `s3`
   :- SubqueryAlias `s1`
   :  +- Project [id#0]
   :     +- SubqueryAlias `s1`
   :        +- LocalRelation [id#0]
   +- SubqueryAlias `s2`
      +- Project [id#1]
         +- SubqueryAlias `s2`
            +- LocalRelation [id#1]
Time taken: 1.455 seconds, Fetched 1 row(s)

AngersZhuuuu · 2019-11-06T08:17:00Z

Ur.... I see. You found the bug caused by your previous pr, right? If so, could you file a new jira for that and investigate the root cause of the bug there?

My pr make this situation pass Optimizer level. == , Anyway, I will raise a issue and explain the change and root cause.
Also try to make it work.

maropu · 2019-11-06T08:27:40Z

If so, could you separate the two work: the porting task you requested and the bug fix? I'm pretty confused now.

AngersZhuuuu · 2019-11-06T08:32:23Z

If so, could you separate the two work: the porting task you requested and the bug fix? I'm pretty confused now.

Yeah, I want to make it clear to you since in comment #25854 (comment) @gatorsmile mentioned two problem together. I will raise a issue for EXISTS/NOT EXISTS problem.

AngersZhuuuu · 2019-11-10T03:34:08Z

How about moving these tests into subquery/in-subquery/in-joins.sql instead of making a new file? Also, I think we need to execute multiple runs for these tests with different configurations;
--SET spark.sql.autoBroadcastJoinThreshold=10485760
--SET spark.sql.autoBroadcastJoinThreshold=-1,spark.sql.join.preferSortMergeJoin=true
--SET spark.sql.autoBroadcastJoinThreshold=-1,spark.sql.join.preferSortMergeJoin=false

move to in-join.sql, it will be run in three configuration auto?

maropu · 2019-11-10T11:30:44Z

Yea, you need to nothing for that. btw, still WIP in this pr?

AngersZhuuuu · 2019-11-10T11:38:04Z

Yea, you need to nothing for that. btw, still WIP in this pr?

Remove WIP

maropu · 2019-11-10T11:44:01Z

Can you remove the test of SubquerySuite that you added in the previous pr? https://github.com/apache/spark/blob/master/sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala#L207-L353

AngersZhuuuu · 2019-11-10T11:57:48Z

Can you remove the test of SubquerySuite that you added in the previous pr? https://github.com/apache/spark/blob/master/sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala#L207-L353

Done

maropu · 2019-11-10T12:08:34Z

sql/core/src/test/resources/sql-tests/inputs/subquery/in-subquery/in-joins.sql

+create temporary view s3 as select * from values
+    (3), (4), (6), (9)
+  as s3(id);
+


nit: Plz drop view in the end?

Plz drop view in the end?

About t1, t2, t3, seems in other test file, don't drop table.

maropu · 2019-11-10T12:09:09Z

ok to test

SparkQA · 2019-11-10T15:46:12Z

Test build #113536 has finished for PR 26406 at commit 6df31c6.

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

SparkQA · 2019-11-10T15:55:43Z

Test build #113538 has finished for PR 26406 at commit 65fe860.

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

AngersZhuuuu · 2019-11-12T07:27:12Z

@maropu Passed test, any more work to do?

maropu

cc: @dongjoon-hyun @HyukjinKwon

dongjoon-hyun · 2019-11-13T01:28:07Z

Thank you for pinging me, @maropu .

dongjoon-hyun

+1, LGTM. Merged to master.

maropu · 2019-11-13T03:32:55Z

Thanks, @dongjoon-hyun !

add test case

6392652

AngersZhuuuu changed the title ~~[SPARK-29145][SQL][FOLLOW-UP] Port in-subquery-on-join-condition.sql~~ [WIP][SPARK-29145][SQL][FOLLOW-UP] Port in-subquery-on-join-condition.sql Nov 6, 2019

dongjoon-hyun added the SQL label Nov 6, 2019

follow comment

9c25e80

maropu changed the title ~~[WIP][SPARK-29145][SQL][FOLLOW-UP] Port in-subquery-on-join-condition.sql~~ [SPARK-29145][SQL][FOLLOW-UP] Port in-subquery-on-join-condition.sql Nov 10, 2019

maropu changed the title ~~[SPARK-29145][SQL][FOLLOW-UP] Port in-subquery-on-join-condition.sql~~ [SPARK-29145][SQL][FOLLOW-UP] Move tests from SubquerySuite to subquery/in-subquery/in-joins.sql Nov 10, 2019

remove test case in SubquerySuite

6df31c6

maropu reviewed Nov 10, 2019

View reviewed changes

drop test table

65fe860

maropu approved these changes Nov 13, 2019

View reviewed changes

dongjoon-hyun approved these changes Nov 13, 2019

View reviewed changes

dongjoon-hyun closed this in eb79af8 Nov 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-29145][SQL][FOLLOW-UP] Move tests from `SubquerySuite` to `subquery/in-subquery/in-joins.sql` #26406

[SPARK-29145][SQL][FOLLOW-UP] Move tests from `SubquerySuite` to `subquery/in-subquery/in-joins.sql` #26406

AngersZhuuuu commented Nov 6, 2019

AngersZhuuuu commented Nov 6, 2019 •

edited

Loading

AngersZhuuuu commented Nov 6, 2019

maropu commented Nov 6, 2019 •

edited

Loading

maropu commented Nov 6, 2019 •

edited

Loading

AngersZhuuuu commented Nov 6, 2019

maropu commented Nov 6, 2019

AngersZhuuuu commented Nov 6, 2019

AngersZhuuuu commented Nov 6, 2019

maropu commented Nov 6, 2019

AngersZhuuuu commented Nov 6, 2019

AngersZhuuuu commented Nov 10, 2019

maropu commented Nov 10, 2019

AngersZhuuuu commented Nov 10, 2019

maropu commented Nov 10, 2019

AngersZhuuuu commented Nov 10, 2019

maropu Nov 10, 2019 •

edited

Loading

AngersZhuuuu Nov 10, 2019

maropu commented Nov 10, 2019

SparkQA commented Nov 10, 2019

SparkQA commented Nov 10, 2019

AngersZhuuuu commented Nov 12, 2019

maropu left a comment

dongjoon-hyun commented Nov 13, 2019

dongjoon-hyun left a comment

maropu commented Nov 13, 2019

[SPARK-29145][SQL][FOLLOW-UP] Move tests from SubquerySuite to subquery/in-subquery/in-joins.sql #26406

[SPARK-29145][SQL][FOLLOW-UP] Move tests from SubquerySuite to subquery/in-subquery/in-joins.sql #26406

Conversation

AngersZhuuuu commented Nov 6, 2019

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?

AngersZhuuuu commented Nov 6, 2019 • edited Loading

AngersZhuuuu commented Nov 6, 2019

maropu commented Nov 6, 2019 • edited Loading

maropu commented Nov 6, 2019 • edited Loading

AngersZhuuuu commented Nov 6, 2019

maropu commented Nov 6, 2019

AngersZhuuuu commented Nov 6, 2019

AngersZhuuuu commented Nov 6, 2019

maropu commented Nov 6, 2019

AngersZhuuuu commented Nov 6, 2019

AngersZhuuuu commented Nov 10, 2019

maropu commented Nov 10, 2019

AngersZhuuuu commented Nov 10, 2019

maropu commented Nov 10, 2019

AngersZhuuuu commented Nov 10, 2019

maropu Nov 10, 2019 • edited Loading

Choose a reason for hiding this comment

AngersZhuuuu Nov 10, 2019

Choose a reason for hiding this comment

maropu commented Nov 10, 2019

SparkQA commented Nov 10, 2019

SparkQA commented Nov 10, 2019

AngersZhuuuu commented Nov 12, 2019

maropu left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Nov 13, 2019

dongjoon-hyun left a comment

Choose a reason for hiding this comment

maropu commented Nov 13, 2019

[SPARK-29145][SQL][FOLLOW-UP] Move tests from `SubquerySuite` to `subquery/in-subquery/in-joins.sql` #26406

[SPARK-29145][SQL][FOLLOW-UP] Move tests from `SubquerySuite` to `subquery/in-subquery/in-joins.sql` #26406

AngersZhuuuu commented Nov 6, 2019 •

edited

Loading

maropu commented Nov 6, 2019 •

edited

Loading

maropu commented Nov 6, 2019 •

edited

Loading

maropu Nov 10, 2019 •

edited

Loading