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-21351][SQL] Remove the UpdateAttributeNullability rule from the optimizer #23508

Closed
wants to merge 2 commits into from

Conversation

maropu
Copy link
Member

@maropu maropu commented Jan 10, 2019

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

SparkQA commented Jan 10, 2019

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
Copy link
Member Author

maropu commented Jan 10, 2019

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

// 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")
Copy link
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?

Copy link
Member Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure

Copy link
Member Author

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
Copy link
Member

Retest this please.

@SparkQA
Copy link

SparkQA commented Mar 9, 2019

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
Copy link
Member Author

maropu commented Mar 9, 2019

@dongjoon-hyun
Copy link
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
Copy link
Member Author

maropu commented Mar 10, 2019

oh, yeah. I'll do that.

@SparkQA
Copy link

SparkQA commented Mar 11, 2019

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
Copy link
Contributor

thanks, merging to master!

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.

4 participants