yyanyy commented on a change in pull request #2275:
URL: https://github.com/apache/iceberg/pull/2275#discussion_r612771744
##########
File path: core/src/main/java/org/apache/iceberg/SerializableTable.java
##########
@@ -147,6 +147,7 @@ public String location() {
return properties;
}
+ // Note that schema parsed from string does not contain the correct schema
ID.
Review comment:
This is related to the comment I had in
https://github.com/apache/iceberg/pull/2465#discussion_r612661016 that we
didn't persist schema id within `toJson()` which is called when we serialize
the table.
Honestly for this case I think we can go either way; we can use a different
`toJson` implementation when serialize the table since schema at that time
almost guaranteed to be the original schema from table metadata; however since
the only usage of `schemaId` is for time travel queries, and this use case
doesn't need id from the current `schema` itself so adding it isn't necessary,
and not having schema Id is not something we don't expect per [this
note](https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/Schema.java#L44-L45).
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
