kbendick commented on a change in pull request #4036:
URL: https://github.com/apache/iceberg/pull/4036#discussion_r799865143
##########
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:
Also, if it were `writeIntegerFieldIf` (or `writeLongFieldIf`), then the
code that uses it would read more clearly when using name constants for field
names.
For me, the constant `MAX_REG_AGE_MS` wasn't clearly a field name when
reading the below line of code, which would change if the name were changed.
Consider:
```java
JsonUtil.writeLongIf(ref.maxRefAgeMs() != null, MAX_REF_AGE_MS,
ref.maxRefAgeMs(), generator);
```
vs:
```java
JsonUtil.writeLongFieldIf(ref.maxRefAgeMs() != null, MAX_REF_AGE_MS,
ref.maxRefAgeMs(), generator);
```
Another name I like to avoid the potential ambiguity about whether a field
with `null` value is written or not would be `writeIntegerFieldOnlyIf`. The
usage of `OnlyIf` to me makes it more clear that we're not writing the field at
all (not even the key) if the condition doesn't pass. But that might just be
how my brain works. 😅
Again, not something I'd block on at all and just a nit. 😄
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
