Skip to content

[SPARK-8658] [SQL] AttributeReference's equals method compares all the members #9216

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
gatorsmile wants to merge 6 commits into from
Closed

[SPARK-8658] [SQL] AttributeReference's equals method compares all the members #9216

gatorsmile wants to merge 6 commits into from

Conversation

@gatorsmile
Copy link
Member

@gatorsmile gatorsmile commented Oct 22, 2015

This fix is to change the equals method to check all of the specified fields for equality of AttributeReference.

@JoshRosen
Copy link
Contributor

JoshRosen commented Oct 22, 2015

Can you add a regression test?

@JoshRosen
Copy link
Contributor

JoshRosen commented Oct 22, 2015

Actually, maybe we don't need one.

@JoshRosen
Copy link
Contributor

JoshRosen commented Oct 22, 2015

Jenkins, this is ok to test.

@SparkQA
Copy link

SparkQA commented Oct 22, 2015

Test build #44134 has finished for PR 9216 at commit 029b5ba.

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

@gatorsmile
Copy link
Member Author

gatorsmile commented Oct 23, 2015

My code change exposed a new defect:
Both rollup and cube are not working correctly no matter whether the build includes my changes or not.

Without my changes, in the first ROLLUP test case, both SQL query results or Dataframe results return the following result:
[3,2,-1]
[3,null,null]
[6,4,-2]
[6,null,null]
[null,null,null]

However, the expected results should be like
[3,2,-1]
[3,null,-1]
[6,4,-2]
[6,null,-2]
[null,null,-3]

I need more time to find the root cause why rollup and cube do not work. The current test cases in HiveDataFrameAnalyticsSuite hide the errors because the results of both SQL and DataFrame are wrong, although the results are the same.

Thanks,

Xiao Li

@cloud-fan
Copy link
Contributor

cloud-fan commented Oct 23, 2015

Hi @gatorsmile , thanks for working on it!
Do you mind opening a new JIRA to describe the rollup and cube bug you discovered? I think we should open another PR to fix that bug.

@gatorsmile
Copy link
Member Author

gatorsmile commented Oct 23, 2015

Hi, @cloud-fan

Sure. Will do. I am trying to see if I can easily fix it. Anyway, I will open a JIRA tonight.

Thanks,

Xiao Li

@gatorsmile
Copy link
Member Author

gatorsmile commented Oct 23, 2015

The JIRA is opened:
https://issues.apache.org/jira/browse/SPARK-11275
I will continue the investigation on this JIRA issue.

@gatorsmile
Copy link
Member Author

gatorsmile commented Nov 3, 2015

@JoshRosen @cloud-fan
I submitted a pull request for JIRA Spark-11275: #9419
Hopefully, after finishing the problem, this one can pass all the tests. Thanks!

This was referenced Nov 7, 2015
Closed
Closed
@SparkQA
Copy link

SparkQA commented Nov 14, 2015

Test build #45901 has finished for PR 9216 at commit 4425dbc.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Nov 14, 2015

Test build #45919 has finished for PR 9216 at commit 8e72b17.

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

@SparkQA
Copy link

SparkQA commented Nov 14, 2015

Test build #45932 has finished for PR 9216 at commit 5decf82.

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

@marmbrus
Copy link
Contributor

marmbrus commented Nov 16, 2015

Thanks, I'm going to merge this to master and 1.6.

asfgit pushed a commit that referenced this pull request Nov 16, 2015
@gatorsmile @marmbrus
[SPARK-8658][SQL] AttributeReference's equals method compares all the…
e042780 
… members

This fix is to change the equals method to check all of the specified fields for equality of AttributeReference.

Author: gatorsmile <[email protected]>

Closes #9216 from gatorsmile/namedExpressEqual.

(cherry picked from commit 75ee12f)
Signed-off-by: Michael Armbrust <[email protected]>
@asfgit asfgit closed this in 75ee12f Nov 16, 2015
cloud-fan
cloud-fan reviewed Nov 17, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala
Copy link
Contributor

@cloud-fan cloud-fan Nov 17, 2015

Choose a reason for hiding this comment

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

@gatorsmile , can you send a follow-up PR to also update the hashCode according to this? thanks!

Copy link
Member Author

@gatorsmile gatorsmile Nov 17, 2015

Choose a reason for hiding this comment

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

sure, will do it tonight.

Copy link
Contributor

@marmbrus marmbrus Nov 17, 2015

Choose a reason for hiding this comment

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

Good catch!

@gatorsmile gatorsmile mentioned this pull request Nov 17, 2015
Closed
asfgit pushed a commit that referenced this pull request Nov 17, 2015
@gatorsmile @marmbrus
[SPARK-8658][SQL][FOLLOW-UP] AttributeReference's equals method compa…
933f025 
…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 <[email protected]>

Closes #9761 from gatorsmile/hashCodeNamedExpression.

(cherry picked from commit 0158ff7)
Signed-off-by: Michael Armbrust <[email protected]>
asfgit pushed a commit that referenced this pull request Nov 17, 2015
@gatorsmile @marmbrus
[SPARK-8658][SQL][FOLLOW-UP] AttributeReference's equals method compa…
0158ff7 
…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 <[email protected]>

Closes #9761 from gatorsmile/hashCodeNamedExpression.
@gatorsmile gatorsmile deleted the namedExpressEqual branch December 5, 2015 18:51
kiszk pushed a commit to kiszk/spark-gpu that referenced this pull request Dec 26, 2015
@gatorsmile @kiszk
[SPARK-8658][SQL][FOLLOW-UP] AttributeReference's equals method compa…
2480086 
…res all the members

Based on the comment of cloud-fan in apache/spark#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 <[email protected]>

Closes #9761 from gatorsmile/hashCodeNamedExpression.
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.

5 participants

@gatorsmile @JoshRosen @SparkQA @cloud-fan @marmbrus