[SPARK-19017][SQL] NOT IN subquery with more than one column may return incorrect results #16467

nsyca · 2017-01-04T01:39:28Z

What changes were proposed in this pull request?

This PR fixes the code in Optimizer phase where the NULL-aware expression of a NOT IN query is expanded in Rule RewritePredicateSubquery.

Example:
The query

select a1,b1
from t1
where (a1,b1) not in (select a2,b2
from t2);

has the (a1, b1) = (a2, b2) rewritten from (before this fix):

Join LeftAnti, ((isnull((_1#2 = a2#16)) || isnull((_2#3 = b2#17))) || ((_1#2 = a2#16) && (_2#3 = b2#17)))

to (after this fix):

Join LeftAnti, (((_1#2 = a2#16) || isnull((_1#2 = a2#16))) && ((_2#3 = b2#17) || isnull((_2#3 = b2#17))))

How was this patch tested?

sql/test, catalyst/test and new test cases in SQLQueryTestSuite.

…rrect results ## What changes were proposed in this pull request? This patch fixes the incorrect results in the rule ResolveSubquery in Catalyst's Analysis phase. ## How was this patch tested? ./dev/run-tests a new unit test on the problematic pattern.

nsyca · 2017-01-04T01:41:24Z

...re/src/test/resources/sql-tests/results/subquery/in-subquery/not-in-multiple-columns.sql.out

+struct<a1:int,b1:int>
+-- !query 2 output
+2	1
+


It returns an empty set without this fix.

Why is (null, 2) missing? There is no tuple in t2 for which b2=2.

Let's consider this:

(null, 2) NOT IN { (1, 1), (null, 3), (1, null) }

which is equal to

.... AND (null <> 1 OR 2 <> null) => ... AND (unknown OR unknown) => ... AND unknown => unknown

Therefore (null, 2) is not part of the result set.

Ok yeah you are right. I was confusing this with the or rules.

nsyca · 2017-01-04T01:41:33Z

...re/src/test/resources/sql-tests/results/subquery/in-subquery/not-in-multiple-columns.sql.out

+struct<a1:int,b1:int>
+-- !query 3 output
+1	1
+


It returns an empty set without this fix.

nsyca · 2017-01-04T01:41:39Z

...re/src/test/resources/sql-tests/results/subquery/in-subquery/not-in-multiple-columns.sql.out

+-- !query 4 schema
+struct<a1:int,b1:int>
+-- !query 4 output
+1	1


It returns an empty set without this fix.

nsyca · 2017-01-04T01:45:01Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

+      if (!parent.exists()) {
+        assert(parent.mkdirs(), "Could not create directory: " + parent)
+      }
+      stringToFile(resultFile, goldenOutput)


This newly added code is to address an issue, when test files are located in a hierarchy of sub-directories, at the time the golden result files are generated it could happen that the structure of those sub-directories are not yet created. The code will create the required sub-directories.

nsyca · 2017-01-04T01:47:31Z

sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala

-      sql("select * from l where a not in (select c from t where b < d)"),
-      Row(1, 2.0) :: Row(1, 2.0) :: Row(3, 3.0) :: Nil)
+      sql("select * from l where (a, b) not in (select c, d from t) and a < 4"),
+      Row(1, 2.0) :: Row(1, 2.0) :: Row(2, 1.0) :: Row(2, 1.0) :: Row(3, 3.0) :: Nil)


Query with correlated predicates in NOT IN subquery could generate incorrect results (this problem is tracked by SPARK-18966). With this fix, it reveals the problem. Here I modify the test case to cover the code path for multiple columns instead.

nsyca · 2017-01-04T01:50:13Z

sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala

    // Empty sub-query
    checkAnswer(
-      sql("select * from l where a not in (select c from r where c > 10 and b < d)"),
+      sql("select * from l where (a, b) not in (select c, d from r where c > 10)"),


With the predicate c > 10 (which filters all the rows in the subquery), it covers up the correlated predicate problem. Instead of removing the test case completely, I just modify to have a different coverage for multiple columns.

Then we also should test an empty subquery :)

This test case is effectively covering the case of empty subquery.

nsyca · 2017-01-04T02:07:56Z

The new test file is placed in the directory structure similar to the one proposed in SPARK-18871 (#16337).

SparkQA · 2017-01-04T03:58:10Z

Test build #70849 has finished for PR 16467 at commit de655d0.

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

hvanhovell · 2017-01-04T17:11:41Z

I left a comment on the JIRA. It feel it is important to reach consensus on the semantics first.

Other than that the PR looks good.

nsyca · 2017-01-04T17:31:05Z

Thank you for taking time to review the PR. I agree we need to reach consensus on the semantics.

hvanhovell

Looks pretty good. I have one question regarding the result of a test case. Otherwise only minor things.

hvanhovell · 2017-01-05T23:02:19Z

sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala

    // Empty sub-query
    checkAnswer(
-      sql("select * from l where a not in (select c from r where c > 10 and b < d)"),
+      sql("select * from l where (a, b) not in (select c, d from r where c > 10)"),


Then we also should test an empty subquery :)

hvanhovell · 2017-01-05T23:02:24Z

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

+          // to
+          //   (a1=a2 OR isnull(a1=a2)) AND (b1=b2 OR isnull(b1=b2)) AND ...
+          val joinConds = splitConjunctivePredicates(joinCond.get)
+          val isNulls = joinConds.map(IsNull)


Minor - We can write this more directly:

val pairs = joinConds.map(c => Or(c, IsNull(c))).reduceLeft(And)

Thank you. I have changed the code.

hvanhovell · 2017-01-05T23:24:48Z

...re/src/test/resources/sql-tests/results/subquery/in-subquery/not-in-multiple-columns.sql.out

+struct<a1:int,b1:int>
+-- !query 2 output
+2	1
+


Why is (null, 2) missing? There is no tuple in t2 for which b2=2.

SparkQA · 2017-01-06T03:56:00Z

Test build #70948 has finished for PR 16467 at commit f3f7773.

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

nsyca · 2017-01-09T20:36:54Z

@hvanhovell, please let me know if you are waiting for my replies to your comments. I believe I have addressed all of them. thanks!

SparkQA · 2017-01-13T03:36:20Z

Test build #71283 has finished for PR 16467 at commit 6a1a415.

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

nsyca · 2017-01-16T15:51:33Z

@hvanhovell Would there be anything left that I have not addressed in this PR?

hvanhovell · 2017-01-24T22:30:06Z

LGTM - merging to master/2.1. Thanks!

Sorry for the long wait.

…rn incorrect results ## What changes were proposed in this pull request? This PR fixes the code in Optimizer phase where the NULL-aware expression of a NOT IN query is expanded in Rule `RewritePredicateSubquery`. Example: The query select a1,b1 from t1 where (a1,b1) not in (select a2,b2 from t2); has the (a1, b1) = (a2, b2) rewritten from (before this fix): Join LeftAnti, ((isnull((_1#2 = a2#16)) || isnull((_2#3 = b2#17))) || ((_1#2 = a2#16) && (_2#3 = b2#17))) to (after this fix): Join LeftAnti, (((_1#2 = a2#16) || isnull((_1#2 = a2#16))) && ((_2#3 = b2#17) || isnull((_2#3 = b2#17)))) ## How was this patch tested? sql/test, catalyst/test and new test cases in SQLQueryTestSuite. Author: Nattavut Sutyanyong <[email protected]> Closes #16467 from nsyca/19017. (cherry picked from commit cdb691e) Signed-off-by: Herman van Hovell <[email protected]>

…rn incorrect results ## What changes were proposed in this pull request? This PR fixes the code in Optimizer phase where the NULL-aware expression of a NOT IN query is expanded in Rule `RewritePredicateSubquery`. Example: The query select a1,b1 from t1 where (a1,b1) not in (select a2,b2 from t2); has the (a1, b1) = (a2, b2) rewritten from (before this fix): Join LeftAnti, ((isnull((_1#2 = a2#16)) || isnull((_2#3 = b2#17))) || ((_1#2 = a2#16) && (_2#3 = b2#17))) to (after this fix): Join LeftAnti, (((_1#2 = a2#16) || isnull((_1#2 = a2#16))) && ((_2#3 = b2#17) || isnull((_2#3 = b2#17)))) ## How was this patch tested? sql/test, catalyst/test and new test cases in SQLQueryTestSuite. Author: Nattavut Sutyanyong <[email protected]> Closes apache#16467 from nsyca/19017.

nsyca added 16 commits July 29, 2016 17:43

New positive test cases

edca333

Fix unit test case failure

64184fd

blocking TABLESAMPLE

29f82b0

Fixing code styling

ac43ab4

Correcting Scala test style

631d396

One (last) attempt to correct the Scala style tests

7eb9b2d

Merge remote-tracking branch 'upstream/master'

1387cf5

Merge remote-tracking branch 'upstream/master'

3faa2d5

Merge remote-tracking branch 'upstream/master'

a308634

first try on the fix

473c81b

add/update test cases

278ebae

Merge remote-tracking branch 'upstream/master'

f1524b9

Merge branch 'master' into 19017

9e1b29e

Add descriptive comment

de655d0

nsyca commented Jan 4, 2017

View reviewed changes

hvanhovell requested changes Jan 5, 2017

View reviewed changes

code optimization

f3f7773

nsyca added 3 commits January 12, 2017 20:05

Merge remote-tracking branch 'upstream/master'

5c36dce

Merge branch 'master' into 19017

05d2683

remove trail whitespaces

6a1a415

nsyca mentioned this pull request Jan 13, 2017

[SPARK-18863][SQL] Output non-aggregate expressions without GROUP BY in a subquery does not yield an error #16572

Closed

asfgit closed this in cdb691e Jan 24, 2017

nsyca deleted the 19017 branch March 14, 2017 21:07

[SPARK-19017][SQL] NOT IN subquery with more than one column may return incorrect results #16467

[SPARK-19017][SQL] NOT IN subquery with more than one column may return incorrect results #16467

Uh oh!

Conversation

nsyca commented Jan 4, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsyca Jan 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsyca commented Jan 4, 2017

Uh oh!

SparkQA commented Jan 4, 2017

Uh oh!

hvanhovell commented Jan 4, 2017

Uh oh!

nsyca commented Jan 4, 2017

Uh oh!

hvanhovell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 6, 2017

Uh oh!

nsyca commented Jan 9, 2017

Uh oh!

SparkQA commented Jan 13, 2017

Uh oh!

nsyca commented Jan 16, 2017

Uh oh!

hvanhovell commented Jan 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nsyca Jan 6, 2017 •

edited

Loading