[SPARK-22249][SQL] isin with empty list throws exception on cached DataFrame #19494

mgaido91 · 2017-10-13T16:00:17Z

What changes were proposed in this pull request?

As pointed out in the JIRA, there is a bug which causes an exception to be thrown if isin is called with an empty list on a cached DataFrame. The PR fixes it.

How was this patch tested?

Added UT.

…taFrame

srowen

This should just be simplified to list.exists(...)

srowen · 2017-10-13T18:07:11Z

sql/core/src/main/scala/org/apache/spark/sql/execution/columnar/InMemoryTableScanExec.scala

    case In(a: AttributeReference, list: Seq[Expression]) if list.forall(_.isInstanceOf[Literal]) =>
      list.map(l => statsFor(a).lowerBound <= l.asInstanceOf[Literal] &&
-        l.asInstanceOf[Literal] <= statsFor(a).upperBound).reduce(_ || _)
+        l.asInstanceOf[Literal] <= statsFor(a).upperBound)


This still looks more complex and less efficient than

list.exists(l => statsFor(a).lowerBound <= l.asInstanceOf[Literal] && l.asInstanceOf[Literal] <= statsFor(a).upperBound)

or better

val stats = statsFor(a) list.exists { l => val literal = l.asInstanceOf[Literal] stats.lowerBound <= literal && literal <= stats.upperBound }

The point being that you should be able to short-circuit evaluation here.
Or have I missed something basic like that these aren't Booleans?

Exactly, they aren't Booleans. They are Expressions. And this is a PartialFunction which must return an Expression. Thus I can't think of a different (more efficient) way of doing this.

I see. How does .contains(true) work then? or did that not work?
I suppose all I mean is that we should write something that works on an empty list (returns false?) and also short-circuits (stops when anything is true). Is that possible?

It was a mistake, sorry. It returned always false.
I see what you mean, but in this piece of code we are only building the Expression and we are not evaluating it. Thus it is not possible to short-circuit, because the Expression must be built entirely.

OK I understand. Rather than build an extra term into the expression every time, why not special-case an empty list? return Literal(false) if empty, otherwise return the same thing as before.

I think this is a bit more elegant and concise as code style, but your idea can be more efficient (though I am not sure how much overhead is introduced by a False evaluation). Then I am updating this PR according to your suggestion, thanks.

mgaido91 · 2017-10-15T09:20:23Z

@srowen I also updated the UT to check all the possible cases.

srowen

This seems OK to me. @dongjoon-hyun do you think this is OK? I think you wrote the original lines.

dongjoon-hyun

Thank you for pining me, @srowen .
And, thank you for fixing this, @mgaido91 .
+1, LGTM.

HyukjinKwon · 2017-10-16T04:52:01Z

ok to test

SparkQA · 2017-10-16T07:05:01Z

Test build #82780 has finished for PR 19494 at commit f5bc105.

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

HyukjinKwon · 2017-10-16T07:07:27Z

retest this please

SparkQA · 2017-10-16T09:48:44Z

Test build #82789 has finished for PR 19494 at commit f5bc105.

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

HyukjinKwon

LGTM too

…taFrame ## What changes were proposed in this pull request? As pointed out in the JIRA, there is a bug which causes an exception to be thrown if `isin` is called with an empty list on a cached DataFrame. The PR fixes it. ## How was this patch tested? Added UT. Author: Marco Gaido <marcogaido91@gmail.com> Closes #19494 from mgaido91/SPARK-22249. (cherry picked from commit 8148f19) Signed-off-by: Sean Owen <sowen@cloudera.com>

srowen · 2017-10-17T07:42:31Z

Merged to master/2.2. It didn't cherry pick cleanly to earlier branches but if it's needed there and someone opens a backport PR I can merge that too

gatorsmile · 2017-10-17T18:40:12Z

sql/core/src/main/scala/org/apache/spark/sql/execution/columnar/InMemoryTableScanExec.scala

    case IsNotNull(a: Attribute) => statsFor(a).count - statsFor(a).nullCount > 0

+    case In(_: AttributeReference, list: Seq[Expression]) if list.isEmpty => Literal.FalseLiteral
    case In(a: AttributeReference, list: Seq[Expression]) if list.forall(_.isInstanceOf[Literal]) =>


Please change it to

if list.forall(_.isInstanceOf[Literal]) && list.nonEmpty()

or using exists.

gatorsmile · 2017-10-17T18:40:27Z

sql/core/src/main/scala/org/apache/spark/sql/execution/columnar/InMemoryTableScanExec.scala

    case IsNull(a: Attribute) => statsFor(a).nullCount > 0
    case IsNotNull(a: Attribute) => statsFor(a).count - statsFor(a).nullCount > 0

+    case In(_: AttributeReference, list: Seq[Expression]) if list.isEmpty => Literal.FalseLiteral


This should be moved to optimizer rules. It will cover both cached and non-cached cases.

