Skip to content

[SPARK-28306][SQL][FOLLOWUP] Fix NormalizeFloatingNumbers rule idempotence for equi-join with <=> predicates#25126

Closed
yeshengm wants to merge 4 commits into
apache:masterfrom
yeshengm:spark-28306
Closed

[SPARK-28306][SQL][FOLLOWUP] Fix NormalizeFloatingNumbers rule idempotence for equi-join with <=> predicates#25126
yeshengm wants to merge 4 commits into
apache:masterfrom
yeshengm:spark-28306

Conversation

@yeshengm

@yeshengm yeshengm commented Jul 11, 2019

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Idempotence of the NormalizeFloatingNumbers rule was broken due to the implementation of ExtractEquiJoinKeys. There is no reason that we don't remove EqualNullSafe join keys from an equi-join's otherPredicates.

How was this patch tested?

A new UT.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just replace it by Equality?

@dongjoon-hyun

dongjoon-hyun commented Jul 12, 2019

Copy link
Copy Markdown
Member

@yeshengm . Please use a proper PR title according to the PR content.
It's a bad practice to use the same title in the follow-up PRs.

@yeshengm yeshengm changed the title [SPARK-28306][SQL][FOLLOWUP] Make NormalizeFloatingNumbers rule idempotent [SPARK-28306][SQL][FOLLOWUP] Fix NormalizeFloatingNumbers rule idempotence for equi-join with <=> predicates Jul 12, 2019
@SparkQA

SparkQA commented Jul 12, 2019

Copy link
Copy Markdown

Test build #107559 has finished for PR 25126 at commit 299a7b0.

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

@SparkQA

SparkQA commented Jul 12, 2019

Copy link
Copy Markdown

Test build #107567 has finished for PR 25126 at commit b7ff960.

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

@dongjoon-hyun

Copy link
Copy Markdown
Member

Could you fix the following UT failure, @yeshengm ?

  • org.apache.spark.sql.catalyst.optimizer.NormalizeFloatingPointNumbersSuite.normalize floating points in join keys (equal null safe) - idempotence

@SparkQA

SparkQA commented Jul 12, 2019

Copy link
Copy Markdown

Test build #107610 has finished for PR 25126 at commit b1a6c0f.

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

@SparkQA

SparkQA commented Jul 12, 2019

Copy link
Copy Markdown

Test build #107614 has finished for PR 25126 at commit 40cbad1.

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

@gatorsmile gatorsmile left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Thanks! Merged to master.

vinodkc pushed a commit to vinodkc/spark that referenced this pull request Jul 18, 2019
…tence for equi-join with `<=>` predicates

## What changes were proposed in this pull request?
Idempotence of the `NormalizeFloatingNumbers` rule was broken due to the implementation of `ExtractEquiJoinKeys`. There is no reason that we don't remove `EqualNullSafe` join keys from an equi-join's `otherPredicates`.

## How was this patch tested?
A new UT.

Closes apache#25126 from yeshengm/spark-28306.

Authored-by: Yesheng Ma <kimi.ysma@gmail.com>
Signed-off-by: gatorsmile <gatorsmile@gmail.com>
j-baker pushed a commit to palantir/spark that referenced this pull request Jan 28, 2020
…tence for equi-join with `<=>` predicates

## What changes were proposed in this pull request?
Idempotence of the `NormalizeFloatingNumbers` rule was broken due to the implementation of `ExtractEquiJoinKeys`. There is no reason that we don't remove `EqualNullSafe` join keys from an equi-join's `otherPredicates`.

## How was this patch tested?
A new UT.

Closes apache#25126 from yeshengm/spark-28306.

Authored-by: Yesheng Ma <kimi.ysma@gmail.com>
Signed-off-by: gatorsmile <gatorsmile@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants