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-31912][SQL][TESTS] Normalize all binary comparison expressions #28734

Closed
wants to merge 1 commit into from
Closed

[SPARK-31912][SQL][TESTS] Normalize all binary comparison expressions #28734

wants to merge 1 commit into from

Conversation

wangyum
Copy link
Member

@wangyum wangyum commented Jun 5, 2020

What changes were proposed in this pull request?

This pr normalize all binary comparison expressions when comparing plans.

Why are the changes needed?

Improve test framework, otherwise this test will fail:

  test("SPARK-31912 Normalize all binary comparison expressions") {
    val original = testRelation
      .where('a === 'b && Literal(13) >= 'b).as("x")
    val optimized = testRelation
      .where(IsNotNull('a) && IsNotNull('b) && 'a === 'b && 'b <= 13 && 'a <= 13).as("x")
    comparePlans(Optimize.execute(original.analyze), optimized.analyze)
  }

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manual test.

@SparkQA
Copy link

SparkQA commented Jun 5, 2020

Test build #123557 has finished for PR 28734 at commit dbc4614.

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

@cloud-fan
Copy link
Contributor

do you have a use case for it? why do we intentionally switch the operand order?

@wangyum
Copy link
Member Author

wangyum commented Jun 9, 2020

I encountered this issue when inferring from inequality attributes before, Its order may change:

test("Constraints inferred from inequality attributes: case2") {
val original = testRelation.where('a < 'b && 'b < 'c && 'c < 5)
val optimized = testRelation.where(IsNotNull('a) && IsNotNull('b) && IsNotNull('c)
&& 'a < 'b && 'b < 'c && 'c > 'a && 'a < 5 && 'b < 5 && 'c < 5)
comparePlans(Optimize.execute(original.analyze), optimized.analyze)
}

@cloud-fan
Copy link
Contributor

ok makes sense, LGTM. It's for a new feature, so merging to master should be good enough?

@wangyum wangyum closed this in 78f9043 Jun 12, 2020
@wangyum
Copy link
Member Author

wangyum commented Jun 12, 2020

Merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants