[SPARK-25416][SQL] ArrayPosition function may return incorrect result when right expression is implicitly down casted by dilipbiswal · Pull Request #22407 · apache/spark

dilipbiswal · 2018-09-12T22:03:53Z

What changes were proposed in this pull request?

In ArrayPosition, we currently cast the right hand side expression to match the element type of the left hand side Array. This may result in down casting and may return wrong result or questionable result.

Example :

spark-sql>select array_position(array(1), 1.34);
1

spark-sql>select array_position(array(1), 'foo');
null

We should safely coerce both left and right hand side expressions.

How was this patch tested?

Added tests in DataFrameFunctionsSuite

… right expression is implicitly down casted.

SparkQA · 2018-09-12T23:56:20Z

Test build #96008 has finished for PR 22407 at commit 629ec84.

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

dilipbiswal · 2018-09-13T00:00:18Z

retest this please

SparkQA · 2018-09-13T03:59:04Z

Test build #96014 has finished for PR 22407 at commit 629ec84.

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

gatorsmile · 2018-09-13T04:59:25Z

cc @ueshin

ueshin

Good catch!
I'm now wondering how about other expressions.
Seems like ArrayContains, ArrayRemove, and ElementAt for map types could have the same problem?
Can you confirm and fix them? Or we can address them in separate prs. Thanks!

ueshin · 2018-09-13T05:31:21Z

- case TypeCheckResult.TypeCheckSuccess =>
+ (left.dataType, right.dataType) match {
+ case (ArrayType(e1, _), e2) if e1.sameType(e2) =>
 TypeUtils.checkForOrderingExpr(right.dataType, s"function $prettyName")


nit: we can use e1 or e2 instead of right.dataType?

@ueshin Sure

ueshin · 2018-09-13T05:54:31Z

+ checkAnswer(
+ df.selectExpr("array_position(array(1.23D), 1)"),
+ Seq(Row(0L), Row(0L))
+ )


What about array_position(array(1.0D), 1)?

@ueshin Sure.. i will add this case.

dilipbiswal · 2018-09-13T06:05:30Z

Seems like ArrayContains, ArrayRemove, and ElementAt for map types could have the same problem?
@ueshin I have a PR opened for ArrayContains link. I will go through ArrayRemove and ElementAt fix if necessary.

ueshin · 2018-09-13T06:08:47Z

@dilipbiswal Thanks! I'll take a look at it.

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

Test build #96024 has finished for PR 22407 at commit bb18108.

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

dilipbiswal · 2018-09-13T07:07:54Z

retest this please.

SparkQA · 2018-09-13T08:01:22Z

Test build #96028 has finished for PR 22407 at commit bb18108.

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

SparkQA · 2018-09-13T11:11:17Z

Test build #96027 has finished for PR 22407 at commit bb18108.

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

dilipbiswal · 2018-09-21T05:49:44Z

@ueshin Wenchen thought it may be risky to backport the fix to tighestCommonType. Given this, can this be looked at now ?

ueshin · 2018-09-21T12:21:11Z

So, before #22448 (branch-2.4 if we don't backport #22448), we don't coerce between decimals, and after that (master, not merged yet though), we will do, right?
I'm okay with the behavior.

I'd retrigger the build because it's been a long time since the last build.

ueshin · 2018-09-21T12:21:23Z

Jenkins, retest this please.

ueshin · 2018-09-21T12:27:36Z

LGTM, pending Jenkins.

SparkQA · 2018-09-21T16:47:02Z

Test build #96421 has finished for PR 22407 at commit bb18108.

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

cloud-fan · 2018-09-22T13:42:36Z

 )

+ checkAnswer(
+ df.selectExpr("array_position(array(1), 1.23D)"),


same problem here. The test doesn't read any column from df, so we should use OneRowRelation.

cloud-fan · 2018-09-22T13:42:52Z

LGTM except one comment

SparkQA · 2018-09-22T23:04:45Z

Test build #96475 has finished for PR 22407 at commit 55d4b95.

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

… when right expression is implicitly down casted ## What changes were proposed in this pull request? In ArrayPosition, we currently cast the right hand side expression to match the element type of the left hand side Array. This may result in down casting and may return wrong result or questionable result. Example : ```SQL spark-sql> select array_position(array(1), 1.34); 1 ``` ```SQL spark-sql> select array_position(array(1), 'foo'); null ``` We should safely coerce both left and right hand side expressions. ## How was this patch tested? Added tests in DataFrameFunctionsSuite Closes#22407 from dilipbiswal/SPARK-25416. Authored-by: Dilip Biswal <dbiswal@us.ibm.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit bb49661) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2018-09-24T13:44:22Z

thanks, merging to master/2.4!

dilipbiswal · 2018-09-24T21:34:29Z

@cloud-fan @ueshin Thank you very much !!

[SPARK-25415] ArrayPosition function may return incorrect result when…
629ec84
… right expression is implicitly down casted.

dilipbiswal changed the title ~~[SPARK-25415] ArrayPosition function may return incorrect result when right expression is implicitly down casted~~[SPARK-25415][SQL] ArrayPosition function may return incorrect result when right expression is implicitly down castedSep 12, 2018

dilipbiswal changed the title ~~[SPARK-25415][SQL] ArrayPosition function may return incorrect result when right expression is implicitly down casted~~[SPARK-25416][SQL] ArrayPosition function may return incorrect result when right expression is implicitly down castedSep 12, 2018

ueshin reviewed Sep 13, 2018
View reviewed changes

Code review
bb18108

cloud-fan reviewed Sep 22, 2018
View reviewed changes

Code review
55d4b95

asfgit closed this in bb49661Sep 24, 2018

Uh oh!

Conversation

dilipbiswal commented Sep 12, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Sep 12, 2018

Uh oh!

dilipbiswal commented Sep 13, 2018

Uh oh!

SparkQA commented Sep 13, 2018

Uh oh!

gatorsmile commented Sep 13, 2018

Uh oh!

ueshin 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!

ueshinSep 13, 2018

Choose a reason for hiding this comment

Uh oh!

dilipbiswalSep 13, 2018

Choose a reason for hiding this comment

Uh oh!

ueshinSep 13, 2018

Choose a reason for hiding this comment

Uh oh!

dilipbiswalSep 13, 2018

Choose a reason for hiding this comment

Uh oh!

dilipbiswal commented Sep 13, 2018

Uh oh!

ueshin commented Sep 13, 2018

Uh oh!

SparkQA commented Sep 13, 2018

Uh oh!

dilipbiswal commented Sep 13, 2018

Uh oh!

SparkQA commented Sep 13, 2018

Uh oh!

SparkQA commented Sep 13, 2018

Uh oh!

dilipbiswal commented Sep 21, 2018

Uh oh!

ueshin commented Sep 21, 2018

Uh oh!

ueshin commented Sep 21, 2018

Uh oh!

ueshin commented Sep 21, 2018

Uh oh!

SparkQA commented Sep 21, 2018

Uh oh!

cloud-fanSep 22, 2018

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Sep 22, 2018

Uh oh!

SparkQA commented Sep 22, 2018

Uh oh!

cloud-fan commented Sep 24, 2018

Uh oh!

dilipbiswal commented Sep 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ueshin left a comment •
edited
Loading