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/11/11 02:57:27 UTC

[GitHub] [spark] LuciferYang opened a new pull request, #38610: [SPARK-41106][SQL] Reduce collection conversion when create AttributeMap

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

   ### What changes were proposed in this pull request?
   This pr aims to reduce collection conversion when create AttributeMap as following ways:
   
   1. Add a new `apply` method to `AttributeMap`
   
   ```
     def apply[A](kvs: Iterable[(Attribute, A)]): AttributeMap[A] = {
       new AttributeMap(kvs.map(kv => (kv._1.exprId, kv)).toMap)
     }
   ```
   
   and use it in applicable scenarios to avoid additional collection conversion.
   
   Although the new `apply` method is more generic, I did not delete the old ones for forward compatibility.
   
   2. For the following 2 scenarios, `leftStats.attributeStats ++ rightStats.attributeStats` is `AttributeMap ++ AttributeMap`, will return a new `AttributeMap`, so this pr remove the redundant collection conversion.
   
   
   https://github.com/apache/spark/blob/7d320d784a2d637fd1a8fd0798da3d2a39b4d7cd/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/JoinEstimation.scala#L86
   
   
   https://github.com/apache/spark/blob/7d320d784a2d637fd1a8fd0798da3d2a39b4d7cd/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statsEstimation/JoinEstimation.scala#L148
   
   3. For the following scenario, `attributePercentiles` is a `Map` and there is a corresponding `apply` method can accept `Map` input, so remove the redundant `toSeq` in this pr
   
   
   https://github.com/apache/spark/blob/7d320d784a2d637fd1a8fd0798da3d2a39b4d7cd/sql/core/src/main/scala/org/apache/spark/sql/execution/command/CommandUtils.scala#L323
   
   
   
   ### Why are the changes needed?
   Minor performance improvements, reducing collection conversion
   
   
   ### Does this PR introduce _any_ user-facing change?
   No
   
   
   ### How was this patch tested?
   
   - Pass GitHub Actions
   - Manual test
   ```
   dev/change-scala-version.sh 2.13
   build/mvn clean install -Phadoop-3 -Phadoop-cloud -Pmesos -Pyarn -Pkinesis-asl -Phive-thriftserver -Pspark-ganglia-lgpl -Pkubernetes -Phive  -Pscala-2.13 -fn
   ```
   
   All Test 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 pull request #38610: [SPARK-41106][SQL] Reduce collection conversion when create AttributeMap

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

   cc @srowen How about 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 pull request #38610: [SPARK-41106][SQL] Reduce collection conversion when create AttributeMap

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

   GA 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 pull request #38610: [SPARK-41106][SQL] Reduce collection conversion when create AttributeMap

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

   Yes, both Scala 2.12 and Scala 2.13 have passed the 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 pull request #38610: [SPARK-41106][SQL] Reduce collection conversion when create AttributeMap

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

   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 #38610: [SPARK-41106][SQL] Reduce collection conversion when create AttributeMap

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

   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 commented on pull request #38610: [SPARK-41106][SQL] Reduce collection conversion when create AttributeMap

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

   Cool, so this has passed on 2.12 and 2.13, seems OK to merge


-- 
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 #38610: [SPARK-41106][SQL] Reduce collection conversion when create AttributeMap

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


##########
sql/catalyst/src/main/scala-2.12/org/apache/spark/sql/catalyst/expressions/AttributeMap.scala:
##########
@@ -31,6 +31,10 @@ object AttributeMap {
     new AttributeMap(kvs.map(kv => (kv._1.exprId, kv)).toMap)
   }
 
+  def apply[A](kvs: Iterable[(Attribute, A)]): AttributeMap[A] = {
+    new AttributeMap(kvs.map(kv => (kv._1.exprId, kv)).toMap)
+  }
+

Review Comment:
   ```
   https://github.com/apache/spark/blob/0cb79a4af27eb226a4f7574aa1c8b54412222583/sql/catalyst/src/main/scala-2.12/org/apache/spark/sql/catalyst/expressions/AttributeMap.scala#L26-L32
   ```
   Can we identify the above two as `@ deprecated` now and remove it at an appropriate time(maybe Spark 4.0?)? The new `apply` method is more general.
   
   



