[SPARK-25308][SQL] ArrayContains function may return a error in the code generation phase. #22315

dilipbiswal · 2018-09-02T05:47:04Z

What changes were proposed in this pull request?

Invoking ArrayContains function with non nullable array type throws the following error in the code generation phase. Below is the error snippet.

Code generation of array_contains([1,2,3], 1) failed:
java.util.concurrent.ExecutionException: org.codehaus.commons.compiler.CompileException: File 'generated.java', Line 40, Column 11: failed to compile: org.codehaus.commons.compiler.CompileException: File 'generated.java', Line 40, Column 11: Expression "isNull_0" is not an rvalue
java.util.concurrent.ExecutionException: org.codehaus.commons.compiler.CompileException: File 'generated.java', Line 40, Column 11: failed to compile: org.codehaus.commons.compiler.CompileException: File 'generated.java', Line 40, Column 11: Expression "isNull_0" is not an rvalue
	at com.google.common.util.concurrent.AbstractFuture$Sync.getValue(AbstractFuture.java:306)
	at com.google.common.util.concurrent.AbstractFuture$Sync.get(AbstractFuture.java:293)
	at com.google.common.util.concurrent.AbstractFuture.get(AbstractFuture.java:116)
	at com.google.common.util.concurrent.Uninterruptibles.getUninterruptibly(Uninterruptibles.java:135)
	at com.google.common.cache.LocalCache$Segment.getAndRecordStats(LocalCache.java:2410)
	at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2380)
	at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2342)
	at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2257)
	at com.google.common.cache.LocalCache.get(LocalCache.java:4000)
	at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4004)
	at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:4874)
	at org.apache.spark.sql.catalyst.expressions.codegen.CodeGenerator$.compile(CodeGenerator.scala:1305)

How was this patch tested?

Added test in CollectionExpressionSuite.

SparkQA · 2018-09-02T07:05:01Z

Test build #95586 has finished for PR 22315 at commit 84b135c.

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

dilipbiswal · 2018-09-02T07:06:30Z

retest this please

mgaido91

The fix LGTM, just some minor comments to address. We might want to add a test for non-nullable types to all operators. Probably we can create an helper for this in order to avoid this issue in the future. What do you think? cc @cloud-fan @gatorsmile

mgaido91 · 2018-09-02T08:17:31Z

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

nit: extra newline

mgaido91 · 2018-09-02T08:18:00Z

...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala

nit: spark-xxxxx?

SparkQA · 2018-09-02T11:00:52Z

Test build #95587 has finished for PR 22315 at commit 84b135c.

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

kiszk · 2018-09-02T14:06:21Z

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

Could you update this part based on this comment instead of if (...) { ... } else?

kiszk · 2018-09-02T14:06:56Z

Thank you
LGTM except one comment.

SparkQA · 2018-09-02T14:14:29Z

Test build #95589 has finished for PR 22315 at commit 6f2881e.

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

SparkQA · 2018-09-02T23:21:31Z

Test build #95593 has finished for PR 22315 at commit 4190f42.

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

maropu · 2018-09-03T02:16:14Z

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

How about the case right.nullable = true and
left.nullable = false AND left.dataType.asInstanceOf[ArrayType].containsNull = false?

@maropu This should be rejected in checkInputDataTypes(), no ?

maropu · 2018-09-03T02:21:49Z

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

nit: How about def checkAndSetIsNullCode(body: String) = if (nullable) {?

@maropu ok..

cloud-fan · 2018-09-03T10:48:15Z

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

This seems too complicated to save a few duplicated code, how about

val loopBody = if (nullable) { s""" |if ($arr.isNullAt($i)) { | ${ev.isNull} = true; |} else ... """ } else { s""" |if (${ctx.genEqual(right.dataType, value, getValue)}) {... """ }

SparkQA · 2018-09-03T11:45:11Z

Test build #95608 has finished for PR 22315 at commit c18fc08.

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

SparkQA · 2018-09-03T19:06:21Z

Test build #95634 has finished for PR 22315 at commit 712542c.

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

dilipbiswal · 2018-09-03T19:17:53Z

retest this please

SparkQA · 2018-09-03T21:15:09Z

Test build #95636 has finished for PR 22315 at commit 712542c.

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

…eneration phase.

cloud-fan · 2018-09-04T00:34:10Z

LGTM

SparkQA · 2018-09-04T01:15:44Z

Test build #95638 has finished for PR 22315 at commit 59ddb99.

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

ueshin · 2018-09-04T04:23:54Z

LGTM.

ueshin · 2018-09-04T04:27:50Z

Thanks! merging to master.

gatorsmile · 2018-09-04T04:37:42Z

@dilipbiswal Could we also add the test cases for the other high-order functions, if missing?

dilipbiswal · 2018-09-04T04:40:32Z

@gatorsmile Sure.. I will check and add.

ueshin · 2018-09-04T04:49:39Z

@dilipbiswal Do we need to backport this to 2.3? If so, could you submit a backport pr to branch-2.3 please? Thanks!

dilipbiswal · 2018-09-04T06:00:14Z

@ueshin Just verified in 2.3. This problem does not exist in 2.3. This is due to the fact that implementation of nullSafeCodeGen is different in 2.3 than in master. However, we are missing the test cases we added in these PRs in 2.3. Should we have the test cases checked in into the branch ? I am afraid that if we ever backported the pr that changed nullSafeCodeGen , we may introduce this bug. Please advise ..

ueshin · 2018-09-04T07:01:30Z

@dilipbiswal Thanks for the verification! I don't think we will backport the nullSafeCodeGen to branch-2.3, but feel free to submit a pr to backport the test cases to branch-2.3.

This was referenced Sep 2, 2018

[SPARK-25307][SQL] ArraySort function may return an error in the code generation phase #22314

Closed

[SPARK-25267][SQL][TEST] Disable ConvertToLocalRelation in the test cases of sql/core and sql/hive #22270

Closed

mgaido91 reviewed Sep 2, 2018

View reviewed changes

kiszk reviewed Sep 2, 2018

View reviewed changes

maropu reviewed Sep 3, 2018

View reviewed changes

cloud-fan reviewed Sep 3, 2018

View reviewed changes

dilipbiswal added 5 commits September 3, 2018 14:19

[SPARK-25308] ArrayContains function may return a error in the code g…

f69c693

…eneration phase.

Code review

db3c3c0

Code review

e47a442

Code review

e17bb46

Code review

59ddb99

dilipbiswal force-pushed the SPARK-25308 branch from 712542c to 59ddb99 Compare September 3, 2018 21:19

asfgit closed this in 8e21696 Sep 4, 2018

[SPARK-25308][SQL] ArrayContains function may return a error in the code generation phase. #22315

[SPARK-25308][SQL] ArrayContains function may return a error in the code generation phase. #22315

Uh oh!

Conversation

dilipbiswal commented Sep 2, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Sep 2, 2018

Uh oh!

dilipbiswal commented Sep 2, 2018

Uh oh!

mgaido91 left a comment • 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!

SparkQA commented Sep 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk commented Sep 2, 2018

Uh oh!

SparkQA commented Sep 2, 2018

Uh oh!

SparkQA commented Sep 2, 2018

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!

SparkQA commented Sep 3, 2018

Uh oh!

SparkQA commented Sep 3, 2018

Uh oh!

dilipbiswal commented Sep 3, 2018

Uh oh!

SparkQA commented Sep 3, 2018

Uh oh!

cloud-fan commented Sep 4, 2018

Uh oh!

SparkQA commented Sep 4, 2018

Uh oh!

ueshin commented Sep 4, 2018

Uh oh!

ueshin commented Sep 4, 2018

Uh oh!

gatorsmile commented Sep 4, 2018

Uh oh!

dilipbiswal commented Sep 4, 2018

Uh oh!

ueshin commented Sep 4, 2018

Uh oh!

dilipbiswal commented Sep 4, 2018

Uh oh!

ueshin commented Sep 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mgaido91 left a comment •

edited

Loading