amogh-jahagirdar commented on code in PR #11354:
URL: https://github.com/apache/iceberg/pull/11354#discussion_r1817905519
##########
core/src/main/java/org/apache/iceberg/SnapshotParser.java:
##########
@@ -129,13 +129,12 @@ static Snapshot fromJson(JsonNode node) {
"Cannot parse summary from non-object value: %s",
sNode);
+ operation = JsonUtil.getString(OPERATION, sNode);
ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
Iterator<String> fields = sNode.fieldNames();
while (fields.hasNext()) {
String field = fields.next();
- if (field.equals(OPERATION)) {
- operation = JsonUtil.getString(OPERATION, sNode);
- } else {
+ if (!field.equals(OPERATION)) {
Review Comment:
@rdblue Sorry, not sure I completely follow. If it's been required in the
spec to have an operation defined in the case a summary mapping is defined, an
Iceberg compliant implementation would've always populated this correctly when
writing out the metadata.
Is there a historical reason where we expect some tables not to have this
populated? I'm also not sure if not having the operation would be OK because
validation during commits needs to check against the operation for intermediate
snapshots, so we would be somehow handling "unknown" operation cases (which I
guess the default would be to assume conflict because it's unknown)
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
