rdblue commented on code in PR #8147:
URL: https://github.com/apache/iceberg/pull/8147#discussion_r1314292820
##########
core/src/main/java/org/apache/iceberg/view/ViewMetadata.java:
##########
@@ -91,41 +111,325 @@ default ViewMetadata checkAndNormalize() {
"Unsupported format version: %s",
formatVersion());
- Preconditions.checkArgument(versions().size() > 0, "Invalid view versions:
empty");
- Preconditions.checkArgument(history().size() > 0, "Invalid view history:
empty");
- Preconditions.checkArgument(schemas().size() > 0, "Invalid schemas:
empty");
+ return this;
+ }
- Preconditions.checkArgument(
- versionsById().containsKey(currentVersionId()),
- "Cannot find current version %s in view versions: %s",
- currentVersionId(),
- versionsById().keySet());
+ static Builder builder() {
+ return new Builder();
+ }
- Preconditions.checkArgument(
- schemasById().containsKey(currentSchemaId()),
- "Cannot find current schema with id %s in schemas: %s",
- currentSchemaId(),
- schemasById().keySet());
+ static Builder buildFrom(ViewMetadata base) {
+ return new Builder(base);
+ }
+
+ class Builder {
+ private static final int LAST_ADDED = -1;
+ private int formatVersion = DEFAULT_VIEW_FORMAT_VERSION;
+ private String location;
+ private List<Schema> schemas = Lists.newArrayList();
+ private int currentVersionId;
+ private Integer lastAddedVersionId;
+ private Integer lastAddedSchemaId;
+ private List<ViewVersion> versions = Lists.newArrayList();
+ private List<ViewHistoryEntry> history = Lists.newArrayList();
+ private Map<String, String> properties = Maps.newHashMap();
+ private List<MetadataUpdate> changes = Lists.newArrayList();
+
+ private Builder() {}
+
+ private Builder(ViewMetadata base) {
+ this.formatVersion = base.formatVersion();
+ this.location = base.location();
+ this.schemas = Lists.newArrayList(base.schemas());
+ this.currentVersionId = base.currentVersionId();
+ this.versions = Lists.newArrayList(base.versions());
+ this.history = Lists.newArrayList(base.history());
+ this.properties = Maps.newHashMap(base.properties());
+ this.changes = Lists.newArrayList(base.changes());
+ }
+
+ public Builder upgradeFormatVersion(int newFormatVersion) {
+ Preconditions.checkArgument(
+ newFormatVersion >= this.formatVersion,
+ "Cannot downgrade v%s view to v%s",
+ formatVersion,
+ newFormatVersion);
+
+ if (this.formatVersion == newFormatVersion) {
+ return this;
+ }
+
+ this.formatVersion = newFormatVersion;
+ this.changes.add(new
MetadataUpdate.UpgradeFormatVersion(newFormatVersion));
+ return this;
+ }
+
+ public Builder setLocation(String newLocation) {
+ Preconditions.checkArgument(null != newLocation, "Invalid location:
null");
+ if (null != this.location && this.location.equals(newLocation)) {
+ return this;
+ }
- int versionHistorySizeToKeep =
- PropertyUtil.propertyAsInt(
- properties(),
- ViewProperties.VERSION_HISTORY_SIZE,
- ViewProperties.VERSION_HISTORY_SIZE_DEFAULT);
+ this.location = newLocation;
+ this.changes.add(new MetadataUpdate.SetLocation(newLocation));
+ return this;
+ }
+
+ public Builder setCurrentVersionId(int newVersionId) {
+ if (newVersionId == -1) {
+ ValidationException.check(
+ lastAddedVersionId != null,
+ "Cannot set last version id: no current version id has been set");
+ return setCurrentVersionId(lastAddedVersionId);
+ }
+
+ if (this.currentVersionId == newVersionId) {
+ return this;
+ }
+
+ checkCurrentVersionIdIsValid(newVersionId);
+
+ // by that time, schemas are defined and the highestFieldId can be
determined and schema
+ // changes can be added
+ int highestFieldId = highestFieldId();
+ for (Schema schema : schemas) {
+ this.changes.add(new MetadataUpdate.AddSchema(schema, highestFieldId));
+ }
+
+ this.currentVersionId = newVersionId;
- if (versionHistorySizeToKeep <= 0) {
- LOG.warn(
- "{} must be positive but was {}",
+ if (lastAddedVersionId != null && lastAddedVersionId == newVersionId) {
+ this.changes.add(new MetadataUpdate.SetCurrentViewVersion(LAST_ADDED));
+ } else {
+ this.changes.add(new
MetadataUpdate.SetCurrentViewVersion(newVersionId));
+ }
+
+ return this;
+ }
+
+ private int highestFieldId() {
+ return
schemas.stream().map(Schema::highestFieldId).max(Integer::compareTo).orElse(0);
+ }
+
+ public Builder setCurrentVersion(ViewVersion version) {
+ Schema schema = indexSchemas(schemas).get(version.schemaId());
+ int newSchemaId = addSchemaInternal(schema);
Review Comment:
This doesn't seem correct to me.
The `ViewVersion` references a schema by ID. This method assumes that schema
has been added to the builder already (or was already added to the view in a
previous commit) because it expects the schema to be present in the ID to
schema map produced by `indexSchemas`. But this then adds the schema and
possibly reassigns the schema ID, which will be a noop because the schema is
already known.
I think the solution is to pass the schema into this method. Then this would
add the schema via `addSchemaInternal` (possibly reassigning its ID), add the
view version that references the schema ID returned by `addSchemaInternal`, and
then call `setCurrentVersionId` with the view version ID returned by
`addVersionInternal`.
--
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]
