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-12904][SQL] Strength reduction for integral and decimal literal comparisons #10882

Closed
wants to merge 7 commits into from

Conversation

rxin
Copy link
Contributor

@rxin rxin commented Jan 23, 2016

This pull request implements strength reduction for comparing integral expressions and decimal literals, which is more common now because we switch to parsing fractional literals as decimal types (rather than doubles). I added the rules to the existing DecimalPrecision rule with some refactoring to simplify the control flow. I also moved DecimalPrecision rule into its own file due to the growing size.

def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperators {
// fix decimal precision for expressions
case q => q.transformExpressions(
decimalAndDecimal.orElse(integralAndDecimalLiteral).orElse(nondecimalAndDecimal))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i broke the previous monolithic decimal precision rule into 2 parts, and then added integralAndDecimalLiteral

@rxin
Copy link
Contributor Author

rxin commented Jan 23, 2016

This should subsume #10845

cc @viirya - I refactored the code so it is a lot cleaner, and looked at overflow cases.

@viirya
Copy link
Member

viirya commented Jan 23, 2016

LGTM

@SparkQA
Copy link

SparkQA commented Jan 23, 2016

Test build #2447 has finished for PR 10882 at commit d5acd17.

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

@SparkQA
Copy link

SparkQA commented Jan 23, 2016

Test build #49932 has finished for PR 10882 at commit d5acd17.

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

@rxin
Copy link
Contributor Author

rxin commented Jan 23, 2016

Merging this in master.

@asfgit asfgit closed this in 423783a Jan 23, 2016
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