Skip to content

[SPARK-12904][SQL] Strength reduction for integral/decimal comparisons #10845

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.

Sign up for GitHub

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

Jump to bottom
Closed
viirya wants to merge 5 commits into from
Closed

[SPARK-12904][SQL] Strength reduction for integral/decimal comparisons #10845

viirya wants to merge 5 commits into from

Conversation

@viirya
Copy link
Member

@viirya viirya commented Jan 20, 2016

JIRA: https://issues.apache.org/jira/browse/SPARK-12904

We can do the following strength reduction for comparisons between an integral column and a decimal literal:

  1. int_col > decimal_literal => int_col > floor(decimal_literal)
  2. int_col >= decimal_literal => int_col >= ceil(decimal_literal)
  3. int_col < decimal_literal => int_col < ceil(decimal_literal)
  4. int_col <= decimal_literal => int_col <= floor(decimal_literal)
  5. decimal_literal > int_col => ceil(decimal_literal) > int_col
  6. decimal_literal >= int_col => floor(decimal_literal) >= int_col
  7. decimal_literal < int_col => floor(decimal_literal) < int_col
  8. decimal_literal <= int_col => ceil(decimal_literal) <= int_col

viirya
viirya reviewed Jan 20, 2016
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala Outdated
Copy link
Member Author

@viirya viirya Jan 20, 2016

Choose a reason for hiding this comment

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

We add this rule in analyser instead of optimizer because DecimalPrecision and ImplicitTypeCasts rules in analyser will add additional Cast operator to the integer expression.

@SparkQA
Copy link

SparkQA commented Jan 20, 2016

Test build #49772 has finished for PR 10845 at commit 5a96018.

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

@SparkQA
Copy link

SparkQA commented Jan 21, 2016

Test build #49863 has finished for PR 10845 at commit 7202c54.

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

rxin
rxin reviewed Jan 22, 2016
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala Outdated
Copy link
Contributor

@rxin rxin Jan 22, 2016

Choose a reason for hiding this comment

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

this should apply for any integral type, not just integer. Also can you make this function shorter by breaking it into two functions? It is pretty long now.

@SparkQA
Copy link

SparkQA commented Jan 22, 2016

Test build #49902 has finished for PR 10845 at commit a1271fe.

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

@SparkQA
Copy link

SparkQA commented Jan 22, 2016

Test build #49906 has finished for PR 10845 at commit 1de0616.

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

@viirya
Copy link
Member Author

viirya commented Jan 22, 2016

retest this please.

rxin
rxin reviewed Jan 22, 2016
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala Outdated
Copy link
Contributor

@rxin rxin Jan 22, 2016

Choose a reason for hiding this comment

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

i don't think you can just do a toInt here .... decimal scope is larger than int.

@rxin
Copy link
Contributor

rxin commented Jan 22, 2016

@viirya rather than me pointing out all the problem in each iteration, can you try figuring out all the corner cases yourself and then submit this for review? Thanks.

@SparkQA
Copy link

SparkQA commented Jan 22, 2016

Test build #49915 has finished for PR 10845 at commit 1de0616.

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

@viirya viirya changed the title [SPARK-12904][SQL] Strength reduction for integer/decimal comparisons [SPARK-12904][SQL] Strength reduction for integral/decimal comparisons Jan 22, 2016
@viirya
Copy link
Member Author

viirya commented Jan 22, 2016

@rxin Sure.

@SparkQA
Copy link

SparkQA commented Jan 22, 2016

Test build #49918 has finished for PR 10845 at commit dc05a08.

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

@rxin rxin mentioned this pull request Jan 23, 2016
Closed
@viirya viirya closed this Jan 23, 2016
@viirya viirya deleted the int-decimal-compare branch December 27, 2023 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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

@viirya @SparkQA @rxin