[GitHub] [iceberg] kbendick commented on a change in pull request #4036: Core: SnapshotRef parser and serialization logic

[GitBox <gi...@apache.org>]

kbendick commented on a change in pull request #4036:
URL: https://github.com/apache/iceberg/pull/4036#discussion_r799852098



##########
File path: core/src/main/java/org/apache/iceberg/util/JsonUtil.java
##########
@@ -134,6 +146,20 @@ public static String getStringOrNull(String property, JsonNode node) {
         .build();
   }
 
+  public static void writeIntegerIf(boolean condition, String key, Integer value, JsonGenerator generator)
+      throws IOException {
+    if (condition) {
+      generator.writeNumberField(key, value);
+    }
+  }

Review comment:
       Nit:
   
   When I see the name of this function in the code as it's used currently, it's unclear if the field is written with a null value or simply skipped (it's skipped). But when I see it used in code, like the following I'm unsure if I'm going to get `{ "max-snapshot-age-ms": null }` or `{ }`.
   ```java
   JsonUtil.writeLongIf(ref.maxSnapshotAgeMs() != null, "max-snapshot-age-ms", ref.maxSnapshotAgeMs(), generator);
   ```
   
   Maybe naming it `writeIntegerFieldIf` would help? Having `Field` in the name signifies to me that we won't be writing the field if the condition doesn't evaluate to true.




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