[GitHub] [iceberg] flyrain commented on a diff in pull request #8466: Arrow, Spark 3.4: Support vectorized reads with struct constants

["flyrain (via GitHub)" <gi...@apache.org>]

flyrain commented on code in PR #8466:
URL: https://github.com/apache/iceberg/pull/8466#discussion_r1324937320


##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorHolder.java:
##########
@@ -59,12 +59,17 @@ public VectorHolder(
 
   // Only used for returning dummy holder
   private VectorHolder() {
+    this(null);

Review Comment:
   Another option is to make `icebergField` non-final and `protected`. Then we don't have to change the constructor, we can add it in here. Either works to me though since we got a null-check now.
   ```
      public ConstantVectorHolder(Types.NestedField icebergField, int numRows, T constantValue) {
         this.icebergField = icebergField;
         this.numRows = numRows;
         this.constantValue = constantValue;
       }
   ```



##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorHolder.java:
##########
@@ -59,12 +59,17 @@ public VectorHolder(
 
   // Only used for returning dummy holder
   private VectorHolder() {
+    this(null);
+  }
+
+  // Only used for creating constant holders for fields

Review Comment:
   How about a JavaDoc format like this?
   ```
   /**
    * Constructor for constant holders of fields.
    *
    * @param field the nested field for the holder.
    */
   ```



##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/VectorHolder.java:
##########
@@ -59,12 +59,17 @@ public VectorHolder(
 
   // Only used for returning dummy holder

Review Comment:
   Not directly related to your PR. Maybe, we should reword it and reformat it like this 
   ```
   /**
    * A dummy holder constructor.
    */
   ```



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