API: Support removeUnusedSpecs in ExpireSnapshots #10755

advancedxy · 2024-07-23T13:32:24Z

This is a continue work of #3462, all the credits should goes to @RussellSpitzer.

Previously there was no way to remove partition specs from a table once they were
added. To fix this we add an api which searches through all reachable manifest
files and records their specsIds. Any specIds which do not find are marked for
removal which is done through a serializable commit.

advancedxy · 2024-07-23T13:38:51Z

@amogh-jahagirdar @RussellSpitzer @aokolnychyi @szehon-ho it would be great if you guys could take a look at this.

This PR is raised based the previous discussion in #10352 (comment)

amogh-jahagirdar

Thank you for carrying this forward @advancedxy ! I don't think I'd go with a general SetPartitionSpecs update, I think I'd have a RemovePartitionSpec, and the TableMetadata builder APIs to remove a given spec (which will have validation that we're not removing the current spec, the spec to remove is a valid partition spec etc).

Few more things to consider:

1.) Should we included removing unused schemas? I know there are users whose tables undergo numerous evolutions for adding fields, and they want to remove old schemas since their metadata ends up being so bloated to the point of performance concerns when reading! My conclusion here is no, I believe that should be a separate operation since I think we want APIs to do one thing and do it well.. A caller can combine the two if desired.

In this approach I think we'd have to do a REST spec change to introduce the new update type. If we think this API is worth it for general purpose metadata cleaning, beyond preventing the drop column issue then we'd probably have to go through with the spec change. However, if this is only being introduced for the drop column issue, maybe we want to think through lighter weight options to solve that particular issue?

Another possible way: are we able to retain the spec and essentially not care about the dropped field's existence? This may be something worth exploring. Sorry for failing to considering this earlier, I wasn't considering this because I assumed we wouldn't need any heavy weight spec changes and the API would be generalizable beyond this drop column case. I'll also do some exploration here.

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

advancedxy · 2024-07-24T03:51:52Z

I don't think I'd go with a general SetPartitionSpecs update, I think I'd have a RemovePartitionSpec, and the TableMetadata builder APIs to remove a given spec (which will have validation that we're not removing the current spec, the spec to remove is a valid partition spec etc).

This is a nice suggestion and better approach. I will change the withSpecs/setSpecs by using removePartitionSpec.

Replying other comments inline.

Should we included removing unused schemas?

No, and I think we are on the same page. PruneUnusedSchemas should be in a separate and dedicated action. However, it might not be possible to do that in current code as there's no SchemaId bounding to DataFile/DeleteFile. It's impossible to decide which schemas are unused?

In this approach I think we'd have to do a REST spec change to introduce the new update type. If we think this API is worth it for general purpose metadata cleaning, beyond preventing the drop column issue then we'd probably have to go through with the spec change. However, if this is only being introduced for the drop column issue, maybe we want to think through lighter weight options to solve that particular issue?

I don't think we should expose removePartitionSpec to the REST catalog, at least for now. I think RemoveUnusedSpecs is a general metadata cleaning API and worth introducing, however the API is self-contained and doesn't have to be coupled with REST catalog. Like the comment in the TableMetadata, it's not safe for external client to simply call removePartitionSpec without checking the spec is unused, which might not an easy task in the REST catalog. Unless we think it's worthy to add check by reading manifest files in the REST catalog, in that way, we may expose the removePartitionSpec to the REST catalog. That's why the org.apache.iceberg.MetadataUpdate.SetPartitionSpecs#applyTo method throws an exception instead of actually implementing it.

If introducing a new MetadataUpdate implies a REST spec change, I think we can change the TableMetadata.Builder#build to build without adding a RemovePartitionSpec change, how does that sounds to you?

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

RussellSpitzer · 2024-07-24T20:59:08Z

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

+          toBeRemoved != null && toBeRemoved.equals(spec),
+          "Cannot remove an unknown spec, spec id: %s",
+          spec.specId());
+      this.specs =


Should we be validating that all the specs we are trying to remove exist? I think this may be fine, just thinking about commit conflicts

Should we be validating that all the specs we are trying to remove exist?

I think so, and L1129 - L1133 indeed ensures the spec to be removed are existed in current table metadata.

I think this may be fine, just thinking about commit conflicts

If you are talking about concurrent RemovedUnusedSpecs, I think it's fine and reasonable to have only one attempt succeeded once at time. For other concurrent commit, there's no conflict and retrying should do the trick.

In general I'm in favor of succeeding in cases like this. There's no need to fail something if it has an accidental retry after the first attempt succeeded.

There's no need to fail something if it has an accidental retry after the first attempt succeeded.

This is a very good point, let me reconsider this part.

Addressed in 190dde6, removing an already removed unused spec should not cause a failure then.

In that case, is it really necessary to validate that a spec was removed and filter the list? If it is a noop to remove an unknown spec ID, then we can simplify the first half of this method:

Preconditions.checkArgument(!specIds.contains(defaultSpecId), "Cannot remove the default partition spec");

Updated, let me know if this is the desired change.

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

advancedxy · 2024-07-26T13:37:34Z

Close and re-open to trigger the CI.

Also gently ping @amogh-jahagirdar @RussellSpitzer to take another look.

api/src/main/java/org/apache/iceberg/RemoveUnusedSpecs.java

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

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

RussellSpitzer · 2024-07-26T19:54:18Z

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

+  /**
+   * Prune the unused partition specs from the table metadata.
+   *
+   * <p>Note: it's not safe for external client to call this directly, it's usually called by the


This method is package private, does it need this note?

I'd like to highlight this note to reduce potential misusage as it's possible for users to put their own code in org.apache.iceberg package to bypass Java's access control.

I agree with @RussellSpitzer. This class is internal and many of its methods can break tables if called incorrectly.

Also, we are no longer adding new operation methods to this. These days we make modifications to the Builder instead.

Let me remove the note then.

Also, we are no longer adding new operation methods to this. These days we make modifications to the Builder instead.

For this part, I think modifications are still going through the Builders. The main purpose of this method is to provide a single access point to prune unused specs purely so that prune unused specs are not mixed with other metadata updates.

core/src/test/java/org/apache/iceberg/TestRemoveUnusedSpecs.java

RussellSpitzer

I think this is really close, I just want to have those remaining nits of mine addressed.

amogh-jahagirdar

@advancedxy This is getting closer, thank you for moving from a "set" to a "remove" semantic, that fits better with the general principles of the project but I think the main question I had was around why we need a special flag for hasRemovedSpecs when building? It seems like that's just working around the fact that there's no metadata update, which I know I mentioned would require a spec change for REST. If that's the case, i think the right solution there should be to add a ToDo with a follow on issue for supporting it for REST.

Also since there's no update type for REST this should mean that the operation fails for REST on the client side so it's clear. Again I think that's fine in the interim, until we add the support for it, but I think we should ideally validate that behavior in a test since I think a bad case would be if the commit for the RemoveUnusedSpecs appears to succeed on REST but nothing actually happens. cc @RussellSpitzer @rdblue

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

amogh-jahagirdar · 2024-07-29T15:46:05Z

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

+   * @param toRemoveSpecs the partition specs to be removed
+   * @return the new table metadata with the unused partition specs removed
+   */
+  TableMetadata pruneUnusedSpecs(List<PartitionSpec> toRemoveSpecs) {


Can we call the parameter specsToRemove?

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

amogh-jahagirdar · 2024-07-29T15:58:25Z

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

+      if (hasRemovedSpecs) {
+        Preconditions.checkArgument(
+            changes.isEmpty(), "Cannot remove partition specs with other metadata update");


@advancedxy I'm not sure I follow, what's the intention of this check?

See https://github.com/apache/iceberg/pull/10755/files#r1690420193 and https://github.com/apache/iceberg/pull/10755/files#r1696099907

I think this check is to make sure that RemoveUnusedSpecs and other metadata updates are not happened together.

amogh-jahagirdar · 2024-07-29T16:01:27Z

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

@@ -1425,6 +1460,7 @@ private boolean hasChanges() {
          || (discardChanges && !changes.isEmpty())
          || metadataLocation != null
          || suppressHistoricalSnapshots
+          || hasRemovedSpecs


Hm I'm trying to understand why we need this special flag, is this a way so that we avoid having to add the metadata update type for REST (since then that would ultimately just be in the changes list)?

This is not the right way to update hasChanges. Instead, this needs to add a change to changes. That way it is sent to REST services to modify the table in the REST commit path.

is this a way so that we avoid having to add the metadata update type for REST (since then that would ultimately just be in the changes list)?

Yes, as discussed earlier, I don't think we should expose removePartitionSpec directly to the REST API as there's no easy way to ensure that the spec to remove is indeed not used.

This is not the right way to update hasChanges. Instead, this needs to add a change to changes.

It was adding a RemovePartitionSpec to the changes. However, that would require a REST spec change and it's a bit heavy. See discussions as well: #10755 (comment)

That way it is sent to REST services to modify the table in the REST commit path.

I might be missing something, so all the metadata changes have to be sent to the REST service? I didn't work with a REST catalog before and don't see how changes are sent to the REST service. It would be great that some reference or code could be pointed to.

I might be missing something, so all the metadata changes have to be sent to the REST service? I didn't work with a REST catalog before and don't see how changes are sent to the REST service. It would be great that some reference or code could be pointed to.

I took a look at the REST catalog related code today, it seems the impl in this PR doesn't work with Iceberg tables backend by REST catalog as there's no update added for RemovePartitionSpec. The RemoveUnusedSpecs will succeed without actually removing unused specs for REST catalog.

If supporting REST catalog is a must requirement, I think we have to go through a REST spec change to add new update type to reflect that. The only concern is how to enforce the removed spec is indeed not used any more? Do all the similar calculation in org.apache.iceberg.rest.CatalogHandlers#commit like how we did in BaseRemoveUnusedSpecs? Or is that necessary?

WDYT? @RussellSpitzer @rdblue @amogh-jahagirdar

Yes, as discussed earlier, I don't think we should expose removePartitionSpec directly to the REST API as there's no easy way to ensure that the spec to remove is indeed not used.

We don't currently have operations that can't be supported by the REST protocol, so this is an area where we should be careful. I agree that we want to ensure that there are no new references to specs that are being removed, but I'm skeptical that there is no way to do that. There are also problems with not sending this because it would be a silent no-op when sending changes to REST catalogs.

It's unlikely that a new snapshot would be written with a spec that is being removed because this already validates that the default spec is not being removed. A conflict here would require that a concurrent writer is using a spec other than the default or has changed the default spec. There's already a validation for the second case, assert-default-spec-id. For the first case, this could require that no branch states have changed using assert-ref-snapshot-id.

The RemoveUnusedSpecs will succeed without actually removing unused specs for REST catalog.

@advancedxy This indicates there's a problem with the current implementation then imo. If there was a way to avoid the REST spec change I think it would've been OK as long as we can guarantee failure on the client side until the support was added for the metadata update type. But I think that's unavoidable, and I agree with @rdblue that we should probably just add the metadata update type.

I also am reasonably confident that a REST catalog can safely handle this RemovePartitionSpec update. The default spec ID needs to be the same and there must have been no writes to any branches. If any of those are not true, the server should fail the update.

We don't currently have operations that can't be supported by the REST protocol, so this is an area where we should be careful.

Yes, after taking a look at the related code, I think we should strive to make all operations supported by REST protocol.

It's unlikely that a new snapshot would be written with a spec that is being removed because this already validates that the default spec is not being removed. A conflict here would require that a concurrent writer is using a spec other than the default or has changed the default spec. There's already a validation for the second case, assert-default-spec-id. For the first case, this could require that no branch states have changed using assert-ref-snapshot-id.

Thanks for the inspiring explanation. I wasn't worrying about the normal path, which I am also confident that REST catalog can safely handle. I'm more concerned of misusage, such that users issue a RemovePartitionSpec request without going through the RemoveUnusedSpec API or accidentally call TableMetadata.Builder.removePartitionSpecs directly. That's why there's method defined in TableMetadata class in the first place, to hide the Builder's method and ensure single point of access.

For the REST catalog, as it's open by default to all kinds of clients. It's more likely to be affected by the misusage. That's why I'm proposing in the catalog handler side do the check as well:

Do all the similar calculation in org.apache.iceberg.rest.CatalogHandlers#commit like how we did in BaseRemoveUnusedSpecs?

However, it's heavy operation, does that worth it?

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

core/src/test/java/org/apache/iceberg/TestRemoveUnusedSpecs.java

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

rdblue · 2024-07-29T23:36:19Z

api/src/main/java/org/apache/iceberg/RemoveUnusedSpecs.java

+ *
+ * <p>{@link #apply()} returns the specs that will remain if committed on the current metadata
+ */
+public interface RemoveUnusedSpecs extends PendingUpdate<List<PartitionSpec>> {}


Does this need to be a separate operation? It seems very specific. I wonder if it is worth adding a maintenance API that could cover more things, like removing old schemas as well.

Missed this comment. I think it would be wonderful to be able to remove unused schemas as well. However, it might depends on #4898 to reliably determine which schemaId is still in use.

Does this need to be a separate operation?

I think so, even if we are going to group other maintenance API like remove unused schema together, they are two different operations and should be in a dedicated operation.

I wonder if it is worth adding a maintenance API that could cover more things, like removing old schemas as well.

Do you by chance have any API name for the Metadata Maintenance class? I am think about something like MetadataCleaner.

dedicated operation

By "dedicated operation", I mean a method in the Table API. Adding this kind of thing is a lot of work, so I'd prefer a reusable option that can handle multiple tasks, like this:

table.maintenance() .removeUnusedSpecs() .removeUnusedSchemas() .commit()

Another option is to do this regularly as part of snapshot expiration. Have you considered that? Since expiration already reads manifests that could be a good place to do this.

[Removing schemas] might depends on #4898 to reliably determine which schemaId is still in use.

We don't need to check whether there are data files that were written with a particular schema ID, only whether there are snapshots that reference the schema ID. Data files are readable by any future schema by design.

table.maintenance() .removeUnusedSpecs() .removeUnusedSchemas() .commit()

This looks promising. But what if we need to further configure the maintenance operations, such as

table.maintenance().removeUnusedSpecs().retainLast(num).commit(); // or something table.maintenance().removeUnusedSchemas().setMinSchemasToKeep(num).commit();

Different maintenance operations may have different configure options. It would be better to use a dedicated operation for each purpose? Of course, It would be great that these maintenance APIs are grouped together.

Have you considered that? Since expiration already reads manifests that could be a good place to do this.

This is attempting and to be honest I haven't thought about it.

Update: just did a quick look at the RemoveSnapshots' implementation, it might add too much complexity to put remove unused spec logic in there.

@advancedxy I think a decent example to look at would be the ManageSnapshots API which handles cherry picking/rollback and branching/tagging operations. That is the public interface (analagous to "maintenance" in this case), but the implementation the individual operation implementations are still in separate classes which are package-private and focused on a single operation, as well as to enable different configuration options as you mentioned.

The ManageSnapshots implementation is tracking all of these operations as part of a transaction (which for the combined schema + partition spec pruning operation case sounds reasonable to me).

I think the pending question is should this be done as part of ExpireSnapshots or should we have a separate operation. If i think about when it makes sense for this cleanup to happen ExpireSnapshots does make sense. I also don't know what we'd call a separate "maintenance" API since there's quite a few maintenance operations in Iceberg.

I think a decent example to look at would be the ManageSnapshots API which handles cherry picking/rollback and branching/tagging operations.

Yes, I am thinking about something similar to that.

If i think about when it makes sense for this cleanup to happen ExpireSnapshots does make sense. I also don't know what we'd call a separate "maintenance" API since there's quite a few maintenance operations in Iceberg.

I agree it's attempting. But I would prefer to use a maintenance API, for the following reasons:

Currently ExpireSnapshots extends PendingUpdate<List<Snapshot>>, if we are going to remove unused spec(and maybe unused schemas as well), we have to change the interface signature, which is breaking change.

ExpireSnapshots doesn't read manifest list yet, it only leverage SnapshotRef and Snapshot to calculate expired snapshots. It's adding complexity and breaks the do one thing and do it well philosophy.

I agree it's attempting. But I would prefer to use a maintenance API, for the following reasons: Currently ExpireSnapshots extends PendingUpdate<List>, if we are going to remove unused spec(and maybe unused schemas as well), we have to change the interface signature, which is breaking change.
ExpireSnapshots doesn't read manifest list yet, it only leverage SnapshotRef and Snapshot to calculate expired snapshots. It's adding complexity and breaks the do one thing and do it well philosophy.

I don't think this is necessarily true that we need to change the interface signature. We are still producing a new set of snapshots that are removed as part of the procedure but in addition to that we are (in an opt-in manner) pruning out the unused specs/schemas which is orthogonal to snapshots for this purpose.

As part of file cleanup the procedure does determine which files are still referenced (going through manifest lists/manifests) and which should be removed (this is in the FileCleanupStrategy implementations like ReachableFileCleanup. I think it's true that it adds a bit of complexity but the complexity is tracking which partition specs are referenced as we traverse the manifests. The same users which are frequently running expire snapshots also probably want to get rid of unused specs/schemas to keep metadata sizes smaller. After some more thought, I feel like it aligns with "one thing and do it well" since the "one thing" logic we're talking about is already common (the logic to traverse manifests) relative to expanding the API surface

rdblue · 2024-07-29T23:38:07Z

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

+            });
+  }
+
+  private TableMetadata removeUnusedSpecs(TableMetadata current) {


I think other operations typically call this method internalApply when it is the core functionality of apply but the class needs to call it from both apply and commit.

Let me rename it to internalApply then.

rdblue · 2024-07-29T23:42:01Z

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

+                MetadataTableUtils.createMetadataTableInstance(table, MetadataTableType.ALL_ENTRIES)
+                    .newScan()
+                    .planFiles(),
+                task -> ((BaseEntriesTable.ManifestReadTask) task).partitionSpecId()));


We generally avoid unchecked casts because this creates a brittle dependency on a particular type being produced. That in turn limits our ability to trust the type system and make reasonable changes quickly.

If I understand correctly, the purpose of using a metadata table here is not to use the Table interface, but instead to reuse some of the code in the all_entries table. I think a more direct path would be to use ManifestGroup and possibly refactor some of the logic from the all_entries table to be more easily reused.

Good point, let me refactor this to avoid unchecked cast.

If I understand correctly, the purpose of using a metadata table here is not to use the Table interface, but instead to reuse some of the code in the all_entries table

I think this code is used to avoid actually accessing the underlying rows, see #3462 (comment). We can/should use the Table interface to access Manifest files directly.

How does that sound to you?

rdblue · 2024-07-29T23:45:27Z

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

@@ -1102,6 +1121,22 @@ public Builder setDefaultPartitionSpec(int specId) {
      return this;
    }

+    private Builder removePartitionSpec(PartitionSpec spec) {
+      Preconditions.checkArgument(
+          changes.isEmpty(), "Cannot remove partition spec with other metadata update");


Why is this necessary?

I think this is needed to avoid conflicts with changes to the default spec ID. I'd probably change this to allow other changes and instead update this and the methods that set the default spec ID to check for one another.

I think this is needed to avoid conflicts with changes to the default spec ID.

Yeah, besides SetDefaultSepc, AddPartitionSpec might also interfere with this. For safety purposes, the previous implementation rejects all other metadata updates since removePartitionSpec is rarely called and always called alone.

advancedxy

Thanks @rdblue and @amogh-jahagirdar for reviewing, will address them in a new commit.

api/src/main/java/org/apache/iceberg/Table.java

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

advancedxy · 2024-07-30T00:41:04Z

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

+            });
+  }
+
+  private TableMetadata removeUnusedSpecs(TableMetadata current) {


Let me rename it to internalApply then.

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

advancedxy · 2024-07-30T00:50:58Z

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

+                MetadataTableUtils.createMetadataTableInstance(table, MetadataTableType.ALL_ENTRIES)
+                    .newScan()
+                    .planFiles(),
+                task -> ((BaseEntriesTable.ManifestReadTask) task).partitionSpecId()));


Good point, let me refactor this to avoid unchecked cast.

If I understand correctly, the purpose of using a metadata table here is not to use the Table interface, but instead to reuse some of the code in the all_entries table

I think this code is used to avoid actually accessing the underlying rows, see #3462 (comment). We can/should use the Table interface to access Manifest files directly.

How does that sound to you?

advancedxy · 2024-07-30T00:56:11Z

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

+  /**
+   * Prune the unused partition specs from the table metadata.
+   *
+   * <p>Note: it's not safe for external client to call this directly, it's usually called by the


Let me remove the note then.

Also, we are no longer adding new operation methods to this. These days we make modifications to the Builder instead.

For this part, I think modifications are still going through the Builders. The main purpose of this method is to provide a single access point to prune unused specs purely so that prune unused specs are not mixed with other metadata updates.

advancedxy · 2024-07-30T01:12:28Z

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

@@ -1102,6 +1121,22 @@ public Builder setDefaultPartitionSpec(int specId) {
      return this;
    }

+    private Builder removePartitionSpec(PartitionSpec spec) {
+      Preconditions.checkArgument(
+          changes.isEmpty(), "Cannot remove partition spec with other metadata update");


I think this is needed to avoid conflicts with changes to the default spec ID.

Yeah, besides SetDefaultSepc, AddPartitionSpec might also interfere with this. For safety purposes, the previous implementation rejects all other metadata updates since removePartitionSpec is rarely called and always called alone.

advancedxy · 2024-07-30T01:15:05Z

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

+          toBeRemoved != null && toBeRemoved.equals(spec),
+          "Cannot remove an unknown spec, spec id: %s",
+          spec.specId());
+      this.specs =


There's no need to fail something if it has an accidental retry after the first attempt succeeded.

This is a very good point, let me reconsider this part.

advancedxy · 2024-07-30T01:16:50Z

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

+      if (hasRemovedSpecs) {
+        Preconditions.checkArgument(
+            changes.isEmpty(), "Cannot remove partition specs with other metadata update");


See https://github.com/apache/iceberg/pull/10755/files#r1690420193 and https://github.com/apache/iceberg/pull/10755/files#r1696099907

I think this check is to make sure that RemoveUnusedSpecs and other metadata updates are not happened together.

advancedxy · 2024-07-30T01:38:41Z

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

@@ -1425,6 +1460,7 @@ private boolean hasChanges() {
          || (discardChanges && !changes.isEmpty())
          || metadataLocation != null
          || suppressHistoricalSnapshots
+          || hasRemovedSpecs


is this a way so that we avoid having to add the metadata update type for REST (since then that would ultimately just be in the changes list)?

Yes, as discussed earlier, I don't think we should expose removePartitionSpec directly to the REST API as there's no easy way to ensure that the spec to remove is indeed not used.

This is not the right way to update hasChanges. Instead, this needs to add a change to changes.

It was adding a RemovePartitionSpec to the changes. However, that would require a REST spec change and it's a bit heavy. See discussions as well: #10755 (comment)

That way it is sent to REST services to modify the table in the REST commit path.

I might be missing something, so all the metadata changes have to be sent to the REST service? I didn't work with a REST catalog before and don't see how changes are sent to the REST service. It would be great that some reference or code could be pointed to.

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

nastra · 2024-11-12T11:38:58Z

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java

+
+    Table loaded = catalog.loadTable(TABLE);
+    assertThat(loaded.specs().values())
+        .hasSameElementsAs(Lists.asList(spec, current, new PartitionSpec[0]));


containsExactly(..)

nastra · 2024-11-12T11:39:30Z

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java

+    table.updateSpec().addField(Expressions.bucket("data", 16)).commit();
+    table.updateSpec().removeField(Expressions.bucket("data", 16)).commit();
+    table.updateSpec().addField("data").commit();
+    assertThat(table.specs().size()).as("Should have 3 total specs").isEqualTo(3);


Suggested change

assertThat(table.specs().size()).as("Should have 3 total specs").isEqualTo(3);

assertThat(table.specs()).as("Should have 3 total specs").hasSize(3);

advancedxy · 2024-11-18T12:35:55Z

@nastra Thanks for reviewing, all your comments should be addressed.

api/src/main/java/org/apache/iceberg/ExpireSnapshots.java

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

core/src/test/java/org/apache/iceberg/TestUpdateRequirements.java

nastra · 2024-11-21T15:46:12Z

core/src/test/java/org/apache/iceberg/TestUpdateRequirements.java

+    SnapshotRef snapshotRef = mock(SnapshotRef.class);
+    when(snapshotRef.snapshotId()).thenReturn(snapshotId);
+    when(snapshotRef.isBranch()).thenReturn(true);
+    when(metadata.refs()).thenReturn(ImmutableMap.of(branch, snapshotRef));


this also requires mocking this: when(metadata.ref(branch)).thenReturn(snapshotRef); (it wasn't failing because the call to validate() was missing)

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java

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

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

api/src/main/java/org/apache/iceberg/ExpireSnapshots.java

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

amogh-jahagirdar · 2024-11-21T23:34:26Z

api/src/main/java/org/apache/iceberg/ExpireSnapshots.java

+   *     reachable by any snapshot
+   * @return this for method chaining
+   */
+  default ExpireSnapshots removeUnusedSpecs(boolean removeUnusedSpecs) {


Thanks @advancedxy ! I'm in favor of the client side API option for now, just had a comment on the name of it.

I think there's an important discussion to be had on how REST servers can indicate to clients the specific supported update types, this can either be done through config endpoint or through the existing endpoints discovery mechanism where each update type is attached as a query fragment to the endpoint and clients just use that.

My opinion is that it's not worth blocking this on a REST protocol discussion since we can always just come back and deprecate this client side option and have it run as part of the default procedure implementation.

advancedxy · 2024-11-25T02:21:02Z

@nastra @amogh-jahagirdar PTAL again, I think all your comments are addressed.

nastra · 2024-11-25T11:30:04Z

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

+            .forEach(
+                (name, ref) -> {
+                  if (ref.isBranch() && !name.equals(SnapshotRef.MAIN_BRANCH)) {
+                    require(new UpdateRequirement.AssertRefSnapshotID(name, ref.snapshotId()));


we should probably also have a test in TestUpdateRequirements that actually changes the branch and eventually fails

in addition to this, CatalogTests currently only tests the happy path, but not the exceptions that are either thrown by AssertDefaultSpecID or AssertRefSnapshotID. We need to test both cases in CatalogTests

I updated the TestUpdateRequirements. However, it's hard to test the failure cases in CatalogTests: before committing, the internalApply also refreshes the base table, so we cannot create a concurrent update to base table easily, which has to be after the refresh and before commit. Do you have any suggestions?

amogh-jahagirdar

Thanks for your patience and dilligent work @advancedxy, that's really appreciated! I think this is very close just had a comment on the builder API and the JavaDoc.

amogh-jahagirdar · 2024-12-02T17:40:36Z

api/src/main/java/org/apache/iceberg/ExpireSnapshots.java

+   * Allows expiration of unreachable metadata, such as partition spec as part of the operation.
+   *
+   * @param clean setting this to true will remove metadata(such as partition spec) that is no
+   *     longer reachable by any snapshot
+   * @return this for method chaining


/** * Allows expiration of unreachable table layout metadata, such as partition specs as part of the operation. * * @param setting this to true will remove table layout metadata that is no * longer reachable by any snapshot * @return this for method chaining

Basically don't think we need to mention the "such as partition spec" twice, I think once at the top level and clarifying that we're talking about layout metadata like the spec should be sufficient imo.

Good point, fixed.

amogh-jahagirdar · 2024-12-02T17:48:43Z

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

@@ -1108,6 +1108,25 @@ public Builder setDefaultPartitionSpec(int specId) {
      return this;
    }

+    Builder removeSpecIds(Iterable<Integer> specIds) {


I think I'd prefer this be called removeSpecs. If we look at the other APIs on TableMetadata builder like setDefaultPartitionSpec(int specId) it aligns with that naming, I don't think we need the "Ids" suffix. In case we ever need a removeSpecs which takes in actual PartitionSpec we'd just overload at that time just like setDefaultPartitionSpec

I'm not super opinionated here though, I remember the early discussion we had where I also mentioned removeSpecIds but this is just something I noticed when taking a look just now.

Updated too. I do think removeSpecs is more consistent with other APIs in the TableMetadata builder.

advancedxy · 2024-12-04T15:30:54Z

@amogh-jahagirdar @nastra since all comments are addressed, do you think it's ready to merge? I'm happy to address more comments if needed.

amogh-jahagirdar · 2024-12-04T16:09:32Z

Thanks @advancedxy , I'll leave it open for a bit since @danielcweeks also mentioned he would take a look. But from my side everything LGTM.

advancedxy · 2024-12-13T12:35:40Z

@danielcweeks would you mind to take a look at this?

rdblue · 2024-12-17T22:00:58Z

api/src/main/java/org/apache/iceberg/ExpireSnapshots.java

+   * operation.
+   *
+   * @param clean setting this to true will remove table layout metadata that is no longer reachable
+   *     by any snapshot


This is a bit verbose. I'd recommend being more direct and not making it a full sentence. Something like:

remove unused partition specs, schemas, or other metadata when true

The same could apply to the method description:

Enable cleaning up unused partition specs, schemas, or other metadata.

Good point, Updated.

rdblue · 2024-12-17T22:01:54Z

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

@@ -273,7 +275,7 @@ private Set<String> findFilesToDelete(
      Set<ManifestFile> manifestsToScan,
      Set<ManifestFile> manifestsToRevert,
      Set<Long> validIds,
-      TableMetadata current) {
+      Map<Integer, PartitionSpec> specsById) {


+1 for fixing this. In general we should avoid passing huge metadata objects around to helper methods that don't need everything!

rdblue · 2024-12-19T16:51:32Z

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

+                  base.snapshot(snapshot).allManifests(ops.io()).stream()
+                      .map(ManifestFile::partitionSpecId)
+                      .forEach(reachableSpecs::add));
+      Set<Integer> specsToRemove =


Nit: newline after a loop.

github-actions bot added API core labels Jul 23, 2024

advancedxy mentioned this pull request Jul 23, 2024

Core: Prevent dropping column which is referenced by active partition… #10352

Closed

amogh-jahagirdar requested changes Jul 23, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/TableMetadata.java Outdated Show resolved Hide resolved

advancedxy commented Jul 24, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/TableMetadata.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Jul 24, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/TableMetadata.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Jul 24, 2024

View reviewed changes

advancedxy commented Jul 25, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/TableMetadata.java Outdated Show resolved Hide resolved

advancedxy closed this Jul 26, 2024

advancedxy reopened this Jul 26, 2024

RussellSpitzer reviewed Jul 26, 2024

View reviewed changes

api/src/main/java/org/apache/iceberg/RemoveUnusedSpecs.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Jul 26, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseMetadataTable.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Jul 26, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseRemoveUnusedSpecs.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Jul 26, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseRemoveUnusedSpecs.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Jul 26, 2024

View reviewed changes

core/src/test/java/org/apache/iceberg/TestRemoveUnusedSpecs.java Outdated Show resolved Hide resolved

RussellSpitzer approved these changes Jul 26, 2024

View reviewed changes

amogh-jahagirdar reviewed Jul 29, 2024

View reviewed changes

amogh-jahagirdar requested a review from rdblue July 29, 2024 23:27

amogh-jahagirdar reviewed Jul 29, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseRemoveUnusedSpecs.java Outdated Show resolved Hide resolved

rdblue reviewed Jul 29, 2024

View reviewed changes

advancedxy commented Jul 30, 2024

View reviewed changes

rdblue reviewed Jul 30, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseRemoveUnusedSpecs.java Outdated Show resolved Hide resolved

rdblue reviewed Jul 30, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/BaseRemoveUnusedSpecs.java Outdated Show resolved Hide resolved

nastra reviewed Nov 12, 2024

View reviewed changes

address comments

0cd6e51

nastra reviewed Nov 21, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/TableMetadata.java Outdated Show resolved Hide resolved

nastra reviewed Nov 21, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/UpdateRequirements.java Outdated Show resolved Hide resolved

amogh-jahagirdar added this to the Iceberg 1.8.0 milestone Nov 21, 2024

amogh-jahagirdar reviewed Nov 21, 2024

View reviewed changes

address comments

a4635e9

nastra reviewed Nov 25, 2024

View reviewed changes

address comments

c02354e

advancedxy closed this Nov 29, 2024

advancedxy reopened this Nov 29, 2024

nastra approved these changes Dec 2, 2024

View reviewed changes

nastra mentioned this pull request Dec 2, 2024

Add partition expire for Iceberg #11686

Open

3 tasks

amogh-jahagirdar reviewed Dec 2, 2024

View reviewed changes

Address comments

323d43a

amogh-jahagirdar approved these changes Dec 4, 2024

View reviewed changes

amogh-jahagirdar requested a review from rdblue December 6, 2024 22:04

rdblue reviewed Dec 17, 2024

View reviewed changes

address comments

5f08dfa

rdblue reviewed Dec 19, 2024

View reviewed changes

Address comments

989beaa

anuragmantri mentioned this pull request Dec 20, 2024

Core: Prevent dropping column which is referenced by active partition specs #11842

Open

	assertThat(table.specs().size()).as("Should have 3 total specs").isEqualTo(3);
	assertThat(table.specs()).as("Should have 3 total specs").hasSize(3);

API: Support removeUnusedSpecs in ExpireSnapshots #10755

Are you sure you want to change the base?

API: Support removeUnusedSpecs in ExpireSnapshots #10755

Conversation

advancedxy commented Jul 23, 2024 • edited Loading

advancedxy commented Jul 23, 2024

amogh-jahagirdar left a comment • edited Loading

Choose a reason for hiding this comment

advancedxy commented Jul 24, 2024

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

advancedxy commented Jul 26, 2024

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

RussellSpitzer left a comment

Choose a reason for hiding this comment

amogh-jahagirdar left a comment • edited Loading

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

amogh-jahagirdar Jul 30, 2024 • edited Loading

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

amogh-jahagirdar Jul 30, 2024 • edited Loading

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

advancedxy 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

advancedxy commented Nov 18, 2024

nastra Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

advancedxy commented Nov 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

advancedxy commented Jul 23, 2024 •

edited

Loading

amogh-jahagirdar left a comment •

edited

Loading

amogh-jahagirdar left a comment •

edited

Loading

amogh-jahagirdar Jul 30, 2024 •

edited

Loading

amogh-jahagirdar Jul 30, 2024 •

edited

Loading

nastra Nov 21, 2024 •

edited

Loading

amogh-jahagirdar Dec 2, 2024 •

edited

Loading

amogh-jahagirdar Dec 2, 2024 •

edited

Loading