rdblue commented on code in PR #16688:
URL: https://github.com/apache/iceberg/pull/16688#discussion_r3463367642
##########
core/src/main/java/org/apache/iceberg/TrackedFile.java:
##########
@@ -35,6 +35,9 @@ interface TrackedFile {
"content_type",
Types.IntegerType.get(),
"Type of content: 0=DATA, 2=EQUALITY_DELETES, 3=DATA_MANIFEST,
4=DELETE_MANIFEST");
+ Types.NestedField WRITER_FORMAT_VERSION =
+ Types.NestedField.required(
+ 157, "writer_format_version", Types.IntegerType.get(), "Writer
format version");
Review Comment:
I don't think that this field name makes sense. Writers don't have a format
version. The Java implementation can write any format version.
I think this is trying to introduce the format version used to produce a
metadata file, in which case it would be the file's format version. We need to
be specific in these cases so that it is not unclear to anyone implementing the
spec or working with the metadata.
I think that we should rename this to `format_version`. It is the format
version that defines the file's compatibility. Any v4 reader (or newer) can
read the file.
Next, should this be required or should we use `null` to signal that the
value is unknown? Why write a non-null value like 0 if we don't really know the
format version? Using `null` seems to fit "unknown" better, at least to me.
Last, why should this be a part of `TrackedFile` rather than `ManifestInfo`?
Is this required for data or delete files? If it is required, what is the value
of writing it? The simplest solution is to make this only apply to manifests,
in which case it should be located in `ManifestInfo`.
--
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]
