[SPARK-33431][CORE][SQL][DSTREAM] Use Option.contains instead of instanceOfOption == Some(x)#30353
[SPARK-33431][CORE][SQL][DSTREAM] Use Option.contains instead of instanceOfOption == Some(x)#30353LuciferYang wants to merge 2 commits intoapache:masterfrom
Conversation
Seems the before/after examples in the description are the same? Anyway, the changes sound reasonable. The other sub-modules (e.g., mllib) do not have the pattern ( |
maropu
changed the title
| // If no attempt is specified, or there is no attemptId for attempts, return all attempts | ||
| attemptId | ||
| .map { id => app.attempts.filter(_.info.attemptId == Some(id)) } | ||
| .map { id => app.attempts.filter(_.info.attemptId.contains(id)) } |
There was a problem hiding this comment.
Hm, these all look like a performance non-sensitive code path. Is it worthwhile to sweep and fix? I am not sure because it will make more conflicts when reverting, backporting, etc. Option == Some(id) can be sort of more readable in a way.
There was a problem hiding this comment.
mainly to reduce unnecessary object creation, if the readability is affected, it can be left unchanged
There was a problem hiding this comment.
I have the same comment; I don't think it matters much either way. If it's just 14 instances, OK.
|
Kubernetes integration test starting |
The description has been corrected
Other modules also have, I can further append new commits to this pr if necessary |
|
Kubernetes integration test status success |
|
Test build #130993 has finished for PR 30353 at commit
|
LuciferYang
changed the title
LuciferYang
changed the title
LuciferYang
changed the title
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #131062 has finished for PR 30353 at commit
|
5 participants
What changes were proposed in this pull request?
There are some code like
InstanceOfOption == Some(x), for this comparison, we need to wrapxas anotherOptioninstance. In this pr use theOption.containsmethod to simplify the above way and avoid to create newOptioninstance.Before:
After:
Why are the changes needed?
Simplify code and reduce object creation.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Pass the Jenkins or GitHub Action