You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/12/26 06:25:08 UTC

[GitHub] [spark] LuciferYang opened a new pull request, #39215: [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

LuciferYang opened a new pull request, #39215:
URL: https://github.com/apache/spark/pull/39215

   ### What changes were proposed in this pull request?
   This pr explicitly define `Seq` as `collection.Seq` in the `ui` related 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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on a diff in pull request #39215: [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

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39215:
URL: https://github.com/apache/spark/pull/39215#discussion_r1057583961


##########
project/MimaExcludes.scala:
##########
@@ -129,7 +129,16 @@ object MimaExcludes {
     ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.copy"),
     ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.this"),
     ProblemFilters.exclude[MissingTypesProblem]("org.apache.spark.storage.BlockManagerMessages$RegisterBlockManager$"),
-    ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.apply")
+    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"),

Review Comment:
   If this is acceptable, I will make more changes, otherwise I will close this pr
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] srowen commented on a diff in pull request #39215: [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

Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #39215:
URL: https://github.com/apache/spark/pull/39215#discussion_r1057719199


##########
project/MimaExcludes.scala:
##########
@@ -129,7 +129,16 @@ object MimaExcludes {
     ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.copy"),
     ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.this"),
     ProblemFilters.exclude[MissingTypesProblem]("org.apache.spark.storage.BlockManagerMessages$RegisterBlockManager$"),
-    ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.apply")
+    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"),

Review Comment:
   I think those are all internal methods right? people wouldn't call these classes in code



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on a diff in pull request #39215: [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

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39215:
URL: https://github.com/apache/spark/pull/39215#discussion_r1057167898


##########
core/src/main/scala/org/apache/spark/status/api/v1/api.scala:
##########
@@ -461,7 +461,7 @@ class ApplicationEnvironmentInfo private[spark] (
     val systemProperties: Seq[(String, String)],
     val metricsProperties: Seq[(String, String)],
     val classpathEntries: Seq[(String, String)],
-    val resourceProfiles: Seq[ResourceProfileInfo])
+    val resourceProfiles: collection.Seq[ResourceProfileInfo])

Review Comment:
   It's a pity that not all cases can be changed,  because some of them involvepublic api changes, such as `systemProperties`, `metricsProperties` and `classpathEntries`, define them to `collection.Seq` need change the following function:
   
   https://github.com/apache/spark/blob/1af0a510202bdadfbc1ab6d04b47fe01a23f4555/core/src/main/scala/org/apache/spark/util/Utils.scala#L2811-L2814
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on a diff in pull request #39215: [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

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39215:
URL: https://github.com/apache/spark/pull/39215#discussion_r1057572810


##########
project/MimaExcludes.scala:
##########
@@ -129,7 +129,16 @@ object MimaExcludes {
     ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.copy"),
     ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.this"),
     ProblemFilters.exclude[MissingTypesProblem]("org.apache.spark.storage.BlockManagerMessages$RegisterBlockManager$"),
-    ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.apply")
+    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"),

Review Comment:
   Need to add `MimaExcludes` for Scala 2.13, should we close this one



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on a diff in pull request #39215: [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

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39215:
URL: https://github.com/apache/spark/pull/39215#discussion_r1057167898


##########
core/src/main/scala/org/apache/spark/status/api/v1/api.scala:
##########
@@ -461,7 +461,7 @@ class ApplicationEnvironmentInfo private[spark] (
     val systemProperties: Seq[(String, String)],
     val metricsProperties: Seq[(String, String)],
     val classpathEntries: Seq[(String, String)],
-    val resourceProfiles: Seq[ResourceProfileInfo])
+    val resourceProfiles: collection.Seq[ResourceProfileInfo])

Review Comment:
   It's a pity that not all cases can be changed,  because some of them involve public api changes, such as `systemProperties`, `metricsProperties` and `classpathEntries`, define them to `collection.Seq` need change the following function:
   
   https://github.com/apache/spark/blob/1af0a510202bdadfbc1ab6d04b47fe01a23f4555/core/src/main/scala/org/apache/spark/util/Utils.scala#L2811-L2814
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] srowen commented on pull request #39215: [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