BTW, this is wrong. If the left value is null, it should return null instead of false.

Ah, really. OK, yeah we need to change this. It can be reverted too.

since this is only for filters, does it make any difference null or false?

Under this special context of filtering partitions, this In with empty list will result in a false literal in the end, no matter the attribute is null or not. We don't possibly have some expressions like IsNull(In(a, Nil)) as the filter predicate for now.

gatorsmile · 2017-10-17T18:42:44Z

...core/src/test/scala/org/apache/spark/sql/execution/columnar/InMemoryColumnarQuerySuite.scala

    }
  }
+
+  test("SPARK-22249: IN should work also with cached DataFrame") {


This test case is just an end-to-end test. This test will still pass if the optimizer has a change. We also need a unit test case.

do you mean this test case is not enough and I should add a test case to check the proper behavior of the optimizer after the change?

gatorsmile · 2017-10-17T18:43:17Z

Sorry for my late review. @mgaido91 Could you submit a follow-up PR to address my comments. Thanks!

gatorsmile · 2017-10-17T19:12:45Z

I will not revert this PR but please submit the fix ASAP.

mgaido91 · 2017-10-17T20:51:31Z

I will, thanks for your suggestions @gatorsmile.

mgaido91 · 2017-10-17T21:48:11Z

I created the followup PR: #19522. Thanks.

…n the optimizer ## What changes were proposed in this pull request? This PR addresses the comments by gatorsmile on [the previous PR](#19494). ## How was this patch tested? Previous UT and added UT. Author: Marco Gaido <marcogaido91@gmail.com> Closes #19522 from mgaido91/SPARK-22249_FOLLOWUP. (cherry picked from commit 1f25d86) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

…n the optimizer ## What changes were proposed in this pull request? This PR addresses the comments by gatorsmile on [the previous PR](apache#19494). ## How was this patch tested? Previous UT and added UT. Author: Marco Gaido <marcogaido91@gmail.com> Closes apache#19522 from mgaido91/SPARK-22249_FOLLOWUP.

…taFrame ## What changes were proposed in this pull request? As pointed out in the JIRA, there is a bug which causes an exception to be thrown if `isin` is called with an empty list on a cached DataFrame. The PR fixes it. ## How was this patch tested? Added UT. Author: Marco Gaido <marcogaido91@gmail.com> Closes apache#19494 from mgaido91/SPARK-22249. (cherry picked from commit 8148f19) Signed-off-by: Sean Owen <sowen@cloudera.com>

…n the optimizer ## What changes were proposed in this pull request? This PR addresses the comments by gatorsmile on [the previous PR](apache#19494). ## How was this patch tested? Previous UT and added UT. Author: Marco Gaido <marcogaido91@gmail.com> Closes apache#19522 from mgaido91/SPARK-22249_FOLLOWUP. (cherry picked from commit 1f25d86) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

[SPARK-22249][SQL] isin with empty list throws exception on cached Da…

549ff15

…taFrame

srowen requested changes Oct 13, 2017

View reviewed changes

fix wrong implementation

979ac9d

srowen reviewed Oct 13, 2017

View reviewed changes

mgaido91 added 2 commits October 15, 2017 11:17

rewrite not to add a false to the normal case

9efb897

added tests

f5bc105

srowen approved these changes Oct 15, 2017

View reviewed changes

dongjoon-hyun approved these changes Oct 15, 2017

View reviewed changes

HyukjinKwon approved these changes Oct 17, 2017

View reviewed changes

asfgit closed this in 8148f19 Oct 17, 2017

gatorsmile reviewed Oct 17, 2017

View reviewed changes

mgaido91 mentioned this pull request Oct 17, 2017

[SPARK-22249][FOLLOWUP][SQL] Check if list of value for IN is empty in the optimizer #19522

Closed

mgaido91 deleted the SPARK-22249 branch November 4, 2017 08:49

[SPARK-22249][SQL] isin with empty list throws exception on cached DataFrame #19494

[SPARK-22249][SQL] isin with empty list throws exception on cached DataFrame #19494

Uh oh!

Conversation

mgaido91 commented Oct 13, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgaido91 Oct 13, 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!

mgaido91 commented Oct 15, 2017

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Oct 16, 2017

Uh oh!

SparkQA commented Oct 16, 2017

Uh oh!

HyukjinKwon commented Oct 16, 2017

Uh oh!

SparkQA commented Oct 16, 2017

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

srowen commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gatorsmile Oct 17, 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!

gatorsmile Oct 17, 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!

gatorsmile commented Oct 17, 2017

Uh oh!

gatorsmile commented Oct 17, 2017

Uh oh!

mgaido91 commented Oct 17, 2017

Uh oh!

mgaido91 commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

mgaido91 Oct 13, 2017 •

edited

Loading

srowen commented Oct 17, 2017 •

edited

Loading

gatorsmile Oct 17, 2017 •

edited

Loading

gatorsmile Oct 17, 2017 •

edited

Loading