You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "ivoson (via GitHub)" <gi...@apache.org> on 2023/11/02 13:38:09 UTC
[PR] [SPARK-45694][SQL] Clean up deprecated API usage View.force and ScalaNumberProxy.signum [spark]
ivoson opened a new pull request, #43637:
URL: https://github.com/apache/spark/pull/43637
### What changes were proposed in this pull request?
Clean up deprecated API usage:
1. ScalaNumberProxy.signum -> use `sign` instead;
2. View.force -> call `toIndexedSeq` directly which is equivalent;
### Why are the changes needed?
Eliminate compile warnings and no longer use deprecated scala APIs.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Pass GA.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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
Re: [PR] [SPARK-45694][SPARK-45695][SQL] Clean up deprecated API usage `View.force` and `ScalaNumberProxy.signum` [spark]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson commented on PR #43637:
URL: https://github.com/apache/spark/pull/43637#issuecomment-1791823590
> Could you re-trigger the failed test pipeline, @ivoson ?
Hi @dongjoon-hyun done, the test passed now.
--
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
Re: [PR] [SPARK-45694][SPARK-45695][SQL] Clean up deprecated API usage `View.force` and `ScalaNumberProxy.signum` [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang closed pull request #43637: [SPARK-45694][SPARK-45695][SQL] Clean up deprecated API usage `View.force` and `ScalaNumberProxy.signum`
URL: https://github.com/apache/spark/pull/43637
--
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
Re: [PR] [SPARK-45694][SQL] Clean up deprecated API usage View.force and ScalaNumberProxy.signum [spark]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson commented on PR #43637:
URL: https://github.com/apache/spark/pull/43637#issuecomment-1790751126
cc @LuciferYang can you please review this PR? Thanks.
--
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
Re: [PR] [SPARK-45694][SPARK-45695][SQL] Clean up deprecated API usage `View.force` and `ScalaNumberProxy.signum` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43637:
URL: https://github.com/apache/spark/pull/43637#issuecomment-1791604791
Could you re-trigger the failed test pipeline, @ivoson ?
--
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
Re: [PR] [SPARK-45694][SPARK-45695][SQL] Clean up deprecated API usage View.force and ScalaNumberProxy.signum [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43637:
URL: https://github.com/apache/spark/pull/43637#discussion_r1380166945
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -362,10 +362,8 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]]
case s: Seq[_] =>
s.map(mapChild)
case m: Map[_, _] =>
- // `map.mapValues().view.force` return `Map` in Scala 2.12 but return `IndexedSeq` in Scala
- // 2.13, call `toMap` method manually to compatible with Scala 2.12 and Scala 2.13
- // `mapValues` is lazy and we need to force it to materialize
- m.view.mapValues(mapChild).view.force.toMap
+ // `mapValues` is lazy and we need to force it to materialize by invoking `toIndexedSeq`
+ m.view.mapValues(mapChild).view.toIndexedSeq.toMap
Review Comment:
How about just `m.view.mapValues(mapChild).toMap`?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -784,13 +782,12 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]]
arg.asInstanceOf[BaseType].clone()
case Some(arg: TreeNode[_]) if containsChild(arg) =>
Some(arg.asInstanceOf[BaseType].clone())
- // `map.mapValues().view.force` return `Map` in Scala 2.12 but return `IndexedSeq` in Scala
- // 2.13, call `toMap` method manually to compatible with Scala 2.12 and Scala 2.13
+ // `mapValues` is lazy and we need to force it to materialize by invoking `toIndexedSeq`
case m: Map[_, _] => m.view.mapValues {
case arg: TreeNode[_] if containsChild(arg) =>
arg.asInstanceOf[BaseType].clone()
case other => other
- }.view.force.toMap // `mapValues` is lazy and we need to force it to materialize
+ }.view.toIndexedSeq.toMap
Review Comment:
ditto
--
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
Re: [PR] [SPARK-45694][SPARK-45695][SQL] Clean up deprecated API usage `View.force` and `ScalaNumberProxy.signum` [spark]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson commented on PR #43637:
URL: https://github.com/apache/spark/pull/43637#issuecomment-1791823766
> <img alt="image" width="513" src="https://user-images.githubusercontent.com/1475305/280160599-56b53eae-d7fc-4b21-9424-317c5a3daee5.png">
> @ivoson Please update the pr description.
Thanks, @LuciferYang . 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
Re: [PR] [SPARK-45694][SPARK-45695][SQL] Clean up deprecated API usage View.force and ScalaNumberProxy.signum [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43637:
URL: https://github.com/apache/spark/pull/43637#discussion_r1380825014
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -362,10 +362,8 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]]
case s: Seq[_] =>
s.map(mapChild)
case m: Map[_, _] =>
- // `map.mapValues().view.force` return `Map` in Scala 2.12 but return `IndexedSeq` in Scala
- // 2.13, call `toMap` method manually to compatible with Scala 2.12 and Scala 2.13
- // `mapValues` is lazy and we need to force it to materialize
- m.view.mapValues(mapChild).view.force.toMap
+ // `mapValues` is lazy and we need to force it to materialize by converting to Map
Review Comment:
Is this, `we need to force it to materialize`, correct with the new 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
Re: [PR] [SPARK-45694][SPARK-45695][SQL] Clean up deprecated API usage View.force and ScalaNumberProxy.signum [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43637:
URL: https://github.com/apache/spark/pull/43637#discussion_r1380825014
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -362,10 +362,8 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]]
case s: Seq[_] =>
s.map(mapChild)
case m: Map[_, _] =>
- // `map.mapValues().view.force` return `Map` in Scala 2.12 but return `IndexedSeq` in Scala
- // 2.13, call `toMap` method manually to compatible with Scala 2.12 and Scala 2.13
- // `mapValues` is lazy and we need to force it to materialize
- m.view.mapValues(mapChild).view.force.toMap
+ // `mapValues` is lazy and we need to force it to materialize by converting to Map
Review Comment:
Is this, `we need to force it to materialize`, correct with the new 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
Re: [PR] [SPARK-45694][SPARK-45695][SQL] Clean up deprecated API usage `View.force` and `ScalaNumberProxy.signum` [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43637:
URL: https://github.com/apache/spark/pull/43637#issuecomment-1791748313
<img width="513" alt="image" src="https://github.com/apache/spark/assets/1475305/56b53eae-d7fc-4b21-9424-317c5a3daee5">
@ivoson Please update the pr description.
--
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
Re: [PR] [SPARK-45694][SPARK-45695][SQL] Clean up deprecated API usage `View.force` and `ScalaNumberProxy.signum` [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43637:
URL: https://github.com/apache/spark/pull/43637#issuecomment-1791845304
Merged into master for Spark 4.0. Thanks @ivoson and @dongjoon-hyun ~
--
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
Re: [PR] [SPARK-45694][SPARK-45695][SQL] Clean up deprecated API usage View.force and ScalaNumberProxy.signum [spark]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson commented on code in PR #43637:
URL: https://github.com/apache/spark/pull/43637#discussion_r1380412775
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -362,10 +362,8 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]]
case s: Seq[_] =>
s.map(mapChild)
case m: Map[_, _] =>
- // `map.mapValues().view.force` return `Map` in Scala 2.12 but return `IndexedSeq` in Scala
- // 2.13, call `toMap` method manually to compatible with Scala 2.12 and Scala 2.13
- // `mapValues` is lazy and we need to force it to materialize
- m.view.mapValues(mapChild).view.force.toMap
+ // `mapValues` is lazy and we need to force it to materialize by invoking `toIndexedSeq`
+ m.view.mapValues(mapChild).view.toIndexedSeq.toMap
Review Comment:
Yeah, it looks better. Thanks, done.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -784,13 +782,12 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]]
arg.asInstanceOf[BaseType].clone()
case Some(arg: TreeNode[_]) if containsChild(arg) =>
Some(arg.asInstanceOf[BaseType].clone())
- // `map.mapValues().view.force` return `Map` in Scala 2.12 but return `IndexedSeq` in Scala
- // 2.13, call `toMap` method manually to compatible with Scala 2.12 and Scala 2.13
+ // `mapValues` is lazy and we need to force it to materialize by invoking `toIndexedSeq`
case m: Map[_, _] => m.view.mapValues {
case arg: TreeNode[_] if containsChild(arg) =>
arg.asInstanceOf[BaseType].clone()
case other => other
- }.view.force.toMap // `mapValues` is lazy and we need to force it to materialize
+ }.view.toIndexedSeq.toMap
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