[SPARK-26965][SQL] Makes ElementAt nullability more precise for array cases #23867

maropu · 2019-02-22T04:10:19Z

What changes were proposed in this pull request?

In master, ElementAt nullable is always true;

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

Line 1977 in be1cadf

override def nullable: Boolean = true

But, If input is an array and foldable, we could make its nullability more precise.
This fix is based on SPARK-26637(#23566).

How was this patch tested?

Added tests in CollectionExpressionsSuite.

SparkQA · 2019-02-22T08:05:01Z

Test build #102617 has finished for PR 23867 at commit a3709b9.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ElementAt(left: Expression, right: Expression)
trait GetArrayItemUtil extends BinaryExpression
case class GetArrayItem(child: Expression, ordinal: Expression)
trait GetMapValueUtil extends BinaryExpression with ImplicitCastInputTypes

dilipbiswal · 2019-02-22T08:10:25Z

retest this please

SparkQA · 2019-02-22T12:33:23Z

Test build #102630 has finished for PR 23867 at commit a3709b9.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ElementAt(left: Expression, right: Expression)
trait GetArrayItemUtil extends BinaryExpression
case class GetArrayItem(child: Expression, ordinal: Expression)
trait GetMapValueUtil extends BinaryExpression with ImplicitCastInputTypes

maropu · 2019-02-22T23:35:27Z

@dongjoon-hyun could you check?

dongjoon-hyun · 2019-02-25T00:36:47Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala

-
-  override def toString: String = s"$child[$ordinal]"
-  override def sql: String = s"${child.sql}[${ordinal.sql}]"
+trait GetArrayItemUtil extends BinaryExpression {


Can we move this after case class GetArrayItem to reduce the diff?

dongjoon-hyun · 2019-02-25T00:39:17Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala

- * Common base class for [[GetMapValue]] and [[ElementAt]].
+ * Common trait for [[GetMapValue]] and [[ElementAt]].
 */
+trait GetMapValueUtil extends BinaryExpression with ImplicitCastInputTypes {


Does GetMapValueUtil need to extend extends BinaryExpression with ImplicitCastInputTypes? If child and key are required, computeNullabilityFromMap seems to able to accept the child and key as parameters. How do you think that way?

It seems BinaryExpression.nullSafeCodeGen is used in the other place?

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

Line 326 in 2d2fb34

nullSafeCodeGen(ctx, ev, (eval1, eval2) => {

Thanks. Then, what about GetArrayItemUtil? Actually, the reason why I asked is that this PR makes some non-trivial lineages like the followings.

def BinaryExpression.left -> private val GetArrayItemUtil.child -> override def GetArrayItem.left.

def BinaryExpression.left -> private val GetMapValueUtil.child -> override def GetMapValue.left

ah, ok. I'll simplify it.

dongjoon-hyun · 2019-02-25T00:42:56Z

Thank you for pinging me, @maropu . I left a few comments. I understand this follows the original code structure, but it seems to become more complicated than the required.

SparkQA · 2019-02-25T05:41:41Z

Test build #102729 has finished for PR 23867 at commit c2d522a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait GetArrayItemUtil extends BinaryExpression

SparkQA · 2019-02-26T07:03:33Z

Test build #102779 has finished for PR 23867 at commit 7590955.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait GetArrayItemUtil

dongjoon-hyun · 2019-02-28T00:18:33Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala

+  /** `Null` is returned for invalid ordinals. */
+  protected def computeNullabilityFromMap: Boolean = if (key.foldable && !key.nullable) {
+    val keyObj = key.eval()
+    child match {


Thank you for updating, @maropu . For this one, can we simplify like the following? We can remove these alias variables.

- private val child = left - private val key = right - /** `Null` is returned for invalid ordinals. */ - protected def computeNullabilityFromMap: Boolean = if (key.foldable && !key.nullable) { - val keyObj = key.eval() - child match { + protected def computeNullabilityFromMap: Boolean = if (right.foldable && !right.nullable) { + val keyObj = right.eval() + left match {

If you want to keep the clear meaning, you may declare them inside of this function. But, since we are not using parameters, I think we already have some assumptions that this is inside BinaryExpression. So, for me, I just want to avoid making aliases like my previous comment.

protected def computeNullabilityFromMap: Boolean = { val child = left val key = right .... }

dongjoon-hyun · 2019-02-28T05:58:05Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala

 case class GetArrayItem(child: Expression, ordinal: Expression)
-  extends BinaryExpression with ExpectsInputTypes with ExtractValue with NullIntolerant {
+  extends BinaryExpression with GetArrayItemUtil with ExpectsInputTypes with ExtractValue
+    with NullIntolerant {


indentation?

dongjoon-hyun · 2019-02-28T05:59:57Z

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

    checkEvaluation(ElementAt(mb0, Literal(Array[Byte](3, 4))), null)
  }

+  test("SPARK-26965 correctly handles ElementAt nullability for arrays") {


Since this is a new feature, it seems that we don't need JIRA ID, SPARK-26965, in the test case name.

I dropped that in the latest commit though, any rule for that?

Yes. I've heard the rule from @cloud-fan in my old PRs.

I'm not sure if that is a written rule or not.

Hi, @cloud-fan . Do we have some URLs for the above SPARK JIRA ID usage rule in the test case name?

oh, I see. I'll be more careful next time, thanks!

dongjoon-hyun · 2019-02-28T06:15:27Z

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

  since = "2.4.0")
-case class ElementAt(left: Expression, right: Expression) extends GetMapValueUtil {
+case class ElementAt(left: Expression, right: Expression)
+    extends GetMapValueUtil with GetArrayItemUtil {


indentation?

SparkQA · 2019-03-01T04:01:09Z

Test build #102885 has finished for PR 23867 at commit 2cb0bf9.

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

maropu · 2019-03-01T04:02:32Z

retest this please

dongjoon-hyun · 2019-03-01T05:17:30Z

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

Ur, ditto. SPARK-26965.

dongjoon-hyun · 2019-03-01T05:18:40Z

+1, LGTM (except one nit comment).

Ping, @cloud-fan .

cloud-fan · 2019-03-01T07:12:47Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala

+    left match {
+      case m: CreateMap if m.resolved =>
+        m.keys.zip(m.values).filter { case (k, _) => k.foldable && !k.nullable }.find {
+          case (k, _) if k.eval() == keyObj => true


can we rely on ==? What if one expression returns unsafe row and the other returns safe row?

I know this is existing code, but after a hindsight maybe it's not worth to linear scan the map entries and just to get the precise nullability.

Ah, I see. I'll update soon.

I dropped the nullablity computation for the map side and added the TODO comment there. How about this?

SparkQA · 2019-03-01T08:15:25Z

Test build #102897 has finished for PR 23867 at commit 2cb0bf9.

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

SparkQA · 2019-03-01T09:40:31Z

Test build #102901 has finished for PR 23867 at commit 499a57b.

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

dongjoon-hyun · 2019-03-04T00:56:14Z

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

-  override def nullable: Boolean = true
+  override def nullable: Boolean = left.dataType match {
+    case _: ArrayType => computeNullabilityFromArray(left, right)
+    case _: MapType => computeNullabilityFromMap


If Map is out of the scope, shall we update the title and use the following code path instead?

Remove extends GetMapValueUtil.

Remove computeNullabilityFromMap and use the following.

case _: MapType => true

cloud-fan · 2019-03-04T02:37:22Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala

  override def left: Expression = child
  override def right: Expression = key

  /** `Null` is returned for invalid ordinals. */


we need to update this comment, to say why the nullability is always true.

SparkQA · 2019-03-04T04:44:25Z

Test build #102958 has finished for PR 23867 at commit 7faf51a.

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

SparkQA · 2019-03-04T05:20:22Z

Test build #102959 has finished for PR 23867 at commit b95a389.

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

SparkQA · 2019-03-04T08:05:01Z

Test build #102969 has finished for PR 23867 at commit 95f2862.

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

dilipbiswal · 2019-03-04T08:08:26Z

retest this please

SparkQA · 2019-03-04T12:26:27Z

Test build #102973 has finished for PR 23867 at commit 95f2862.

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

cloud-fan · 2019-03-04T13:27:31Z

thanks, merging to master!

Fix

a3709b9

kiszk approved these changes Feb 24, 2019

View reviewed changes

dongjoon-hyun reviewed Feb 25, 2019

View reviewed changes

Fix

c2d522a

Fix

7590955

dongjoon-hyun reviewed Feb 28, 2019

View reviewed changes

dongjoon-hyun reviewed Mar 1, 2019

View reviewed changes

Fix

499a57b

maropu force-pushed the SPARK-26965 branch from 2cb0bf9 to 499a57b Compare March 1, 2019 05:19

cloud-fan reviewed Mar 1, 2019

View reviewed changes

Fix

7faf51a

dongjoon-hyun reviewed Mar 4, 2019

View reviewed changes

Fix

b95a389

maropu force-pushed the SPARK-26965 branch from c333b4a to b95a389 Compare March 4, 2019 01:03

maropu changed the title ~~[SPARK-26965][SQL] Makes ElementAt nullability more precise~~ [SPARK-26965][SQL] Makes ElementAt nullability more precise for array cases Mar 4, 2019

cloud-fan reviewed Mar 4, 2019

View reviewed changes

Add comments

95f2862

cloud-fan closed this in 68fbbbe Mar 4, 2019

[SPARK-26965][SQL] Makes ElementAt nullability more precise for array cases #23867

[SPARK-26965][SQL] Makes ElementAt nullability more precise for array cases #23867

Uh oh!

Conversation

maropu commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Feb 22, 2019

Uh oh!

dilipbiswal commented Feb 22, 2019

Uh oh!

SparkQA commented Feb 22, 2019

Uh oh!

maropu commented Feb 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Feb 25, 2019 • 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!

dongjoon-hyun commented Feb 25, 2019

Uh oh!

SparkQA commented Feb 25, 2019

Uh oh!

SparkQA commented Feb 26, 2019

Uh oh!

dongjoon-hyun Feb 28, 2019 • 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!

dongjoon-hyun Feb 28, 2019 • 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!

SparkQA commented Mar 1, 2019

Uh oh!

maropu commented Mar 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Mar 1, 2019

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 Mar 1, 2019

Uh oh!

SparkQA commented Mar 1, 2019

Uh oh!

dongjoon-hyun Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

maropu commented Feb 22, 2019 •

edited

Loading

dongjoon-hyun Feb 25, 2019 •

edited

Loading

dongjoon-hyun Feb 28, 2019 •

edited

Loading

dongjoon-hyun Feb 28, 2019 •

edited

Loading

dongjoon-hyun Mar 4, 2019 •

edited

Loading