You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/08/11 05:43:04 UTC
[GitHub] [iceberg] jackye1995 opened a new pull request #2957: Core: introduce TableMetadataBuilder
jackye1995 opened a new pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957
continuation of #2887, as discussed with @rdblue, introduce a table metadata builder to cleanup the code.
I am separating the builder to a different class to avoid making the `TableMetadata` class too long. I had to add a few package-private getters but I think that's a reasonable trade off.
If we agree on this implementation, I will also update tests to use the builder instead of the `TableMetadata` constructor.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #2957: Core: introduce TableMetadataBuilder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#discussion_r686513009
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -835,49 +856,41 @@ public TableMetadata buildReplacement(Schema updatedSchema, PartitionSpec update
schemasBuilder.add(new Schema(freshSchemaId, freshSchema.columns(), freshSchema.identifierFieldIds()));
}
- TableMetadata metadata = new TableMetadata(null, formatVersion, uuid, newLocation,
- lastSequenceNumber, System.currentTimeMillis(), newLastColumnId.get(), freshSchemaId, schemasBuilder.build(),
- specId, specListBuilder.build(), Math.max(lastAssignedPartitionId, newSpec.lastAssignedFieldId()),
- orderId, sortOrdersBuilder.build(), ImmutableMap.copyOf(newProperties),
- -1, snapshots, ImmutableList.of(), addPreviousFile(file, lastUpdatedMillis, newProperties));
-
- if (formatVersion != newFormatVersion) {
- metadata = metadata.upgradeToFormatVersion(newFormatVersion);
- }
-
- return metadata;
+ return builder().from(this)
+ .refreshLastUpdateMillis()
+ .withFormatVersion(newFormatVersion)
+ .withLocation(newLocation)
+ .withLastColumnId(newLastColumnId.get())
+ .withCurrentSchemaId(freshSchemaId)
+ .withSchemas(schemasBuilder.build())
+ .withDefaultSpecId(specId)
+ .withSpecs(specListBuilder.build())
+ .withLastAssignedPartitionId(Math.max(lastAssignedPartitionId, newSpec.lastAssignedFieldId()))
+ .withDefaultSortOrderId(orderId)
+ .withSortOrders(sortOrdersBuilder.build())
+ .withProperties(ImmutableMap.copyOf(newProperties))
+ .withCurrentSnapshotId(INITIAL_SNAPSHOT_ID)
+ .withSnapshotLog(ImmutableList.of())
+ .build();
}
public TableMetadata updateLocation(String newLocation) {
- return new TableMetadata(null, formatVersion, uuid, newLocation,
- lastSequenceNumber, System.currentTimeMillis(), lastColumnId, currentSchemaId, schemas, defaultSpecId, specs,
- lastAssignedPartitionId, defaultSortOrderId, sortOrders, properties, currentSnapshotId,
- snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
+ return builder().from(this)
+ .refreshLastUpdateMillis()
+ .withLocation(newLocation)
+ .build();
}
public TableMetadata upgradeToFormatVersion(int newFormatVersion) {
- Preconditions.checkArgument(newFormatVersion <= SUPPORTED_TABLE_FORMAT_VERSION,
Review comment:
this logic here is moved to the builder. What we are doing here is simple precondition checks and then directly run the constructor logic with the new format version, so the logic in the original PR to call upgradeToFormatVersion is unnecessary, and can be centralized to the build logic.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 closed pull request #2957: Core: add TableMetadata update builder
Posted by GitBox <gi...@apache.org>.
jackye1995 closed pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #2957: Core: add TableMetadata update builder
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#discussion_r709824337
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -475,10 +488,7 @@ public TableMetadata withUUID() {
if (uuid != null) {
return this;
} else {
- return new TableMetadata(null, formatVersion, UUID.randomUUID().toString(), location,
- lastSequenceNumber, lastUpdatedMillis, lastColumnId, currentSchemaId, schemas, defaultSpecId, specs,
- lastAssignedPartitionId, defaultSortOrderId, sortOrders, properties,
- currentSnapshotId, snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
+ return builderFrom(this).generateUUID().build();
Review comment:
should this also have a `.withFile(null)`?
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -500,11 +510,14 @@ public TableMetadata updateSchema(Schema newSchema, int newLastColumnId) {
builder.add(new Schema(newSchemaId, newSchema.columns(), newSchema.identifierFieldIds()));
}
- return new TableMetadata(null, formatVersion, uuid, location,
- lastSequenceNumber, System.currentTimeMillis(), newLastColumnId,
- newSchemaId, builder.build(), defaultSpecId, updatedSpecs, lastAssignedPartitionId,
- defaultSortOrderId, updatedSortOrders, properties, currentSnapshotId, snapshots, snapshotLog,
- addPreviousFile(file, lastUpdatedMillis));
+ return builderFrom(this)
Review comment:
should this have a `.withFile(null)` and `.withPreviousFiles(addPreviousFile(file, lastUpdatedMillis))`?
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -676,26 +694,22 @@ private TableMetadata setCurrentSnapshotTo(Snapshot snapshot) {
.add(new SnapshotLogEntry(nowMillis, snapshot.snapshotId()))
.build();
- return new TableMetadata(null, formatVersion, uuid, location,
- lastSequenceNumber, nowMillis, lastColumnId, currentSchemaId, schemas, defaultSpecId, specs,
- lastAssignedPartitionId, defaultSortOrderId, sortOrders, properties, snapshot.snapshotId(), snapshots,
- newSnapshotLog, addPreviousFile(file, lastUpdatedMillis));
+ return builderFrom(this)
+ .refreshLastUpdateMillis()
Review comment:
does this actually need to be equal to `nowMillis`? because when we reach this part, the millis will be different for the `SnapshotLogEntry` and for `TableMetadata`
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -538,11 +551,12 @@ public TableMetadata updatePartitionSpec(PartitionSpec newPartitionSpec) {
builder.add(freshSpec(newDefaultSpecId, schema, newPartitionSpec));
}
- return new TableMetadata(null, formatVersion, uuid, location,
- lastSequenceNumber, System.currentTimeMillis(), lastColumnId, currentSchemaId, schemas, newDefaultSpecId,
- builder.build(), Math.max(lastAssignedPartitionId, newPartitionSpec.lastAssignedFieldId()),
- defaultSortOrderId, sortOrders, properties,
- currentSnapshotId, snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
+ return builderFrom(this)
+ .refreshLastUpdateMillis()
+ .withDefaultSpecId(newDefaultSpecId)
+ .withSpecs(builder.build())
+ .withLastAssignedPartitionId(Math.max(lastAssignedPartitionId, newPartitionSpec.lastAssignedFieldId()))
+ .build();
Review comment:
I think it's a bit confusing when some stuff is set via builder methods and other stuff is set when `build()` is called (such as it's the case for `previousFiles`). IMO it would be more consistent when everything that needs to be set is set via builder methods. What could be still added to the `build` method is potential validation logic (if necessary)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #2957: Core: add TableMetadata update builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#discussion_r712340319
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -475,10 +488,7 @@ public TableMetadata withUUID() {
if (uuid != null) {
return this;
} else {
- return new TableMetadata(null, formatVersion, UUID.randomUUID().toString(), location,
- lastSequenceNumber, lastUpdatedMillis, lastColumnId, currentSchemaId, schemas, defaultSpecId, specs,
- lastAssignedPartitionId, defaultSortOrderId, sortOrders, properties,
- currentSnapshotId, snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
+ return builderFrom(this).generateUUID().build();
Review comment:
it's null by default, is it worth adding it explicitly?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra edited a comment on pull request #2957: Core: add TableMetadata update builder
Posted by GitBox <gi...@apache.org>.
nastra edited a comment on pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#issuecomment-920635916
@rdblue independently from this PR, have you guys thought about using [Immutable annotations](https://immutables.github.io/) on things that should be immutable, which also gives you `make-a-copy-of-X-and-update-fields-a-b-c` for free?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on pull request #2957: Core: add TableMetadata update builder
Posted by GitBox <gi...@apache.org>.
nastra commented on pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#issuecomment-920635916
@rdblue independently from this PR, have you guys thought about using [Immutable annotations](https://immutables.github.io/) on things that should be immutable, which also gives you `make-a-copy-of-X-and-update--fields-a-b-c` for free?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #2957: Core: add TableMetadata update builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#issuecomment-986143461
close in favor of #3664
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #2957: Core: introduce TableMetadataBuilder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#discussion_r686510864
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataBuilder.java
##########
@@ -0,0 +1,252 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+
+public class TableMetadataBuilder {
+
+ private InputFile file;
+ private Integer formatVersion;
+ private String uuid;
+ private String location;
+ private Long lastSequenceNumber;
+ private Long lastUpdatedMillis;
+ private Integer lastColumnId;
+ private Integer currentSchemaId;
+ private List<Schema> schemas;
+ private Integer defaultSpecId;
+ private List<PartitionSpec> specs;
+ private Integer lastAssignedPartitionId;
+ private Integer defaultSortOrderId;
+ private List<SortOrder> sortOrders;
+ private Map<String, String> properties;
+ private Long currentSnapshotId;
+ private List<Snapshot> snapshots;
+ private List<HistoryEntry> snapshotLog;
+ private List<TableMetadata.MetadataLogEntry> previousFiles;
+
+ private int baseFormatVersion;
+ private InputFile baseFile;
+ private long baseLastUpdateMillis;
+ private List<TableMetadata.MetadataLogEntry> basePreviousFiles;
+
+ TableMetadataBuilder() {
+ }
+
+ public TableMetadataBuilder withFile(InputFile inputFile) {
+ this.file = inputFile;
+ return this;
+ }
+
+ public TableMetadataBuilder withLocation(String inputLocation) {
+ this.location = inputLocation;
+ return this;
+ }
+
+ public TableMetadataBuilder withFormatVersion(int inputFormatVersion) {
+ this.formatVersion = inputFormatVersion;
+ return this;
+ }
+
+ public TableMetadataBuilder withUuid(String inputUuid) {
+ this.uuid = inputUuid;
+ return this;
+ }
+
+ public TableMetadataBuilder generateUUID() {
+ return withUuid(UUID.randomUUID().toString());
+ }
+
+ public TableMetadataBuilder withLastSequenceNumber(long inputLastSequenceNumber) {
+ this.lastSequenceNumber = inputLastSequenceNumber;
+ return this;
+ }
+
+ public TableMetadataBuilder refreshLastUpdateMillis() {
+ return withLastUpdatedMillis(System.currentTimeMillis());
+ }
+
+ public TableMetadataBuilder withLastUpdatedMillis(Long inputLastUpdatedMillis) {
+ this.lastUpdatedMillis = inputLastUpdatedMillis;
+ return this;
+ }
+
+ public TableMetadataBuilder withLastColumnId(int inputLastColumnId) {
+ this.lastColumnId = inputLastColumnId;
+ return this;
+ }
+
+ public TableMetadataBuilder withCurrentSchemaId(int inputCurrentSchemaId) {
+ this.currentSchemaId = inputCurrentSchemaId;
+ return this;
+ }
+
+ public TableMetadataBuilder withSchemas(List<Schema> inputSchemas) {
+ this.schemas = inputSchemas;
+ return this;
+ }
+
+ public TableMetadataBuilder withDefaultSpecId(int inputDefaultSpecId) {
+ this.defaultSpecId = inputDefaultSpecId;
+ return this;
+ }
+
+ public TableMetadataBuilder withSpecs(List<PartitionSpec> inputSpecs) {
+ this.specs = inputSpecs;
+ return this;
+ }
+
+ public TableMetadataBuilder withLastAssignedPartitionId(int inputLastAssignedPartitionId) {
+ this.lastAssignedPartitionId = inputLastAssignedPartitionId;
+ return this;
+ }
+
+ public TableMetadataBuilder withDefaultSortOrderId(int inputDefaultSortOrderId) {
+ this.defaultSortOrderId = inputDefaultSortOrderId;
+ return this;
+ }
+
+ public TableMetadataBuilder withSortOrders(List<SortOrder> inputSortOrders) {
+ this.sortOrders = inputSortOrders;
+ return this;
+ }
+
+ public TableMetadataBuilder withProperties(Map<String, String> inputProperties) {
+ this.properties = inputProperties;
+ return this;
+ }
+
+ public TableMetadataBuilder withCurrentSnapshotId(long inputCurrentSnapshotId) {
+ this.currentSnapshotId = inputCurrentSnapshotId;
+ return this;
+ }
+
+ public TableMetadataBuilder withSnapshots(List<Snapshot> inputSnapshots) {
+ this.snapshots = inputSnapshots;
+ return this;
+ }
+
+ public TableMetadataBuilder withSnapshotLog(List<HistoryEntry> inputSnapshotLog) {
+ this.snapshotLog = inputSnapshotLog;
+ return this;
+ }
+
+ public TableMetadataBuilder withPreviousFiles(List<TableMetadata.MetadataLogEntry> inputPreviousFiles) {
+ this.previousFiles = inputPreviousFiles;
+ return this;
+ }
+
+ public TableMetadataBuilder from(TableMetadata base) {
+ this.formatVersion = base.formatVersion();
+ this.uuid = base.uuid();
+ this.location = base.location();
+ this.lastSequenceNumber = base.lastSequenceNumber();
+ this.lastUpdatedMillis = base.lastUpdatedMillis();
+ this.lastColumnId = base.lastColumnId();
+ this.currentSchemaId = base.currentSchemaId();
+ this.schemas = base.schemas();
+ this.specs = base.specs();
+ this.defaultSpecId = base.defaultSpecId();
+ this.lastAssignedPartitionId = base.lastAssignedPartitionId();
+ this.defaultSortOrderId = base.defaultSortOrderId();
+ this.sortOrders = base.sortOrders();
+ this.properties = base.properties();
+ this.currentSnapshotId = base.currentSnapshotId();
+ this.snapshots = base.snapshots();
+ this.snapshotLog = base.snapshotLog();
+
+ this.baseFormatVersion = base.formatVersion();
+ this.baseFile = base.file();
+ this.baseLastUpdateMillis = base.lastUpdatedMillis();
+ this.basePreviousFiles = base.previousFiles();
+ return this;
+ }
+
+ @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ public TableMetadata build() {
+ if (formatVersion == null) {
+ formatVersion = TableMetadata.DEFAULT_TABLE_FORMAT_VERSION;
+ } else if (formatVersion != baseFormatVersion) {
+ Preconditions.checkArgument(formatVersion <= TableMetadata.SUPPORTED_TABLE_FORMAT_VERSION,
+ "Cannot upgrade table to unsupported format version: v%s (supported: v%s)",
+ formatVersion, TableMetadata.SUPPORTED_TABLE_FORMAT_VERSION);
+ Preconditions.checkArgument(formatVersion >= baseFormatVersion,
+ "Cannot downgrade v%s table to v%s", baseFormatVersion, formatVersion);
+ }
+
+ if (lastSequenceNumber == null) {
+ lastSequenceNumber = TableMetadata.INITIAL_SEQUENCE_NUMBER;
+ }
+
+ if (lastUpdatedMillis == null) {
+ lastUpdatedMillis = System.currentTimeMillis();
+ }
+
+ Preconditions.checkNotNull(lastColumnId, "Last column ID not assigned for table metadata");
+ Preconditions.checkArgument(schemas != null && !schemas.isEmpty(),
+ "Table metadata must have at least 1 schema");
+ Preconditions.checkNotNull(currentSchemaId, "Table metadata must have the current schema ID");
Review comment:
technically we can calculate the schema, spec, partition field and sort order id using a max, but for all use cases I see, we always know the actual values of these IDs and can avoid the recomputation, so I just decide to throw exception if it is not set by the caller.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #2957: Core: add TableMetadata update builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#discussion_r712341536
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -676,26 +694,22 @@ private TableMetadata setCurrentSnapshotTo(Snapshot snapshot) {
.add(new SnapshotLogEntry(nowMillis, snapshot.snapshotId()))
.build();
- return new TableMetadata(null, formatVersion, uuid, location,
- lastSequenceNumber, nowMillis, lastColumnId, currentSchemaId, schemas, defaultSpecId, specs,
- lastAssignedPartitionId, defaultSortOrderId, sortOrders, properties, snapshot.snapshotId(), snapshots,
- newSnapshotLog, addPreviousFile(file, lastUpdatedMillis));
+ return builderFrom(this)
+ .refreshLastUpdateMillis()
Review comment:
good point, will update them to match
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2957: Core: introduce TableMetadataBuilder
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#discussion_r698076432
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -403,9 +403,26 @@ static TableMetadata fromJson(FileIO io, InputFile file, JsonNode node) {
}
}
- return new TableMetadata(file, formatVersion, uuid, location,
- lastSequenceNumber, lastUpdatedMillis, lastAssignedColumnId, currentSchemaId, schemas, defaultSpecId, specs,
- lastAssignedPartitionId, defaultSortOrderId, sortOrders, properties, currentVersionId,
- snapshots, entries.build(), metadataEntries.build());
+ return TableMetadata.builder()
Review comment:
I think the benefit of a builder for this class would be using it to accumulate state as it is read, rather than keeping the local variables.
For example, rather than calling `snapshots.add` and then calling `withSnapshots(snapshots)`, the builder could help by exposing a `addSnapshot` method. But before adding that, I think it's worth considering what the purpose of the builder is, and I don't think that it is to make parsing easier.
To me, the purpose of the builder is to make updating table metadata easier. That's what I initially introduced the high-level methods for, like `TableMetadata.replaceCurrentSnapshot`. I think a builder pattern works better for implementing those methods since you can make multiple changes in the same builder instead of building a new `TableMetadata` and calling a different high-level method on it.
So in the end, I don't think that I would update this just yet like I said in my first comment. And I would probably change the API of the builder in general. If you want to use a builder here, then maybe two builders are appropriate. One for building from existing table metadata and one for just constructing the `TableMetadata` objects.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #2957: Core: add TableMetadata update builder
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#discussion_r712820889
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -475,10 +488,7 @@ public TableMetadata withUUID() {
if (uuid != null) {
return this;
} else {
- return new TableMetadata(null, formatVersion, UUID.randomUUID().toString(), location,
- lastSequenceNumber, lastUpdatedMillis, lastColumnId, currentSchemaId, schemas, defaultSpecId, specs,
- lastAssignedPartitionId, defaultSortOrderId, sortOrders, properties,
- currentSnapshotId, snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
+ return builderFrom(this).generateUUID().build();
Review comment:
you're right, not worth adding it explicitly. I was confused with `baseFile` and `file`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #2957: Core: add TableMetadata update builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#issuecomment-917857005
@rdblue thanks for the review! Based on the conversation, I try to first introduce the builder for update only. To indicate the builder is for update only, I used name `TableMetadataUpdateBuilder` instead of `TableMetadataBuilder`.
I was thinking about using the same builder for both update and new metadata, but there might be some unnecessary complications to mix those 2 together, because we need to set defaults for any null values when building new metadata, whereas we want to inherit whatever was in the base metadata without any defaults in an update. So my current stance is that we can introduce another builder later for new table metadata (when that is necessary), and evaluate if it is worth letting them sharing some common class or not.
For the feature of accumulating states, I am leaving it out of this PR for now. I tried to implement it, but in update builder we need to recreate a list anyway for whatever we need to overwrite from the old metadata, so the benefit for building the list inside of builder is not obvious compared to building it outside and then overwrite the whole list.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2957: Core: introduce TableMetadataBuilder
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#discussion_r698071555
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataParser.java
##########
@@ -403,9 +403,26 @@ static TableMetadata fromJson(FileIO io, InputFile file, JsonNode node) {
}
}
- return new TableMetadata(file, formatVersion, uuid, location,
- lastSequenceNumber, lastUpdatedMillis, lastAssignedColumnId, currentSchemaId, schemas, defaultSpecId, specs,
- lastAssignedPartitionId, defaultSortOrderId, sortOrders, properties, currentVersionId,
- snapshots, entries.build(), metadataEntries.build());
+ return TableMetadata.builder()
Review comment:
What is the benefit of using the builder here instead of instantiating `TableMetadata` directly? Looks like it made the method too long! I don't think that this needs to use the builder because there isn't anything to validate or change.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2957: Core: introduce TableMetadataBuilder
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#discussion_r698071635
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataBuilder.java
##########
@@ -0,0 +1,252 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+
+public class TableMetadataBuilder {
Review comment:
Does this need to be public? I would prefer not making the builder public for now.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #2957: Core: add TableMetadata update builder
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#issuecomment-920477110
I was imagining the builder would work a little differently. There's definitely some value in the regular builder pattern where you can mostly copy fields and just make calls to change values. But I think adding a builder could bring more value if we actually moved the implementations of the `TableMetadata` modifications into the builder. So what I was thinking was to basically move all of the methods like `replaceCurrentSnapshot` and `removeSnapshotsIf` into the builder itself.
That approach allows us to make more than one change without adding special handling for `upgradeToFormatVersion`. We could chain changes to produce a new `TableMetadata` because the methods keep track of table state.
A second benefit that I'm interested in is tracking the high-level changes the led to a new `TableMetadata` object. We're building a REST-based catalog and I think this is the first step to an update that posts just what changed. For example, `replaceCurrentSnapshot` could produce a new `TableMetadata`, but also keep track of the facts that (1) a new snapshot was added and (2) the current snapshot was set to that snapshot's ID. Those granular changes make a REST protocol much more efficient.
Does that motivation make sense?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #2957: Core: add TableMetadata update builder
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#discussion_r712816983
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -500,11 +510,14 @@ public TableMetadata updateSchema(Schema newSchema, int newLastColumnId) {
builder.add(new Schema(newSchemaId, newSchema.columns(), newSchema.identifierFieldIds()));
}
- return new TableMetadata(null, formatVersion, uuid, location,
- lastSequenceNumber, System.currentTimeMillis(), newLastColumnId,
- newSchemaId, builder.build(), defaultSpecId, updatedSpecs, lastAssignedPartitionId,
- defaultSortOrderId, updatedSortOrders, properties, currentSnapshotId, snapshots, snapshotLog,
- addPreviousFile(file, lastUpdatedMillis));
+ return builderFrom(this)
Review comment:
yes I know that this is being added during `build()` but IMO this is rather counter-intuitive as it's not clear what's being set explicitly and what's being set implicitly.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #2957: Core: introduce TableMetadataBuilder
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#discussion_r698075710
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadataBuilder.java
##########
@@ -0,0 +1,252 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+import org.apache.iceberg.io.InputFile;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+
+public class TableMetadataBuilder {
+
+ private InputFile file;
+ private Integer formatVersion;
+ private String uuid;
+ private String location;
+ private Long lastSequenceNumber;
+ private Long lastUpdatedMillis;
+ private Integer lastColumnId;
+ private Integer currentSchemaId;
+ private List<Schema> schemas;
+ private Integer defaultSpecId;
+ private List<PartitionSpec> specs;
+ private Integer lastAssignedPartitionId;
+ private Integer defaultSortOrderId;
+ private List<SortOrder> sortOrders;
+ private Map<String, String> properties;
+ private Long currentSnapshotId;
+ private List<Snapshot> snapshots;
+ private List<HistoryEntry> snapshotLog;
+ private List<TableMetadata.MetadataLogEntry> previousFiles;
+
+ private int baseFormatVersion;
+ private InputFile baseFile;
+ private long baseLastUpdateMillis;
+ private List<TableMetadata.MetadataLogEntry> basePreviousFiles;
+
+ TableMetadataBuilder() {
+ }
+
+ public TableMetadataBuilder withFile(InputFile inputFile) {
+ this.file = inputFile;
+ return this;
+ }
+
+ public TableMetadataBuilder withLocation(String inputLocation) {
+ this.location = inputLocation;
+ return this;
+ }
+
+ public TableMetadataBuilder withFormatVersion(int inputFormatVersion) {
+ this.formatVersion = inputFormatVersion;
+ return this;
+ }
+
+ public TableMetadataBuilder withUUID(String inputUuid) {
+ this.uuid = inputUuid;
+ return this;
+ }
+
+ public TableMetadataBuilder generateUUID() {
+ return withUUID(UUID.randomUUID().toString());
+ }
+
+ public TableMetadataBuilder withLastSequenceNumber(long inputLastSequenceNumber) {
+ this.lastSequenceNumber = inputLastSequenceNumber;
+ return this;
+ }
+
+ public TableMetadataBuilder refreshLastUpdateMillis() {
+ return withLastUpdatedMillis(System.currentTimeMillis());
+ }
+
+ public TableMetadataBuilder withLastUpdatedMillis(Long inputLastUpdatedMillis) {
+ this.lastUpdatedMillis = inputLastUpdatedMillis;
+ return this;
+ }
+
+ public TableMetadataBuilder withLastColumnId(int inputLastColumnId) {
+ this.lastColumnId = inputLastColumnId;
+ return this;
+ }
+
+ public TableMetadataBuilder withCurrentSchemaId(int inputCurrentSchemaId) {
+ this.currentSchemaId = inputCurrentSchemaId;
+ return this;
+ }
+
+ public TableMetadataBuilder withSchemas(List<Schema> inputSchemas) {
+ this.schemas = inputSchemas;
+ return this;
+ }
+
+ public TableMetadataBuilder withDefaultSpecId(int inputDefaultSpecId) {
+ this.defaultSpecId = inputDefaultSpecId;
+ return this;
+ }
+
+ public TableMetadataBuilder withSpecs(List<PartitionSpec> inputSpecs) {
+ this.specs = inputSpecs;
+ return this;
+ }
+
+ public TableMetadataBuilder withLastAssignedPartitionId(int inputLastAssignedPartitionId) {
+ this.lastAssignedPartitionId = inputLastAssignedPartitionId;
+ return this;
+ }
+
+ public TableMetadataBuilder withDefaultSortOrderId(int inputDefaultSortOrderId) {
+ this.defaultSortOrderId = inputDefaultSortOrderId;
+ return this;
+ }
+
+ public TableMetadataBuilder withSortOrders(List<SortOrder> inputSortOrders) {
+ this.sortOrders = inputSortOrders;
+ return this;
+ }
+
+ public TableMetadataBuilder withProperties(Map<String, String> inputProperties) {
+ this.properties = inputProperties;
+ return this;
+ }
+
+ public TableMetadataBuilder withCurrentSnapshotId(long inputCurrentSnapshotId) {
+ this.currentSnapshotId = inputCurrentSnapshotId;
+ return this;
+ }
+
+ public TableMetadataBuilder withSnapshots(List<Snapshot> inputSnapshots) {
+ this.snapshots = inputSnapshots;
+ return this;
+ }
+
+ public TableMetadataBuilder withSnapshotLog(List<HistoryEntry> inputSnapshotLog) {
+ this.snapshotLog = inputSnapshotLog;
+ return this;
+ }
+
+ public TableMetadataBuilder withPreviousFiles(List<TableMetadata.MetadataLogEntry> inputPreviousFiles) {
+ this.previousFiles = inputPreviousFiles;
+ return this;
+ }
+
+ public TableMetadataBuilder from(TableMetadata base) {
+ this.formatVersion = base.formatVersion();
+ this.uuid = base.uuid();
+ this.location = base.location();
+ this.lastSequenceNumber = base.lastSequenceNumber();
+ this.lastUpdatedMillis = base.lastUpdatedMillis();
+ this.lastColumnId = base.lastColumnId();
+ this.currentSchemaId = base.currentSchemaId();
+ this.schemas = base.schemas();
+ this.specs = base.specs();
+ this.defaultSpecId = base.defaultSpecId();
+ this.lastAssignedPartitionId = base.lastAssignedPartitionId();
+ this.defaultSortOrderId = base.defaultSortOrderId();
+ this.sortOrders = base.sortOrders();
+ this.properties = base.properties();
+ this.currentSnapshotId = base.currentSnapshotId();
+ this.snapshots = base.snapshots();
+ this.snapshotLog = base.snapshotLog();
+
+ this.baseFormatVersion = base.formatVersion();
+ this.baseFile = base.file();
+ this.baseLastUpdateMillis = base.lastUpdatedMillis();
+ this.basePreviousFiles = base.previousFiles();
+ return this;
+ }
+
+ @SuppressWarnings("checkstyle:CyclomaticComplexity")
+ public TableMetadata build() {
+ if (formatVersion == null) {
+ formatVersion = TableMetadata.DEFAULT_TABLE_FORMAT_VERSION;
+ } else if (formatVersion != baseFormatVersion) {
+ Preconditions.checkArgument(formatVersion <= TableMetadata.SUPPORTED_TABLE_FORMAT_VERSION,
+ "Cannot upgrade table to unsupported format version: v%s (supported: v%s)",
+ formatVersion, TableMetadata.SUPPORTED_TABLE_FORMAT_VERSION);
+ Preconditions.checkArgument(formatVersion >= baseFormatVersion,
+ "Cannot downgrade v%s table to v%s", baseFormatVersion, formatVersion);
+ }
Review comment:
I generally prefer to structure builders so that validation is done as soon as possible. That keeps `build` methods smaller. This could be moved into the `withFormatVersion` method.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #2957: Core: add TableMetadata update builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#issuecomment-923099516
@nastra I think we do want to have very defined behavior and easy customization to the builder, that's why I avoid using builder classes like the one you shared.
@rdblue this makes sense, I will update this based on your suggestion.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #2957: Core: add TableMetadata update builder
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #2957:
URL: https://github.com/apache/iceberg/pull/2957#discussion_r712340729
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -500,11 +510,14 @@ public TableMetadata updateSchema(Schema newSchema, int newLastColumnId) {
builder.add(new Schema(newSchemaId, newSchema.columns(), newSchema.identifierFieldIds()));
}
- return new TableMetadata(null, formatVersion, uuid, location,
- lastSequenceNumber, System.currentTimeMillis(), newLastColumnId,
- newSchemaId, builder.build(), defaultSpecId, updatedSpecs, lastAssignedPartitionId,
- defaultSortOrderId, updatedSortOrders, properties, currentSnapshotId, snapshots, snapshotLog,
- addPreviousFile(file, lastUpdatedMillis));
+ return builderFrom(this)
Review comment:
The previous file is added by default from the base metadata during build, unless explicitly overwritten.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org