nastra commented on code in PR #7417:
URL: https://github.com/apache/iceberg/pull/7417#discussion_r1176139576
##########
core/src/main/java/org/apache/iceberg/view/ViewVersionParser.java:
##########
@@ -80,22 +80,7 @@ static ViewVersion fromJson(JsonNode node) {
long timestamp = JsonUtil.getLong(TIMESTAMP_MS, node);
Map<String, String> summary = JsonUtil.getStringMap(SUMMARY, node);
Preconditions.checkArgument(
- summary != null, "Cannot parse view version with missing required
field: %s", SUMMARY);
-
- String operation = null;
- ImmutableMap.Builder<String, String> versionSummary =
ImmutableMap.builder();
- for (Map.Entry<String, String> summaryEntry : summary.entrySet()) {
- if (summaryEntry.getKey().equals(OPERATION)) {
- operation = summaryEntry.getValue();
- } else {
- versionSummary.put(summaryEntry.getKey(), summaryEntry.getValue());
- }
- }
-
- Preconditions.checkArgument(
- operation != null,
- "Cannot parse view version summary with missing required field: %s",
- OPERATION);
+ summary.containsKey(OPERATION), "Invalid view version summary, missing
%s", OPERATION);
Review Comment:
one issue with this is unfortunately that we're only checking for the
existence of `operation` in the Parser. With Immutables you'd typically do this
on the constructed object itself during initialization using a method annotated
with `@Value.Check` to guarantee a consistent state.
For example, with the current version one could create a `ViewVersion`
without an `operation` in the `summary` map, which would fail with a confusing
error: `java.lang.NullPointerException: operation`.
@rdblue I have opened https://github.com/apache/iceberg/pull/7428 to move
the check to `ViewVersion`
--
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]
