Core: add TableMetadata update builder #2957

jackye1995 · 2021-08-11T05:42:52Z

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.

jackye1995 · 2021-08-11T05:45:23Z

core/src/main/java/org/apache/iceberg/TableMetadataBuilder.java

+    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");


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.

jackye1995 · 2021-08-11T05:47:37Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

  }

  public TableMetadata upgradeToFormatVersion(int newFormatVersion) {
-    Preconditions.checkArgument(newFormatVersion <= SUPPORTED_TABLE_FORMAT_VERSION,


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.

rdblue · 2021-08-29T21:13:43Z

core/src/main/java/org/apache/iceberg/TableMetadataParser.java

-        lastSequenceNumber, lastUpdatedMillis, lastAssignedColumnId, currentSchemaId, schemas, defaultSpecId, specs,
-        lastAssignedPartitionId, defaultSortOrderId, sortOrders, properties, currentVersionId,
-        snapshots, entries.build(), metadataEntries.build());
+    return TableMetadata.builder()


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.

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.

rdblue · 2021-08-29T21:14:30Z

core/src/main/java/org/apache/iceberg/TableMetadataBuilder.java

+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+
+public class TableMetadataBuilder {


Does this need to be public? I would prefer not making the builder public for now.

rdblue · 2021-08-29T21:50:09Z

core/src/main/java/org/apache/iceberg/TableMetadataBuilder.java

+          formatVersion, TableMetadata.SUPPORTED_TABLE_FORMAT_VERSION);
+      Preconditions.checkArgument(formatVersion >= baseFormatVersion,
+          "Cannot downgrade v%s table to v%s", baseFormatVersion, formatVersion);
+    }


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.

jackye1995 · 2021-09-13T05:36:48Z

@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.

rdblue · 2021-09-16T00:13:57Z

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?

nastra

I think it would be better if everything that needs to be updated is explicitly set via builder methods as right now it's a mix of updating things via builder methods and implicitly setting stuff in build() itself.
Also I'm not sure if in most cases withInputFile(null) is missing from the builder calls

nastra · 2021-09-16T06:45:12Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

-          lastSequenceNumber, lastUpdatedMillis, lastColumnId, currentSchemaId, schemas, defaultSpecId, specs,
-          lastAssignedPartitionId, defaultSortOrderId, sortOrders, properties,
-          currentSnapshotId, snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
+      return builderFrom(this).generateUUID().build();


should this also have a .withFile(null)?

it's null by default, is it worth adding it explicitly?

you're right, not worth adding it explicitly. I was confused with baseFile and file

nastra · 2021-09-16T06:48:18Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

-        newSchemaId, builder.build(), defaultSpecId, updatedSpecs, lastAssignedPartitionId,
-        defaultSortOrderId, updatedSortOrders, properties, currentSnapshotId, snapshots, snapshotLog,
-        addPreviousFile(file, lastUpdatedMillis));
+    return builderFrom(this)


should this have a .withFile(null) and .withPreviousFiles(addPreviousFile(file, lastUpdatedMillis))?

The previous file is added by default from the base metadata during build, unless explicitly overwritten.

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.

nastra · 2021-09-16T06:54:33Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

+        .withDefaultSpecId(newDefaultSpecId)
+        .withSpecs(builder.build())
+        .withLastAssignedPartitionId(Math.max(lastAssignedPartitionId, newPartitionSpec.lastAssignedFieldId()))
+        .build();


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)

nastra · 2021-09-16T06:57:54Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

-        lastAssignedPartitionId, defaultSortOrderId, sortOrders, properties, snapshot.snapshotId(), snapshots,
-        newSnapshotLog, addPreviousFile(file, lastUpdatedMillis));
+    return builderFrom(this)
+        .refreshLastUpdateMillis()


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

good point, will update them to match

nastra · 2021-09-16T07:08:51Z

@rdblue independently from this PR, have you guys thought about using Immutable annotations on things that should be immutable, which also gives you make-a-copy-of-X-and-update-fields-a-b-c for free?

jackye1995 · 2021-09-20T16:48:23Z

@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.

jackye1995 · 2021-12-05T00:46:03Z

close in favor of #3664

jackye1995 requested a review from rdblue August 11, 2021 05:42

github-actions bot added the core label Aug 11, 2021

jackye1995 commented Aug 11, 2021

View reviewed changes

jackye1995 force-pushed the metadata-builder branch from 424796c to 22df22e Compare August 11, 2021 05:49

rdblue reviewed Aug 29, 2021

View reviewed changes

rdblue mentioned this pull request Sep 12, 2021

Add TableLocationSupplier for more flexible table placement #2855

Closed

Core: add TableMetadata update builder

601da70

jackye1995 force-pushed the metadata-builder branch from 22df22e to 601da70 Compare September 13, 2021 05:11

jackye1995 changed the title ~~Core: introduce TableMetadataBuilder~~ Core: add TableMetadata update builder Sep 13, 2021

nastra reviewed Sep 16, 2021

View reviewed changes

This was referenced Nov 18, 2021

Bump Nessie to 0.15.1 + related changes #3257

Merged

Core: Use Immutables for TableMetadata #3580

Closed

jackye1995 closed this Dec 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: add TableMetadata update builder #2957

Core: add TableMetadata update builder #2957

jackye1995 commented Aug 11, 2021

jackye1995 Aug 11, 2021

jackye1995 Aug 11, 2021

rdblue Aug 29, 2021

rdblue Aug 29, 2021

rdblue Aug 29, 2021

rdblue Aug 29, 2021

jackye1995 commented Sep 13, 2021

rdblue commented Sep 16, 2021

nastra left a comment

nastra Sep 16, 2021

jackye1995 Sep 20, 2021

nastra Sep 21, 2021

nastra Sep 16, 2021

jackye1995 Sep 20, 2021

nastra Sep 21, 2021

nastra Sep 16, 2021

nastra Sep 16, 2021

jackye1995 Sep 20, 2021

nastra commented Sep 16, 2021 •

edited

Loading

jackye1995 commented Sep 20, 2021

jackye1995 commented Dec 5, 2021

Core: add TableMetadata update builder #2957

Core: add TableMetadata update builder #2957

Conversation

jackye1995 commented Aug 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackye1995 commented Sep 13, 2021

rdblue commented Sep 16, 2021

nastra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastra commented Sep 16, 2021 • edited Loading

jackye1995 commented Sep 20, 2021

jackye1995 commented Dec 5, 2021

nastra commented Sep 16, 2021 •

edited

Loading