Posted by GitBox <gi...@apache.org>.
srowen commented on PR #39215:
URL: https://github.com/apache/spark/pull/39215#issuecomment-1366979394

   Merged to master


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] srowen closed pull request #39215: [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

Posted by GitBox <gi...@apache.org>.
srowen closed pull request #39215: [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
URL: https://github.com/apache/spark/pull/39215


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on a diff in pull request #39215: [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

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39215:
URL: https://github.com/apache/spark/pull/39215#discussion_r1057590714


##########
sql/core/src/main/scala/org/apache/spark/status/protobuf/sql/SQLExecutionUIDataSerializer.scala:
##########
@@ -64,7 +64,7 @@ class SQLExecutionUIDataSerializer extends ProtobufSerDe {
       getOptional(ui.hasCompletionTime, () => new Date(ui.getCompletionTime))
     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))

Review 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



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] codecov-commenter commented on pull request #39215: [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

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #39215:
URL: https://github.com/apache/spark/pull/39215#issuecomment-1365070569

   # [Codecov](https://codecov.io/gh/apache/spark/pull/39215?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#39215](https://codecov.io/gh/apache/spark/pull/39215?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1af0a51) into [master](https://codecov.io/gh/apache/spark/commit/d5865d0c085cc41b39e0d615970bce7da270def6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d5865d0) will **decrease** coverage by `46.75%`.
   > The diff coverage is `52.50%`.
   
   > :exclamation: Current head 1af0a51 differs from pull request most recent head ad179a1. Consider uploading reports for the commit ad179a1 to get more accurate results
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #39215       +/-   ##
   ===========================================
   - Coverage   90.94%   44.18%   -46.76%     
   ===========================================
     Files         342      171      -171     
     Lines       76680    40980    -35700     
     Branches    10462     6478     -3984     
   ===========================================
   - Hits        69733    18106    -51627     
   - Misses       5353    22138    +16785     
   + Partials     1594      736      -858     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests | `44.18% <52.50%> (-46.75%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/spark/pull/39215?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ython/pyspark/sql/connect/proto/expressions\_pb2.py](https://codecov.io/gh/apache/spark/pull/39215/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3B5c3Bhcmsvc3FsL2Nvbm5lY3QvcHJvdG8vZXhwcmVzc2lvbnNfcGIyLnB5) | `52.32% <0.00%> (ø)` | |
   | [python/pyspark/sql/connect/proto/relations\_pb2.py](https://codecov.io/gh/apache/spark/pull/39215/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3B5c3Bhcmsvc3FsL2Nvbm5lY3QvcHJvdG8vcmVsYXRpb25zX3BiMi5weQ==) | `57.72% <0.00%> (ø)` | |
   | [python/pyspark/sql/connect/column.py](https://codecov.io/gh/apache/spark/pull/39215/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3B5c3Bhcmsvc3FsL2Nvbm5lY3QvY29sdW1uLnB5) | `87.63% <91.66%> (-0.01%)` | :arrow_down: |
   | [python/pyspark/sql/connect/dataframe.py](https://codecov.io/gh/apache/spark/pull/39215/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3B5c3Bhcmsvc3FsL2Nvbm5lY3QvZGF0YWZyYW1lLnB5) | `76.93% <100.00%> (+0.11%)` | :arrow_up: |
   | [python/pyspark/sql/connect/expressions.py](https://codecov.io/gh/apache/spark/pull/39215/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3B5c3Bhcmsvc3FsL2Nvbm5lY3QvZXhwcmVzc2lvbnMucHk=) | `89.05% <100.00%> (-0.07%)` | :arrow_down: |
   | [python/pyspark/sql/connect/plan.py](https://codecov.io/gh/apache/spark/pull/39215/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3B5c3Bhcmsvc3FsL2Nvbm5lY3QvcGxhbi5weQ==) | `80.25% <100.00%> (+0.15%)` | :arrow_up: |
   | [python/pyspark/sql/connect/types.py](https://codecov.io/gh/apache/spark/pull/39215/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3B5c3Bhcmsvc3FsL2Nvbm5lY3QvdHlwZXMucHk=) | `92.00% <100.00%> (+0.16%)` | :arrow_up: |
   | [...on/pyspark/sql/tests/connect/test\_connect\_basic.py](https://codecov.io/gh/apache/spark/pull/39215/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3B5c3Bhcmsvc3FsL3Rlc3RzL2Nvbm5lY3QvdGVzdF9jb25uZWN0X2Jhc2ljLnB5) | `99.06% <100.00%> (ø)` | |
   | [...n/pyspark/sql/tests/connect/test\_connect\_column.py](https://codecov.io/gh/apache/spark/pull/39215/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3B5c3Bhcmsvc3FsL3Rlc3RzL2Nvbm5lY3QvdGVzdF9jb25uZWN0X2NvbHVtbi5weQ==) | `97.26% <100.00%> (+0.67%)` | :arrow_up: |
   | [...l/tests/connect/test\_connect\_column\_expressions.py](https://codecov.io/gh/apache/spark/pull/39215/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHl0aG9uL3B5c3Bhcmsvc3FsL3Rlc3RzL2Nvbm5lY3QvdGVzdF9jb25uZWN0X2NvbHVtbl9leHByZXNzaW9ucy5weQ==) | `97.58% <100.00%> (ø)` | |
   | ... and [291 more](https://codecov.io/gh/apache/spark/pull/39215/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] srowen commented on a diff in pull request #39215: [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

Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #39215:
URL: https://github.com/apache/spark/pull/39215#discussion_r1057328506


##########
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLAppStatusListener.scala:
##########
@@ -486,7 +486,7 @@ private class LiveExecutionData(val executionId: Long) extends LiveEntity {
   var details: String = null
   var physicalPlanDescription: String = null
   var modifiedConfigs: Map[String, String] = _
-  var metrics = Seq[SQLPlanMetric]()
+  var metrics = scala.collection.Seq[SQLPlanMetric]()

Review Comment:
   Just `collection.Seq`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on pull request #39215: [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

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #39215:
URL: https://github.com/apache/spark/pull/39215#issuecomment-1367116092

   Thanks @srowen 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] srowen commented on pull request #39215: [WIP][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

Posted by GitBox <gi...@apache.org>.
srowen commented on PR #39215:
URL: https://github.com/apache/spark/pull/39215#issuecomment-1366386151

   Seems OK to me, un-mark it as Draft to let it test


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on a diff in pull request #39215: [WIP][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

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39215:
URL: https://github.com/apache/spark/pull/39215#discussion_r1058059669


##########
project/MimaExcludes.scala:
##########
@@ -129,7 +129,27 @@ object MimaExcludes {
     ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.copy"),
     ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.this"),
     ProblemFilters.exclude[MissingTypesProblem]("org.apache.spark.storage.BlockManagerMessages$RegisterBlockManager$"),
-    ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.apply")
+    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"),

Review Comment:
   Scala 2.12 does not need to add these exclude filters, all changes only involve Scala 2.13. 
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on a diff in pull request #39215: [WIP][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

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39215:
URL: https://github.com/apache/spark/pull/39215#discussion_r1058061532


##########
project/MimaExcludes.scala:
##########
@@ -129,7 +129,16 @@ object MimaExcludes {
     ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.copy"),
     ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.this"),
     ProblemFilters.exclude[MissingTypesProblem]("org.apache.spark.storage.BlockManagerMessages$RegisterBlockManager$"),
-    ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.storage.BlockManagerMessages#RegisterBlockManager.apply")
+    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"),

Review Comment:
   Yes, the mima filters to be added are all internal apis
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on pull request #39215: [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

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #39215:
URL: https://github.com/apache/spark/pull/39215#issuecomment-1366648855

   Local maven test with Scala 2.13, all passed
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] [spark] LuciferYang commented on a diff in pull request #39215: [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

Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #39215:
URL: https://github.com/apache/spark/pull/39215#discussion_r1057406919


##########
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLAppStatusListener.scala:
##########
@@ -486,7 +486,7 @@ private class LiveExecutionData(val executionId: Long) extends LiveEntity {
   var details: String = null
   var physicalPlanDescription: String = null
   var modifiedConfigs: Map[String, String] = _
-  var metrics = Seq[SQLPlanMetric]()
+  var metrics = scala.collection.Seq[SQLPlanMetric]()

Review Comment:
   done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org