[GitHub] [iceberg] nastra commented on a diff in pull request #7417: Views: Fix SQL view representation field name

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

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