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-8658] [SQL] [FOLLOW-UP] AttributeReference's equals method compares all the members #9761

Closed
wants to merge 2 commits into from

Conversation

gatorsmile
Copy link
Member

Based on the comment of @cloud-fan in #9216, update the AttributeReference's hashCode function by including the hashCode of the other attributes including name, nullable and qualifiers.

Here, I am not 100% sure if we should include name in the hashCode calculation, since the original hashCode calculation does not include it.

@marmbrus @cloud-fan Please review if the changes are good.

@SparkQA
Copy link

SparkQA commented Nov 17, 2015

Test build #46069 has finished for PR 9761 at commit eb63f09.

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

@gatorsmile
Copy link
Member Author

@nongli I saw you have a related discussion with @chenghao-intel . The failed test case was introduced in your PR #9480. I am not sure the root reason why we intentionally exclude name from hashCode, but the original equals includes name. It breaks a general principle of hashCode function design:

An object’s hashCode method must take the same fields into account as its equals method. 

Based on my understanding, in a case-insensitive HiveContext, we still should detect their differences when the case of name is different but the exprId values are the same

@nongli
Copy link
Contributor

nongli commented Nov 17, 2015

This change looks good to me. The failing test is not valid anymore and should be changed to verify that the hash codes are not equal.

@gatorsmile
Copy link
Member Author

Ok. I will also add three more lines for covering the new hashCode and equals functions.

@SparkQA
Copy link

SparkQA commented Nov 17, 2015

Test build #46095 has finished for PR 9761 at commit b2e354a.

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

@marmbrus
Copy link
Contributor

Thanks! Merging to master and 1.6.

asfgit pushed a commit that referenced this pull request Nov 17, 2015
…res all the members

Based on the comment of cloud-fan in #9216, update the AttributeReference's hashCode function by including the hashCode of the other attributes including name, nullable and qualifiers.

Here, I am not 100% sure if we should include name in the hashCode calculation, since the original hashCode calculation does not include it.

marmbrus cloud-fan Please review if the changes are good.

Author: gatorsmile <gatorsmile@gmail.com>

Closes #9761 from gatorsmile/hashCodeNamedExpression.

(cherry picked from commit 0158ff7)
Signed-off-by: Michael Armbrust <michael@databricks.com>
@asfgit asfgit closed this in 0158ff7 Nov 17, 2015
@gatorsmile gatorsmile deleted the hashCodeNamedExpression branch November 18, 2015 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants