yyanyy commented on a change in pull request #2465:
URL: https://github.com/apache/iceberg/pull/2465#discussion_r616222010
##########
File path: core/src/main/java/org/apache/iceberg/SchemaParser.java
##########
@@ -158,7 +169,7 @@ public static String toJson(Schema schema, boolean pretty) {
if (pretty) {
generator.useDefaultPrettyPrinter();
}
- toJson(schema.asStruct(), generator);
+ toJson(schema.asStruct(), schema.schemaId(), schema.rowIdentifiers(),
generator);
Review comment:
Thanks for the response @rdblue ! Just want to mention an additional
case: in addition to the possibility of writing incorrect ID for a table
schema, writing id here also makes us persisting id for things not related to
the table schema itself, e.g. [manifest
list](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/ManifestListWriter.java#L94-L98)
and [delete
files](https://github.com/apache/iceberg/blob/master/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java#L387-L414).
But if we are fine with writing incorrect ID, we probably will be fine with
writing ID for non-schema structs as well...
I guess essentially we are choosing between 1) simpler code with potential
confusing IDs and 2) code complexities (due to avoid writing incorrect/unuseful
IDs); I personally would prefer the latter but I don't strongly oppose the
former if we think it's easier to reason about in code. Also we might want to
involve people from Hive as part of this discussion since they are using this
method as well to preserve the schema
([example](https://github.com/apache/iceberg/blob/master/mr/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java#L119))
--
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]
