[SPARK-41709][CORE][SQL][UI] Explicitly define Seq as collection.Seq to avoid toSeq when create ui objects from protobuf objects for Scala 2.13 - #39215
Closed
LuciferYang wants to merge 14 commits into
Conversation
LuciferYang
changed the title
Seq as collection.Seq to avoid toSeq when create ui objects from protobuf objects for Scala 2.13Seq as collection.Seq to avoid toSeq when create ui objects from protobuf objects for Scala 2.13
LuciferYang
changed the title
Seq as collection.Seq to avoid toSeq when create ui objects from protobuf objects for Scala 2.13Seq as collection.Seq to avoid toSeq when create ui objects from protobuf objects for Scala 2.13
LuciferYang
commented
Dec 26, 2022
Codecov Report
@@ Coverage Diff @@
## master #39215 +/- ##
=======================================
Coverage 90.95% 90.95%
=======================================
Files 342 342
Lines 76706 76706
Branches 10460 10460
=======================================
Hits 69765 69765
Misses 5352 5352
Partials 1589 1589
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
LuciferYang
marked this pull request as draft
December 26, 2022 10:46
srowen
reviewed
Dec 26, 2022
| var physicalPlanDescription: String = null | ||
| var modifiedConfigs: Map[String, String] = _ | ||
| var metrics = Seq[SQLPlanMetric]() | ||
| var metrics = scala.collection.Seq[SQLPlanMetric]() |
LuciferYang
marked this pull request as ready for review
December 27, 2022 02:24
LuciferYang
marked this pull request as draft
December 27, 2022 09:46
LuciferYang
commented
Dec 27, 2022
| ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.apply"), | ||
|
|
||
| // [SPARK-41709][CORE][SQL][UI] Explicitly define Seq as collection.Seq to avoid toSeq when create ui objects from protobuf objects for Scala 2.13 | ||
| ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.status.api.v1.ApplicationEnvironmentInfo.resourceProfiles"), |
Contributor
Author
There was a problem hiding this comment.
Need to add MimaExcludes for Scala 2.13, should we close this one
Contributor
Author
There was a problem hiding this comment.
If this is acceptable, I will make more changes, otherwise I will close this pr
Member
There was a problem hiding this comment.
I think those are all internal methods right? people wouldn't call these classes in code
Contributor
Author
There was a problem hiding this comment.
Yes, the mima filters to be added are all internal apis
LuciferYang
commented
Dec 27, 2022
| val errorMessage = getOptional(ui.hasErrorMessage, () => ui.getErrorMessage) | ||
| val metrics = | ||
| ui.getMetricsList.asScala.map(m => SQLPlanMetricSerializer.deserialize(m)).toSeq | ||
| ui.getMetricsList.asScala.map(m => SQLPlanMetricSerializer.deserialize(m)) |
Contributor
Author
There was a problem hiding this comment.
If no toSeq, one operation is about 10ns. After adding toSeq, the delay will increase linearly with the length of the original data:
- 240ns when input length is 10
- 1740ns when input length is 100
- 16600ns when input length is 1000
LuciferYang
changed the title
Seq as collection.Seq to avoid toSeq when create ui objects from protobuf objects for Scala 2.13Seq as collection.Seq to avoid toSeq when create ui objects from protobuf objects for Scala 2.13
LuciferYang
commented
Dec 28, 2022
| ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.apply"), | ||
|
|
||
| // [SPARK-41709][CORE][SQL][UI] Explicitly define Seq as collection.Seq to avoid toSeq when create ui objects from protobuf objects for Scala 2.13 | ||
| ProblemFilters.exclude[IncompatibleResultTypeProblem]("org.apache.spark.status.api.v1.ApplicationEnvironmentInfo.sparkProperties"), |
Contributor
Author
There was a problem hiding this comment.
Scala 2.12 does not need to add these exclude filters, all changes only involve Scala 2.13.
Member
|
Seems OK to me, un-mark it as Draft to let it test |
LuciferYang
marked this pull request as ready for review
December 28, 2022 05:50
LuciferYang
changed the title
Seq as collection.Seq to avoid toSeq when create ui objects from protobuf objects for Scala 2.13Seq as collection.Seq to avoid toSeq when create ui objects from protobuf objects for Scala 2.13
Contributor
Author
|
Local maven test with Scala 2.13, all passed |
Member
|
Merged to master |
Contributor
Author
|
Thanks @srowen |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This pr explicitly define
Seqascollection.Seqin theuirelated class definitions to avoid collection conversion when creating ui objects from protobuf objects to make no performance difference between Scala 2.13 and Scala 2.12.Why are the changes needed?
Avoid collection conversion when creating ui objects from protobuf objects for Scala 2.13.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Pass GitHub Actions