ice: Added support for deleting partitions #26

subkanthi · 2025-05-12T15:21:03Z

Delete partitions.
closes: #7

…eletes

subkanthi · 2025-05-15T18:47:12Z

spark-sql (nyc)> select count(*) from taxismay15 where tpep_pickup_datetime='2025-01-01';
2

ice delete-file --namespace=nyc --table=taxismay15 --partition='[{"partition_name": "tpep_pickup_datetime", "value": "2025-01-01T00:00:00"}]'

spark-sql (nyc)> select count(*) from taxismay15 where tpep_pickup_datetime='2025-01-01';
0

shyiko · 2025-05-15T19:33:59Z

examples/scratch/README.md

@@ -50,6 +50,9 @@ ice create-table flowers.iris_no_copy --schema-from-parquet=file://iris.parquet
 local-mc cp iris.parquet local/bucket1/flowers/iris_no_copy/
 ice insert flowers.iris_no_copy --no-copy s3://bucket1/flowers/iris_no_copy/iris.parquet

+# delete partition
+ice delete-file --namespace=nyc --table=taxismay15 --partition='[{"partition_name": "tpep_pickup_datetime", "value": "2025-01-01T00:00:00"}]'


s/delete-file/delete to be consistent with ice insert + we're not deleting "file" here

s/partition_name/name

shyiko · 2025-05-15T19:39:06Z

ice/src/main/java/com/altinity/ice/cli/internal/cmd/DeleteFile.java

+      for (com.altinity.ice.cli.Main.PartitionFilter pf : partitions) {
+        org.apache.iceberg.expressions.Expression e =
+            org.apache.iceberg.expressions.Expressions.equal(pf.partitionName(), pf.value());
+        expr = (expr == null) ? e : org.apache.iceberg.expressions.Expressions.and(expr, e);


shouldn't this be "or" instead of "and"? I think it's time to add tests

shyiko · 2025-05-15T19:46:01Z

ice/src/main/java/com/altinity/ice/cli/Main.java

+      @CommandLine.Option(
+              names = {"--partition"},
+              description =
+                  "JSON array of partition filters: [{\"partition_name\": \"vendorId\", \"value\": 5}]. "


it would be nice if there was a way to provide a list of values without having to repeat partition name

partition: values

shyiko · 2025-05-15T19:46:56Z

ice/src/main/java/com/altinity/ice/cli/internal/cmd/DeleteFile.java

+      RESTCatalog catalog,
+      String namespace,
+      String tableName,
+      List<com.altinity.ice.cli.Main.PartitionFilter> partitions)


supporting dryRun (that would print all matched files) would also be nice

…eletes

shyiko · 2025-05-25T05:35:09Z

ice/src/main/java/com/altinity/ice/cli/Main.java

+  void deletePartition(
+      @CommandLine.Option(names = "--namespace", description = "Namespace name", required = true)
+          String namespace,
+      @CommandLine.Option(names = "--table", description = "Table name", required = true)


specifying ns & table via --namespace & --table is inconsistent with the rest of the commands: ice insert, describe, etc. all expect ns.table as an arg.

…eletes

…idate ice CLI commands.

…eletes

…fied Integration test to start admin server and use etcdcatalog

shyiko · 2025-06-23T22:45:02Z

ice-rest-catalog/src/test/java/com/altinity/ice/rest/catalog/RESTCatalogIT.java

+  private Server adminServer;
+
+  @SuppressWarnings("rawtypes")
+  private final GenericContainer etcd =


lets move RESTCatalogIT to a separate PR. There are still a few things to iron out here:

doesn't looks like we really need etcd here. in-memory sqlite should be enough

redundant icebergConfig in https://github.com/Altinity/ice/pull/26/files#diff-ca269f882cb860088e50c407fa428d338e81103552eedf6376bcca8768fbab18R76-R84

Thread.sleep(5000);

not sure how https://github.com/Altinity/ice/pull/26/files#diff-ca269f882cb860088e50c407fa428d338e81103552eedf6376bcca8768fbab18R124-R126 can possibly work considering that we're not starting minio here

adminServer is not needed

shyiko · 2025-06-23T22:46:40Z

ice/src/main/java/com/altinity/ice/cli/Main.java

+      }
+      TableIdentifier tableId = TableIdentifier.parse(name);
+
+      DeletePartition.run(catalog, tableId, partitions, dryRun);


s/DeletePartition/Delete

shyiko · 2025-06-23T22:48:29Z

ice/src/main/java/com/altinity/ice/cli/Main.java

@@ -426,6 +428,42 @@ void deleteNamespace(
    }
  }

+  @CommandLine.Command(name = "delete", description = "Delete Partition(s).")


Suggested change

@CommandLine.Command(name = "delete", description = "Delete Partition(s).")

@CommandLine.Command(name = "delete", description = ""Delete data from catalog.")

shyiko · 2025-06-23T22:48:41Z

ice/src/main/java/com/altinity/ice/cli/Main.java

@@ -426,6 +428,42 @@ void deleteNamespace(
    }
  }

+  @CommandLine.Command(name = "delete", description = "Delete Partition(s).")
+  void deletePartition(


s/deletePartition/delete

shyiko · 2025-06-23T22:50:13Z

ice/src/main/java/com/altinity/ice/cli/Main.java

+      @CommandLine.Option(
+              names = "--dry-run",
+              description = "Log files that would be deleted without actually deleting them")
+          boolean dryRun)


might be worth it to have it true be default considering the nature of the operation

…eletes

subkanthi · 2025-06-29T21:24:29Z

Testing:
delete creates a new avro file in metadata.

shyiko · 2025-06-30T01:38:37Z

ice/src/main/java/com/altinity/ice/cli/internal/cmd/Delete.java

+      }
+      scan = scan.filter(expr);
+    }
+    Iterable<FileScanTask> tasks = scan.planFiles();


scan.planFiles returns CloseableIterable which is supposed to be closed

…d added close()

shyiko · 2025-06-30T17:58:33Z

ice/src/main/java/com/altinity/ice/cli/internal/cmd/Delete.java

+    for (FileScanTask task : tasks) {
+      filesToDelete.add(task.file());
+    }
+    tasks.close();


what if tasks.next() throws?

shyiko · 2025-07-02T01:30:16Z

ice/src/main/java/com/altinity/ice/cli/internal/cmd/Delete.java

+        filesToDelete.add(task.file());
+      }
+    } catch (Exception e) {
+      logger.error("Error getting files to delete: {}", e.getMessage());


https://rolf-engelhard.de/2013/04/logging-anti-patterns-part-ii/

shyiko · 2025-07-07T20:59:34Z

ice/src/main/java/com/altinity/ice/cli/internal/cmd/Delete.java

+        filesToDelete.add(task.file());
+      }
+    } catch (Exception e) {
+      throw e;


what's the point to

catch (Exception e) { throw e; }

?

I thought the try-with-resources require catch block, I was wrong

ice: Added support for positional delete file.

e93e775

subkanthi linked an issue May 12, 2025 that may be closed by this pull request

ice: Support deletes #7

Closed

shyiko force-pushed the master branch from da74387 to 292889e Compare May 12, 2025 17:18

subkanthi added 2 commits May 14, 2025 12:41

ice: Added support to delete file with partition name and value.

7b976f6

Merge branch 'master' of github.com:Altinity/ice into 7-ice-support-d…

732804a

…eletes

subkanthi changed the title ~~ice: Added support for positional delete file.~~ ice: Added support for deleting partitions May 15, 2025

ice: Added support to delete file with partition name and value.

615b430

subkanthi marked this pull request as ready for review May 15, 2025 18:45

subkanthi requested a review from shyiko May 15, 2025 18:45

subkanthi added 3 commits May 15, 2025 14:52

ice: Fix formatting errors.

40838b4

ice: Remove wildcard imports

539d889

ice: Fix formatting.

40a313a

shyiko reviewed May 15, 2025

View reviewed changes

subkanthi added 3 commits May 21, 2025 15:30

Merge branch 'master' of github.com:Altinity/ice into 7-ice-support-d…

25ac38e

…eletes

ice: Renamed DeleteFile to DeletePartition, Added --dry-run option.

d8ca2ee

ice: DeletePartition , add support to delete a list of values.

7aa2075

shyiko reviewed May 25, 2025

View reviewed changes

subkanthi added 12 commits June 5, 2025 08:41

Added Integration test to start rest catalog server with shutdown

84247ec

Merged conflicts from main.

0f16095

Merged conflicts from main.

dd5ee85

Merged conflicts from main.

00dae46

Merged conflicts from main.

c14da54

Fixed imports

13e0751

revert devbox.json changes.

2e5795a

Merge branch 'master' of github.com:Altinity/ice into 7-ice-support-d…

c720c83

…eletes

Added RESTCatalogIT integration test to start ice rest server and val…

4f93ceb

…idate ice CLI commands.

Reverted back change to shutDown token.

86c0e7f

formatting changes.

2396bc1

Merge branch 'master' of github.com:Altinity/ice into 7-ice-support-d…

cabb5a4

…eletes

subkanthi added 2 commits June 20, 2025 00:24

Modified deletePartition to use namespace.tablename as argument. Modi…

7f7f1a8

…fied Integration test to start admin server and use etcdcatalog

Added insert command to RESTCatalogIT

b75d4af

shyiko reviewed Jun 23, 2025

View reviewed changes

subkanthi added 6 commits June 28, 2025 09:42

Merge branch 'master' of github.com:Altinity/ice into 7-ice-support-d…

ede4b82

…eletes

Addressed PR review comments.

3b4ae26

Reverted back removal of private

12ab306

Removed yaml files added for Integration test.

bce53f2

reverted back private change to Main

271c864

Updated README.md with example

a474855

subkanthi requested a review from shyiko June 29, 2025 21:25

shyiko reviewed Jun 30, 2025

View reviewed changes

Removed wildcard imports, replaced Iterable with CloseableIterable an…

9f5a8be

…d added close()

subkanthi requested a review from shyiko June 30, 2025 17:47

shyiko reviewed Jun 30, 2025

View reviewed changes

Add try/catch to close CloseableIterable.

344b0b3

subkanthi requested a review from shyiko July 1, 2025 21:06

shyiko reviewed Jul 2, 2025

View reviewed changes

Remove logging line before throwing exception.

23c389a

shyiko reviewed Jul 7, 2025

View reviewed changes

Removed catch block.

ee0c915

subkanthi requested a review from shyiko July 8, 2025 21:36

shyiko approved these changes Jul 9, 2025

View reviewed changes

subkanthi merged commit 50ee773 into master Jul 9, 2025
1 check passed

	@CommandLine.Command(name = "delete", description = "Delete Partition(s).")
	@CommandLine.Command(name = "delete", description = ""Delete data from catalog.")

ice: Added support for deleting partitions #26

ice: Added support for deleting partitions #26

Uh oh!

Conversation

subkanthi commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

subkanthi commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

subkanthi commented Jun 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

subkanthi commented May 12, 2025 •

edited

Loading

subkanthi commented May 15, 2025 •

edited

Loading