Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-03-14 Thread via GitHub
pvary commented on PR #11837: URL: https://github.com/apache/iceberg/pull/11837#issuecomment-2724153881 LGTM +1 Let's wait a bit, so if anyone would like to comment, they can. Will merge next week, unless there are any objections in the meantime -- This is an automated message from t

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-03-13 Thread via GitHub
gaborkaszab commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1993175661 ## core/src/main/java/org/apache/iceberg/FileCleanupStrategy.java: ## @@ -23,15 +23,25 @@ import java.util.function.Consumer; import org.apache.iceberg.avro.Avr

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-03-13 Thread via GitHub
pvary commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1993268022 ## core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java: ## @@ -1621,6 +1627,82 @@ public void testRetainFilesOnRetainedBranches() { assertThat(deletedFi

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-03-13 Thread via GitHub
gaborkaszab commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1993039477 ## core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java: ## @@ -1621,6 +1627,82 @@ public void testRetainFilesOnRetainedBranches() { assertThat(del

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-03-11 Thread via GitHub
pvary commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1989419495 ## core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java: ## @@ -1621,6 +1627,82 @@ public void testRetainFilesOnRetainedBranches() { assertThat(deletedFi

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-03-11 Thread via GitHub
pvary commented on PR #11837: URL: https://github.com/apache/iceberg/pull/11837#issuecomment-2714512533 I'm fine with the current approach. Could you please rebase, and address the little nit? Thanks, Peter -- This is an automated message from the Apache Git Service. To respond to

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-03-11 Thread via GitHub
pvary commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1989420919 ## core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java: ## @@ -1621,6 +1627,82 @@ public void testRetainFilesOnRetainedBranches() { assertThat(deletedFi

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-03-11 Thread via GitHub
pvary commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1989403001 ## core/src/main/java/org/apache/iceberg/FileCleanupStrategy.java: ## @@ -23,15 +23,25 @@ import java.util.function.Consumer; import org.apache.iceberg.avro.Avro; im

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-03-03 Thread via GitHub
gaborkaszab commented on PR #11837: URL: https://github.com/apache/iceberg/pull/11837#issuecomment-2693931184 > Why did you decide against an util method? The code is really very similar to the orphan file removal stuff I figured that first we should agree on the approach, because las

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-28 Thread via GitHub
pvary commented on PR #11837: URL: https://github.com/apache/iceberg/pull/11837#issuecomment-2690965941 Why did you decide against an util method? The code is really very similar to the orphan file removal stuff -- This is an automated message from the Apache Git Service. To respond to th

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-28 Thread via GitHub
gaborkaszab commented on PR #11837: URL: https://github.com/apache/iceberg/pull/11837#issuecomment-2690864221 Hi @amogh-jahagirdar , @pvary , I managed to simplify the original PR and uploaded a new version. I took a deeper look at how [orphan file deletion does the same thing](https

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-14 Thread via GitHub
gaborkaszab commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1955942576 ## core/src/main/java/org/apache/iceberg/BulkDeleteConsumer.java: ## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or mo

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-14 Thread via GitHub
gaborkaszab commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1955843549 ## core/src/main/java/org/apache/iceberg/BulkDeleteConsumer.java: ## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or mo

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-14 Thread via GitHub
gaborkaszab commented on PR #11837: URL: https://github.com/apache/iceberg/pull/11837#issuecomment-2658767328 Thanks for taking a look @pvary, @amogh-jahagirdar and @steveloughran ! I'll be offline for a couple of days but will take a deeper look after that. Another thing we discussed

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-14 Thread via GitHub
gaborkaszab commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1955828374 ## core/src/main/java/org/apache/iceberg/BulkDeleteConsumer.java: ## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or mo

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-14 Thread via GitHub
gaborkaszab commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1955843549 ## core/src/main/java/org/apache/iceberg/BulkDeleteConsumer.java: ## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or mo

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-14 Thread via GitHub
gaborkaszab commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1955828374 ## core/src/main/java/org/apache/iceberg/BulkDeleteConsumer.java: ## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or mo

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-14 Thread via GitHub
gaborkaszab commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1955819540 ## core/src/main/java/org/apache/iceberg/BulkDeleteConsumer.java: ## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or mo

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-10 Thread via GitHub
amogh-jahagirdar commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1950075730 ## core/src/main/java/org/apache/iceberg/BulkDeleteConsumer.java: ## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + *

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-10 Thread via GitHub
amogh-jahagirdar commented on PR #11837: URL: https://github.com/apache/iceberg/pull/11837#issuecomment-2649544429 Sorry for the late followup, I'm taking a look! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the UR

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-10 Thread via GitHub
pvary commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1949864076 ## core/src/main/java/org/apache/iceberg/BulkDeleteConsumer.java: ## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more con

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-10 Thread via GitHub
steveloughran commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1949606394 ## core/src/main/java/org/apache/iceberg/BulkDeleteConsumer.java: ## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-10 Thread via GitHub
pvary commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1949131366 ## core/src/main/java/org/apache/iceberg/BulkDeleteConsumer.java: ## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more con

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-10 Thread via GitHub
pvary commented on code in PR #11837: URL: https://github.com/apache/iceberg/pull/11837#discussion_r1949121981 ## core/src/main/java/org/apache/iceberg/BulkDeleteConsumer.java: ## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more con

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-02-10 Thread via GitHub
gaborkaszab commented on PR #11837: URL: https://github.com/apache/iceberg/pull/11837#issuecomment-2647626948 @amogh-jahagirdar Would you mind taking a look. This came from a Slack discussion we had earlier. cc @pvary in case you have some capacity for this. Thanks! -- This is a

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-01-21 Thread via GitHub
gaborkaszab commented on PR #11837: URL: https://github.com/apache/iceberg/pull/11837#issuecomment-2604978058 Hi @amogh-jahagirdar , Would you mind taking a look? This PR came up with a conversation with you on Iceberg Slack. https://apache-iceberg.slack.com/archives/C03LG1D563F/p1733215

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-01-21 Thread via GitHub
gaborkaszab commented on PR #11837: URL: https://github.com/apache/iceberg/pull/11837#issuecomment-2604973773 > I'm going to suggest some tests of failure handling to see what happens there > > * missing file (all should ignore) > * putting a non-empty directory where one of the fi

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2025-01-15 Thread via GitHub
steveloughran commented on PR #11837: URL: https://github.com/apache/iceberg/pull/11837#issuecomment-2592695395 I'm going to suggest some tests of failure handling to see what happens there * missing file (all should ignore) * putting a non-empty directory where one of the file path

Re: [PR] Core: Bulk deletion in RemoveSnapshots [iceberg]

2024-12-20 Thread via GitHub
gaborkaszab commented on PR #11837: URL: https://github.com/apache/iceberg/pull/11837#issuecomment-2557371186 Slack discussion about this: https://apache-iceberg.slack.com/archives/C03LG1D563F/p1733215233582339 -- This is an automated message from the Apache Git Service. To respond to the