leangjonathan commented on code in PR #10861:
URL: https://github.com/apache/iceberg/pull/10861#discussion_r1710087155
##########
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:
The problem we are solving involves how we maintain different upgrade paths.
This stepwise internal upgrade is for future proofing version requirements.
@amogh-jahagirdar @RussellSpitzer correct me if I'm wrong.
In the logic for upgrade requirements we currently have the following
pattern:
```
public static List<UpdateRequirement> forUpdateTable(
TableMetadata base, List<MetadataUpdate> metadataUpdates) {
Preconditions.checkArgument(null != base, "Invalid table metadata:
null");
Preconditions.checkArgument(null != metadataUpdates, "Invalid metadata
updates: null");
Builder builder = new Builder(base, false);
builder.require(new UpdateRequirement.AssertTableUUID(base.uuid()));
metadataUpdates.forEach(builder::update);
return builder.build();
}
```
Where `metadataUpdates.forEach(builder::update);` will register
`UpdateRequirements` based on the specific metadata update into a list for
later processing.
Using the 1, 2, 3 example...
Let's say that V2 required some field XYZ to be populated and we register
that as an `UpdateRequirement` implementation.
```
class AssertXYZPopulated implements UpdateRequirement {
...
}
```
And we also fill in the appropriate builder logic to link
`MetadataUpdate.UpgradeFormatVersion(2)` to `AssertXYZPopulated`
For V1->V2 upgrades the original logic worked since we would have a
`MetadataUpdate.UpgradeFormatVersion(2)` instance registered and can thus
enforce `AssertXYZPopulated`.
However, for V1->V3 upgrades, the original logic would have only had
`MetadataUpdate.UpgradeFormatVersion(3)` (which does not have the mapping to
`AssertXYZPopulated`) and we would fail to enforce the V2 upgrade requirements.
By adding the stepwise logic we are ensuring future upgrade requirements are
always enforced version by version.
--
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
