amogh-jahagirdar commented on code in PR #16688:
URL: https://github.com/apache/iceberg/pull/16688#discussion_r3463526187
##########
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 think I can get behind the rename to `format_version` but for the other
points,
1.) At the time of upgrade we always know that any existing manifests are
pre-v4. Yes we don't know which exact format version but it's not truly
unknown. Any subsequent writes will produce v4 metadata. It feels a bit odd to
persist null in metadata for the pre-v4 cases but in the end either approach
can be hidden behind the API. From a metadata footprint perspective I think 0
and null are effectively the same here.
2.) Yes the intent behind putting in TrackedFile and having it required for
data files is so that we know exactly which versions produced that. This is
largely for consistency of the metadata with leaf manifests (without
sacrificing metadata footprint) and also as a minor point being able to be more
flexible with cases like a small V3 table with a single manifest (or very few
entries) and just moving all those entries to a root manifest without losing
the fidelity that they were produced by V3 to begin with. Or on upgrade just
knowing which files were from the previous version of the table. This would
help with debuggability in case of issues on upgrade where we need to figure
out "hey what are possibly impacted files". The latter point is minor though.
If we're confident that we only need it for manifests I think that's OK but
then we're in an awkward spot for future evolutions of the format where we need
higher fidelity for leaf entries on the format versdion (because we'd have
2 places, and more writer rules etc).
--
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]
