Skip to content

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

Closed
maropu wants to merge 3 commits into
apache:masterfrom
maropu:GetArrayItemNullability
Closed

[SPARK-26637][SQL] Makes GetArrayItem nullability more precise#23566
maropu wants to merge 3 commits into
apache:masterfrom
maropu:GetArrayItemNullability

Conversation

@maropu

@maropumaropu commented Jan 16, 2019

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

In the master, GetArrayItem nullable is always true;

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

Line 236 in cf133e6

overridedefnullable:Boolean=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.

@maropumaropu mentioned this pull request Jan 16, 2019
Closed
@SparkQA

SparkQA commented Jan 16, 2019

Copy link
Copy Markdown

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.

cloud-fan
cloud-fan reviewed Jan 21, 2019
View reviewed changes
Comment thread...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala Outdated
ar(intOrdinal).nullable
case GetArrayStructFields(CreateArray(ar), _, _, _, containsNull)
if intOrdinal < ar.length =>
containsNull

@cloud-fancloud-fanJan 21, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@maropumaropuJan 21, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

ok

@SparkQA

SparkQA commented Jan 21, 2019

Copy link
Copy Markdown

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
maropuforce-pushed the GetArrayItemNullability branch from 1f9106f to 40a598dCompareJanuary 22, 2019 01:05
cloud-fan
cloud-fan reviewed Jan 22, 2019
View reviewed changes
Comment threadsql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ComplexTypeSuite.scala Outdated
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)

@cloud-fancloud-fanJan 22, 2019

Copy link
Copy Markdown
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.

@maropumaropuJan 22, 2019

Copy link
Copy Markdown
MemberAuthor

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

SparkQA commented Jan 22, 2019

Copy link
Copy Markdown

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.

cloud-fan
cloud-fan reviewed Jan 22, 2019
View reviewed changes
Comment threadsql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ComplexTypeSuite.scala Outdated

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

@cloud-fancloud-fanJan 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ditto

@SparkQA

SparkQA commented Jan 22, 2019

Copy link
Copy Markdown

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

cloud-fan commented Jan 23, 2019

Copy link
Copy Markdown
Contributor

thanks, merging to master!

@maropumaropu mentioned this pull request Jan 28, 2019
Closed
dongjoon-hyun pushed a commit that referenced this pull request Jan 28, 2019
@maropu@dongjoon-hyun
[SPARK-26747][SQL] Makes GetMapValue nullability more precise
92706e6
## 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 <yamamuro@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@maropuJackey Lee
[SPARK-26637][SQL] Makes GetArrayItem nullability more precise
734725c
## 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`.
Closesapache#23566 from maropu/GetArrayItemNullability.
Authored-by: Takeshi Yamamuro <yamamuro@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@maropuJackey Lee
[SPARK-26747][SQL] Makes GetMapValue nullability more precise
2a30ff4
## 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`.
Closesapache#23669 from maropu/SPARK-26747.
Authored-by: Takeshi Yamamuro <yamamuro@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
@maropumaropu mentioned this pull request Feb 22, 2019
Closed
cloud-fan pushed a commit that referenced this pull request Mar 4, 2019
@maropu@cloud-fan
[SPARK-26965][SQL] Makes ElementAt nullability more precise for array…
68fbbbe
… 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 <yamamuro@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fancloud-fancloud-fan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@maropu@SparkQA@cloud-fan