Skip to content

[MINOR]Remove unnecessary gets when getting a value from map.#23901

Closed
10110346 wants to merge 1 commit intoapache:masterfrom
10110346:removegetfrommap
Closed

[MINOR]Remove unnecessary gets when getting a value from map.#23901
10110346 wants to merge 1 commit intoapache:masterfrom
10110346:removegetfrommap

Conversation

@10110346
Copy link
Contributor

What changes were proposed in this pull request?

Redundant get when getting a value from Map given a key.

How was this patch tested?

N/A

@HyukjinKwon
Copy link
Member

BTW, how did you find those instances?

@10110346
Copy link
Contributor Author

BTW, how did you find those instances?

Thanks, I found those using IntelliJ IDEA--->Analyze--->Inspect Code....

@HyukjinKwon
Copy link
Member

I haven;t take a super close look but looks fine.

@SparkQA
Copy link

SparkQA commented Feb 27, 2019

Test build #102809 has finished for PR 23901 at commit 77950e8.

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

Copy link
Contributor

@vanzin vanzin left a comment

Choose a reason for hiding this comment

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

Normally I find these changes pretty pointless. There's nothing wrong with the existing code. Unless you're actually making other changes around the code being modified here, I'd just leave things as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of just following what your IDE says, you could actually have simplified this whole block of code.

@SparkQA
Copy link

SparkQA commented Feb 28, 2019

Test build #102845 has finished for PR 23901 at commit 765be91.

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

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Yeah I agree with this kind of cleanup for Spark 3.

Copy link
Member

Choose a reason for hiding this comment

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

You're welcome to fix this to === while here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thanks

@HyukjinKwon
Copy link
Member

I agree with @vanzin's in general to be honest - I don't think it's encouraged to fix them. I am a-okay to fix it in Spark 3 but I would like to avoid to put a lot of efforts on this.

@SparkQA
Copy link

SparkQA commented Mar 1, 2019

Test build #102888 has finished for PR 23901 at commit c837686.

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

@srowen
Copy link
Member

srowen commented Mar 1, 2019

I agree with the general principle of not changing trivial stuff because of merge conflicts. My logic here is: Spark 3 is a good place as back ports from 3 to 2 are pretty rare; we want to be able to touch up the code at some point; generally OK with this from newer contributors.

@srowen
Copy link
Member

srowen commented Mar 1, 2019

Merged to master

@srowen srowen closed this in 02bbe97 Mar 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants