amogh-jahagirdar commented on code in PR #10861:
URL: https://github.com/apache/iceberg/pull/10861#discussion_r1705919060
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1004,24 +1004,28 @@ private Builder setInitialFormatVersion(int
newFormatVersion) {
return this;
}
- public Builder upgradeFormatVersion(int newFormatVersion) {
+ public Builder upgradeFormatVersion(int targetFormatVersion) {
Review Comment:
Nit: TBH the original `newFormatVersion` name was fine to me, so I don't
think we really need to change that and keep the diff simpler, but I'm not
super opionated.
##########
core/src/test/java/org/apache/iceberg/TestFormatVersions.java:
##########
@@ -28,51 +28,79 @@
public class TestFormatVersions extends TestBase {
@Parameters(name = "formatVersion = {0}")
protected static List<Object> parameters() {
- return Arrays.asList(1);
+ return Arrays.asList(1, 2);
}
@TestTemplate
public void testDefaultFormatVersion() {
Review Comment:
Yeah it's really testing the formatVersion API which we should keep to make
sure behavior stays the same no matter how many version there are!
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1004,24 +1004,28 @@ private Builder setInitialFormatVersion(int
newFormatVersion) {
return this;
}
- public Builder upgradeFormatVersion(int newFormatVersion) {
+ public Builder upgradeFormatVersion(int targetFormatVersion) {
Preconditions.checkArgument(
- newFormatVersion <= SUPPORTED_TABLE_FORMAT_VERSION,
+ targetFormatVersion <= SUPPORTED_TABLE_FORMAT_VERSION,
"Cannot upgrade table to unsupported format version: v%s (supported:
v%s)",
- newFormatVersion,
+ targetFormatVersion,
SUPPORTED_TABLE_FORMAT_VERSION);
Preconditions.checkArgument(
- newFormatVersion >= formatVersion,
+ targetFormatVersion >= formatVersion,
"Cannot downgrade v%s table to v%s",
formatVersion,
- newFormatVersion);
+ targetFormatVersion);
- if (newFormatVersion == formatVersion) {
+ if (targetFormatVersion == formatVersion) {
return this;
}
- this.formatVersion = newFormatVersion;
- changes.add(new MetadataUpdate.UpgradeFormatVersion(newFormatVersion));
+ // register incremental version changes separately to ensure all upgrade
requirements are met
+ for (int version = formatVersion + 1; version <= targetFormatVersion;
version++) {
+ changes.add(new MetadataUpdate.UpgradeFormatVersion(version));
Review Comment:
More fundamental question (which I don't think we really need to answer in
this PR, but maybe just think through) cc @RussellSpitzer @leangjonathan
@rdblue @nastra :
What if a REST server gets a list of upgrades out of order, like "1->3->2".
I know here in the reference implementation that's not happening because of the
loop but some other implementation theoretically could send changes in that
order.
Do we need to change the REST spec to require a certain failure for this
case?
I think ultimately we *do not* need to require that servers have a single
upgrade path.
Servers can handle the upgrade path according to what they think is best
(either by re-ordering those if they want to do the upgrade iteratively or by
just doing the direct upgrade path if they like). This keeps the spec small and
implementations can do whatever intelligent things they like.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
