Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-26637][SQL] Makes GetArrayItem nullability more precise #23566

Closed
wants to merge 3 commits into from

Conversation

maropu
Copy link
Member

@maropu maropu commented Jan 16, 2019

What changes were proposed in this pull request?

In the master, GetArrayItem nullable is always true;

But, If input array size is constant and ordinal is foldable, we could make GetArrayItem nullability more precise. This pr added code to make GetArrayItem nullability more precise.

How was this patch tested?

Added tests in ComplexTypeSuite.

@SparkQA
Copy link

SparkQA commented Jan 16, 2019

Test build #101316 has finished for PR 23566 at commit bb9b37f.

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

ar(intOrdinal).nullable
case GetArrayStructFields(CreateArray(ar), _, _, _, containsNull)
if intOrdinal < ar.length =>
containsNull
Copy link
Contributor

@cloud-fan cloud-fan Jan 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case GetArrayStructFields(CreateArray(elements), field, _, _, _) if intOrdinal < elements.length =>
  elements(i).nullable || field.nullable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@SparkQA
Copy link

SparkQA commented Jan 21, 2019

Test build #101475 has finished for PR 23566 at commit 1f9106f.

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

@maropu maropu force-pushed the GetArrayItemNullability branch from 1f9106f to 40a598d Compare January 22, 2019 01:05
val f2 = StructField("b", IntegerType, nullable = true)
val structType = StructType(f1 :: f2 :: Nil)
val c = AttributeReference("c", structType, nullable = false)()
val stArray1 = GetArrayStructFields(CreateArray(c :: Nil), f1, 0, 2, containsNull = f1.nullable)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

containsNull should be c.nullable, according to the logic in ExtractValue.apply.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I'll update soon.

@SparkQA
Copy link

SparkQA commented Jan 22, 2019

Test build #101503 has finished for PR 23566 at commit 40a598d.

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


val d = AttributeReference("d", structType, nullable = true)()
val stArray3 = GetArrayStructFields(CreateArray(c :: d :: Nil), f1, 0, 2,
containsNull = f1.nullable)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@SparkQA
Copy link

SparkQA commented Jan 22, 2019

Test build #101532 has finished for PR 23566 at commit 13b9c3f.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 1ed1b4d Jan 23, 2019
dongjoon-hyun pushed a commit that referenced this pull request Jan 28, 2019
## What changes were proposed in this pull request?
In master, `GetMapValue` nullable is always true;
https://github.com/apache/spark/blob/cf133e611020ed178f90358464a1b88cdd9b7889/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala#L371

But, If input key is foldable, we could make its nullability more precise.
This fix is the same with SPARK-26637(#23566).

## How was this patch tested?
Added tests in `ComplexTypeSuite`.

Closes #23669 from maropu/SPARK-26747.

Authored-by: Takeshi Yamamuro <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
## What changes were proposed in this pull request?
In the master, GetArrayItem nullable is always true;
https://github.com/apache/spark/blob/cf133e611020ed178f90358464a1b88cdd9b7889/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala#L236

But, If input array size is constant and ordinal is foldable, we could make GetArrayItem nullability more precise. This pr added code to make `GetArrayItem` nullability more precise.

## How was this patch tested?
Added tests in `ComplexTypeSuite`.

Closes apache#23566 from maropu/GetArrayItemNullability.

Authored-by: Takeshi Yamamuro <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
## What changes were proposed in this pull request?
In master, `GetMapValue` nullable is always true;
https://github.com/apache/spark/blob/cf133e611020ed178f90358464a1b88cdd9b7889/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala#L371

But, If input key is foldable, we could make its nullability more precise.
This fix is the same with SPARK-26637(apache#23566).

## How was this patch tested?
Added tests in `ComplexTypeSuite`.

Closes apache#23669 from maropu/SPARK-26747.

Authored-by: Takeshi Yamamuro <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
cloud-fan pushed a commit that referenced this pull request Mar 4, 2019
… cases

## What changes were proposed in this pull request?
In master, `ElementAt` nullable is always true;
https://github.com/apache/spark/blob/be1cadf16dc70e22eae144b3dfce9e269ef95acc/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L1977

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`.

Closes #23867 from maropu/SPARK-26965.

Authored-by: Takeshi Yamamuro <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants