You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "aokolnychyi (via GitHub)" <gi...@apache.org> on 2023/05/18 22:45:11 UTC

[GitHub] [iceberg] aokolnychyi opened a new pull request, #7650: Spark 3.4: Cosmetic updates for SparkPositionDeltaWrite

aokolnychyi opened a new pull request, #7650:
URL: https://github.com/apache/iceberg/pull/7650

   This PR contains minor cosmetic updates for `SparkPositionDeltaWrite`.


-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] szehon-ho commented on a diff in pull request #7650: Spark 3.4: Cosmetic updates for SparkPositionDeltaWrite

Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #7650:
URL: https://github.com/apache/iceberg/pull/7650#discussion_r1198385588


##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/source/SparkPositionDeltaWrite.java:
##########
@@ -386,10 +384,14 @@ protected InternalRowWrapper initPartitionRowWrapper(Types.StructType partitionT
     protected Map<Integer, StructProjection> buildPartitionProjections(
         Types.StructType partitionType, Map<Integer, PartitionSpec> specs) {
       Map<Integer, StructProjection> partitionProjections = Maps.newHashMap();
-      specs.forEach(
-          (specID, spec) ->
-              partitionProjections.put(
-                  specID, StructProjection.create(partitionType, spec.partitionType())));
+
+      for (Map.Entry<Integer, PartitionSpec> entry : specs.entrySet()) {

Review Comment:
   Nit: could we save some code by streaming over keySet()?  
   
   for (int specId : specs.keyset()) {
      Partition spec = specs.get(specId);
      ...
   }
   
   Itd be nice to do a values transform, but guess there's no clean way here



-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] aokolnychyi commented on pull request #7650: Spark 3.4: Cosmetic updates for SparkPositionDeltaWrite

Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #7650:
URL: https://github.com/apache/iceberg/pull/7650#issuecomment-1553751415

   cc @szehon-ho, one more small 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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] szehon-ho commented on a diff in pull request #7650: Spark 3.4: Cosmetic updates for SparkPositionDeltaWrite

Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #7650:
URL: https://github.com/apache/iceberg/pull/7650#discussion_r1198385588


##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/source/SparkPositionDeltaWrite.java:
##########
@@ -386,10 +384,14 @@ protected InternalRowWrapper initPartitionRowWrapper(Types.StructType partitionT
     protected Map<Integer, StructProjection> buildPartitionProjections(
         Types.StructType partitionType, Map<Integer, PartitionSpec> specs) {
       Map<Integer, StructProjection> partitionProjections = Maps.newHashMap();
-      specs.forEach(
-          (specID, spec) ->
-              partitionProjections.put(
-                  specID, StructProjection.create(partitionType, spec.partitionType())));
+
+      for (Map.Entry<Integer, PartitionSpec> entry : specs.entrySet()) {

Review Comment:
   Nit: could we save a line by streaming over keySet()?  
   
   for (int specId : specs.keyset()) {
      Partition spec = specs.get(specId);
      ...
   }
   
   Itd be nice to do a values transform, but guess there's no clean way here



-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #7650: Spark 3.4: Cosmetic updates for SparkPositionDeltaWrite

Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #7650:
URL: https://github.com/apache/iceberg/pull/7650#discussion_r1198453557


##########
spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/source/SparkPositionDeltaWrite.java:
##########
@@ -386,10 +384,14 @@ protected InternalRowWrapper initPartitionRowWrapper(Types.StructType partitionT
     protected Map<Integer, StructProjection> buildPartitionProjections(
         Types.StructType partitionType, Map<Integer, PartitionSpec> specs) {
       Map<Integer, StructProjection> partitionProjections = Maps.newHashMap();
-      specs.forEach(
-          (specID, spec) ->
-              partitionProjections.put(
-                  specID, StructProjection.create(partitionType, spec.partitionType())));
+
+      for (Map.Entry<Integer, PartitionSpec> entry : specs.entrySet()) {

Review Comment:
   I like it, even though it does an extra lookup. Changed.



-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] aokolnychyi merged pull request #7650: Spark 3.4: Cosmetic updates for SparkPositionDeltaWrite

Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi merged PR #7650:
URL: https://github.com/apache/iceberg/pull/7650


-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org