##########
sql/catalyst/src/main/scala-2.12/org/apache/spark/sql/catalyst/expressions/AttributeMap.scala:
##########
@@ -31,6 +31,10 @@ object AttributeMap {
     new AttributeMap(kvs.map(kv => (kv._1.exprId, kv)).toMap)
   }
 
+  def apply[A](kvs: Iterable[(Attribute, A)]): AttributeMap[A] = {
+    new AttributeMap(kvs.map(kv => (kv._1.exprId, kv)).toMap)
+  }
+

Review Comment:
   
   https://github.com/apache/spark/blob/0cb79a4af27eb226a4f7574aa1c8b54412222583/sql/catalyst/src/main/scala-2.12/org/apache/spark/sql/catalyst/expressions/AttributeMap.scala#L26-L32
   
   Can we identify the above two as `@ deprecated` now and remove it at an appropriate time(maybe Spark 4.0?)? The new `apply` method is more general.
   
   



-- 
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 #38610: [SPARK-41106][SQL] Reduce collection conversion when create AttributeMap

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


##########
sql/catalyst/src/main/scala-2.12/org/apache/spark/sql/catalyst/expressions/AttributeMap.scala:
##########
@@ -31,6 +31,10 @@ object AttributeMap {
     new AttributeMap(kvs.map(kv => (kv._1.exprId, kv)).toMap)
   }
 
+  def apply[A](kvs: Iterable[(Attribute, A)]): AttributeMap[A] = {
+    new AttributeMap(kvs.map(kv => (kv._1.exprId, kv)).toMap)
+  }
+

Review Comment:
   
   https://github.com/apache/spark/blob/0cb79a4af27eb226a4f7574aa1c8b54412222583/sql/catalyst/src/main/scala-2.12/org/apache/spark/sql/catalyst/expressions/AttributeMap.scala#L26-L32
   
   Can we identify the above two as `@deprecated` now and remove it at an appropriate time(maybe Spark 4.0?)? The new `apply` method is more general.
   
   



##########
sql/catalyst/src/main/scala-2.12/org/apache/spark/sql/catalyst/expressions/AttributeMap.scala:
##########
@@ -31,6 +31,10 @@ object AttributeMap {
     new AttributeMap(kvs.map(kv => (kv._1.exprId, kv)).toMap)
   }
 
+  def apply[A](kvs: Iterable[(Attribute, A)]): AttributeMap[A] = {
+    new AttributeMap(kvs.map(kv => (kv._1.exprId, kv)).toMap)
+  }
+

Review Comment:
   
   https://github.com/apache/spark/blob/0cb79a4af27eb226a4f7574aa1c8b54412222583/sql/catalyst/src/main/scala-2.12/org/apache/spark/sql/catalyst/expressions/AttributeMap.scala#L26-L32
   
   Can we identify the above two as `@deprecated` now and remove them at an appropriate time(maybe Spark 4.0?)? The new `apply` method is more general.
   
   



-- 
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 #38610: [SPARK-41106][SQL] Reduce collection conversion when create AttributeMap

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


##########
sql/catalyst/src/main/scala-2.12/org/apache/spark/sql/catalyst/expressions/AttributeMap.scala:
##########
@@ -31,6 +31,10 @@ object AttributeMap {
     new AttributeMap(kvs.map(kv => (kv._1.exprId, kv)).toMap)
   }
 
+  def apply[A](kvs: Iterable[(Attribute, A)]): AttributeMap[A] = {
+    new AttributeMap(kvs.map(kv => (kv._1.exprId, kv)).toMap)
+  }
+

Review Comment:
   I don't think it matters either way; we can keep it or not



-- 
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 #38610: [SPARK-41106][SQL] Reduce collection conversion when create AttributeMap

Posted by GitBox <gi...@apache.org>.
srowen closed pull request #38610: [SPARK-41106][SQL] Reduce collection conversion when create AttributeMap
URL: https://github.com/apache/spark/pull/38610


-- 
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