Skip to content

[SPARK-33427][SQL][FOLLOWUP] Put key and value into IdentityHashMap sequantially #30459

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

[SPARK-33427][SQL][FOLLOWUP] Put key and value into IdentityHashMap sequantially #30459

wants to merge 3 commits into from

Conversation

@viirya
Copy link
Member

@viirya viirya commented Nov 22, 2020

What changes were proposed in this pull request?

This follow-up fixes an issue when inserting key/value pairs into IdentityHashMap in SubExprEvaluationRuntime.

Why are the changes needed?

The last commits to #30341 follows review comment to use IdentityHashMap. Because we leverage IdentityHashMap to compare keys in reference, we should not convert expression pairs to Scala map before inserting. Scala map compares keys by equality so we will loss keys with different references.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Run benchmark to verify.

@github-actions github-actions bot added the SQL label Nov 22, 2020
@viirya
Copy link
Member Author

viirya commented Nov 22, 2020

cc @dongjoon-hyun @maropu

dongjoon-hyun
dongjoon-hyun reviewed Nov 22, 2020
View reviewed changes
...lyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/SubExprEvaluationRuntime.scala
proxyExpressionCurrentId += 1

proxyMap.putAll(e.map(_ -> proxy).toMap.asJava)
e.map(proxyMap.put(_, proxy))
Copy link
Member

@dongjoon-hyun dongjoon-hyun Nov 22, 2020

Choose a reason for hiding this comment

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

cc @cloud-fan

@SparkQA
Copy link

SparkQA commented Nov 22, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36111/

@SparkQA
Copy link

SparkQA commented Nov 22, 2020

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36111/

@SparkQA
Copy link

SparkQA commented Nov 22, 2020

Test build #131507 has finished for PR 30459 at commit b3c7607.

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

@SparkQA
Copy link

SparkQA commented Nov 23, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36120/

@SparkQA
Copy link

SparkQA commented Nov 23, 2020

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36120/

@viirya
Copy link
Member Author

viirya commented Nov 23, 2020

Thanks @HyukjinKwon

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Nov 23, 2020

Let me merge this in. The last is comment-only.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Nov 23, 2020

Merged to master.

@SparkQA
Copy link

SparkQA commented Nov 23, 2020

Test build #131525 has finished for PR 30459 at commit dbc986f.

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

@SparkQA
Copy link

SparkQA commented Nov 23, 2020

Test build #131516 has finished for PR 30459 at commit cef4b55.

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

@maropu
Copy link
Member

maropu commented Nov 24, 2020

late LGTM

@viirya
Copy link
Member Author

viirya commented Nov 24, 2020 •
edited
Loading

Thanks @maropu

@viirya viirya deleted the SPARK-33427-map branch December 27, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

@dongjoon-hyun dongjoon-hyun dongjoon-hyun left review comments

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@viirya @SparkQA @HyukjinKwon @maropu @dongjoon-hyun