rdblue commented on code in PR #16688:
URL: https://github.com/apache/iceberg/pull/16688#discussion_r3463529118
##########
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:
Thanks to @amogh-jahagirdar for pointing out the thread where null/0 were
discussed. I think it came down to [this
comment](https://github.com/apache/iceberg/pull/16688#discussion_r3389037110):
> The enum [`WriterFormatVersion`] hurts here:
>
> 1. Likely call shape is >= 4 checks (gate v4 metadata behavior); enum
forces .id() >= 4 at every site.
> 2. WriterFormatVersion.fromId throws on unknown ids — a v4 reader will
crash deserializing a v5-written leaf. Plain int + >= 4 gate is
forward-compatible.
> 3. PRE_V4 as a constant ages awkwardly when v5 ships.
My replies:
1. Can't we define and use `greaterThan(int)` to handle this? We don't have
to get the ID and compare directly.
2. Similar to 1, `fromId` can return `PRE_V4` or `UNKNOWN` for null
3. I liked the earlier discussion that used "unknown" to solve this issue.
For that we can use either ID 0 or null.
I think either 0 or null would be reasonable. I don't think that these
points lead me to conclude that 0 is better. I think it's okay either way and
that this is probably the smallest concern. I do agree that we should use 4 to
represent v4, 5 for v5, 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]
