Skip to content

[SPARK-21351][SQL] Remove the UpdateAttributeNullability rule from the optimizer - #23508

Closed
maropu wants to merge 2 commits into
apache:masterfrom
maropu:SPARK-21351
Closed

[SPARK-21351][SQL] Remove the UpdateAttributeNullability rule from the optimizer#23508
maropu wants to merge 2 commits into
apache:masterfrom
maropu:SPARK-21351

Conversation

@maropu

@maropumaropu commented Jan 10, 2019

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

This pr removed UpdateAttributeNullability from the optimizer because the same logic happens in the analyzer. See SPARK-26459(#23390) for more detailed discussion.

How was this patch tested?

N/A

@SparkQA

SparkQA commented Jan 10, 2019

Copy link
Copy Markdown

Test build #101017 has finished for PR 23508 at commit c0bcfae.

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

@maropu

maropu commented Jan 10, 2019

Copy link
Copy Markdown
MemberAuthor

@cloud-fan@gatorsmile It seems the removal don't affect the existing tests now.

cloud-fan
cloud-fan reviewed Jan 11, 2019
View reviewed changes
Comment thread...ala/org/apache/spark/sql/catalyst/optimizer/UpdateAttributeNullabilityInOptimizerSuite.scala
// but the AttributeReference is not updated to reflect this. So, we need to update nullability
// by the `UpdateAttributeNullability` rule.
val original = rel
.select(GetArrayItem(CreateArray(Seq('a, 'a + 1L)), 0) as "b")

@cloud-fancloud-fanJan 11, 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.

Shall we update GetArrayItem.nullable, to special-handle the combination of CreateArray and Literal?

@maropumaropuJan 11, 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.

Ah, I see, we can do so. Is it ok to file a separate jira and make a pr for that in following activities?

@cloud-fancloud-fanJan 11, 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.

sure

@maropumaropuJan 16, 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.

Made a pr in #23566

@dongjoon-hyun

dongjoon-hyun commented Mar 8, 2019

Copy link
Copy Markdown
Member

Retest this please.

@SparkQA

SparkQA commented Mar 9, 2019

Copy link
Copy Markdown

Test build #103226 has finished for PR 23508 at commit c0bcfae.

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

@maropu

maropu commented Mar 9, 2019

Copy link
Copy Markdown
MemberAuthor

ping @cloud-fan@gatorsmile@dongjoon-hyun

@dongjoon-hyun

dongjoon-hyun commented Mar 10, 2019

Copy link
Copy Markdown
Member

Hi, @maropu .
Based on the our discussion history and the recent nullability fixes, we decided to remove this from the optimizer step.

As a the final piece of this removal, could you remove the following UpdateAttributeNullability optimizer description? Otherwise, someone may try to add it back.

 *
* This rule should be executed again at the end of optimization phase, as optimizer may change
* some expressions and their nullabilities as well. See SPARK-21351 for more details.

@maropu

maropu commented Mar 10, 2019

Copy link
Copy Markdown
MemberAuthor

oh, yeah. I'll do that.

@SparkQA

SparkQA commented Mar 11, 2019

Copy link
Copy Markdown

Test build #103285 has finished for PR 23508 at commit 9f94605.

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

@cloud-fan

cloud-fan commented Mar 11, 2019

Copy link
Copy Markdown
Contributor

thanks, merging to master!

@JoshRosenJoshRosen mentioned this pull request Jul 15, 2019
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fancloud-fancloud-fan left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@maropu@SparkQA@dongjoon-hyun@cloud-fan