fix: add handling for unmanaged files to vacuum command #1817

Jan-Schweizer · 2023-11-07T17:34:07Z

Description

Adds handling for unmanaged files to the VACUUM command. The VACUUM command should correctly handle files that are not managed by a Delta table, e.g. files and directories that start with _ or ..
Additionally fixes test test_non_managed_files.

Related Issue(s)

closes #227 (Unit and integration tests for the vacuum command)

github-actions · 2023-11-07T17:34:29Z

ACTION NEEDED

delta-rs follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

The new version of this crate properly sets a lease duration such that the locks can actually expire

# Description Introduce a `LogStore` abstraction to channel all log store reads and writes through a single place. This is supposed to allow implementations with more sophisticated locking mechanisms that do not rely on atomic rename semantics for the underlying object store. This does not change any functionality - it reorganizes read operations and commits on the delta commit log to be funneled through the respective methods of `LogStore`. ## Rationale The goal is to align the implementation of multi-cluster writes for Delta Lake on S3 with the one provided by the original `delta` library, enabling multi-cluster writes with some writers using Spark / Delta library and other writers using `delta-rs` For an overview of how it's done in delta, please see: 1. Delta [blog post](https://delta.io/blog/2022-05-18-multi-cluster-writes-to-delta-lake-storage-in-s3/) (high-level concept) 2. Associated Databricks [design doc](https://docs.google.com/document/d/1Gs4ZsTH19lMxth4BSdwlWjUNR-XhKHicDvBjd2RqNd8/edit#heading=h.mjjuxw9mcz9h) (detailed read) 3. [S3DynamoDbLogStore.java](https://github.com/delta-io/delta/blob/master/storage-s3-dynamodb/src/main/java/io/delta/storage/S3DynamoDBLogStore.java)(content warning: Java code behind this link) This approach requires readers of a delta table to "recover" unfinished commits from writers - as a result, reading and writing is combined in a single interface, which in this PR is modeled after [LogStore.java](https://github.com/delta-io/delta/blob/master/storage/src/main/java/io/delta/storage/LogStore.java). Currently in `delta-rs`, read path for commits is implemented directly in `DeltaTable`, and there's no mechanism to implement storage-specific behavior like interacting with DynamoDb. --------- Co-authored-by: Robert Pack <42610831+roeap@users.noreply.github.com>

wjones127

Looks good. Thanks for finishing this.

wjones127

It looks like there is a test assertion that just needs to be updated, since _change_data and _delta_index are exceptions in is_hidden_directory().

roeap · 2023-11-10T06:11:11Z

We may have some additional aspects into consideration when selecting the files to be vacuumed. As it stands I think we may accidentally delete deletion vector files for files that are still part of the log. We should however be able to get the corresponding files names from the add action and add them to the managed files.

Some .crc files might also get deleted pre-maturely, but these are always optional, so no harm to the table data.

Update the Slack link to https://go.delta.io/slack

…tion vector files

Jan-Schweizer · 2023-11-11T16:57:44Z

crates/deltalake-core/src/operations/vacuum.rs

+                    None
+                };
+            }))
+            .collect::<HashSet<String>>();


Since the deletion vector file path is being constructed on the fly by the absolut_path function, it now must be a HashSet<String> instead of HashSet<&str>. This also means that we now have to clone the String of the add and remove path. Is this fine or is there a better way to handle this?
Since the add/remove path potentially needs to be converted to an absolute path, I need to construct a String any way

Also, this nested handling of the deletion vector paths doesn't look nice, can you show me a better/more idiomatic way to achieve this?

Jan-Schweizer · 2023-11-11T16:59:47Z

crates/deltalake-core/src/operations/vacuum.rs

-    Ok((path_name.starts_with('.') || path_name.starts_with('_'))
-        && !path_name.starts_with("_delta_index")
-        && !path_name.starts_with("_change_data")
+fn is_hidden_file(partition_columns: &[String], path: &Path) -> Result<bool, DeltaTableError> {


I renamed this to is_hidden_file since we are not only ignoring directories but also single files

Jan-Schweizer · 2023-11-15T15:37:35Z

crates/deltalake-core/src/operations/vacuum.rs

+    /// Whether a file is contained within the set of managed files.
+    fn is_file_managed(&self, managed_files: &HashSet<String>, file: &Path) -> bool {
+        return if is_absolute_path(file.as_ref()) {
+            managed_files.contains(file.as_ref())
+        } else {
+            let path = format!("{}{}", self.log_store.root_uri(), file.as_ref());
+            managed_files.contains(&path)
+        };
+    }


Can the location of an object in the log_store ever be absolute? Or is it always relative to the table root? If so, then we wouldn't need the is_absolute_path check

@Jan-Schweizer the location can be an absolute URL, and that's the case with shallow cloned tables. The add action describes the path key.

So for better or worse, we need to check for an absolute path 😄

rtyler · 2023-11-30T06:34:52Z

crates/deltalake-core/src/operations/vacuum.rs

+    /// Whether a file is contained within the set of managed files.
+    fn is_file_managed(&self, managed_files: &HashSet<String>, file: &Path) -> bool {
+        return if is_absolute_path(file.as_ref()) {
+            managed_files.contains(file.as_ref())
+        } else {
+            let path = format!("{}{}", self.log_store.root_uri(), file.as_ref());
+            managed_files.contains(&path)
+        };
+    }


@Jan-Schweizer the location can be an absolute URL, and that's the case with shallow cloned tables. The add action describes the path key.

So for better or worse, we need to check for an absolute path 😄

# Description I have made a bunch of improvements to fix the overall structure due to example sections not being consistent. I've also enabled some extra features. Fixed also the issue of some classes/functions not being shown properly.

# Description This PR prunes each merge bin with only 1 file, even though there are multiple merge bins. # Related Issue(s) - closes delta-io#1901 # Documentation This PR adds test_idempotent_with_multiple_bins() for testing.

# Description Prepare for next release # Related Issue(s)  # Documentation

# Description This is a continuation of the discussion in the delta-io#1917 Getting rid of panic in the library crate in favor of returning an error so lib users could handle it in a way they see it Test changes accordingly Co-authored-by: Robert Pack <42610831+roeap@users.noreply.github.com>

Add handling for unmanaged files to vacuum command

1dc65b3

Jan-Schweizer requested review from wjones127, roeap and rtyler as code owners November 7, 2023 17:34

Jan-Schweizer changed the title ~~Add handling for unmanaged files to vacuum command~~ fix: Add handling for unmanaged files to vacuum command Nov 7, 2023

Jan-Schweizer changed the title ~~fix: Add handling for unmanaged files to vacuum command~~ fix: add handling for unmanaged files to vacuum command Nov 7, 2023

Jan-Schweizer and others added 3 commits November 7, 2023 18:55

collapse nested if block

e59bb34

chore: upgrade to the latest dynamodb-lock crate

90b7741

The new version of this crate properly sets a lease duration such that the locks can actually expire

Jan-Schweizer requested a review from fvaleye as a code owner November 9, 2023 12:35

github-actions bot added the binding/python Issues for the Python package label Nov 9, 2023

Merge branch 'main' into fix/handle-unmanaged-files-in-vacuum-command

8e64a0d

github-actions bot removed the binding/python Issues for the Python package label Nov 9, 2023

wjones127 previously approved these changes Nov 10, 2023

View reviewed changes

wjones127 reviewed Nov 10, 2023

View reviewed changes

roeap and others added 4 commits November 10, 2023 15:14

fix: use correct folder for auto assigned labels

7559c24

fix: run integration tests in CI

da6e438

Update README.md

140f949

Update the Slack link to https://go.delta.io/slack

Correctly handle hidden files in _change_data and _delta_index & dele…

a327fa8

…tion vector files

Jan-Schweizer dismissed wjones127’s stale review via a327fa8 November 11, 2023 16:52

github-actions bot added binding/rust Issues for the Rust crate crate/core labels Nov 11, 2023

Merge branch 'main' into fix/handle-unmanaged-files-in-vacuum-command

b40f276

Jan-Schweizer commented Nov 11, 2023

View reviewed changes

Jan-Schweizer requested a review from wjones127 November 11, 2023 17:04

Jan-Schweizer added 2 commits November 15, 2023 12:00

Merge branch 'main' into fix/handle-unmanaged-files-in-vacuum-command

a358f06

Fix paths for managed files

752773a

Jan-Schweizer commented Nov 15, 2023

View reviewed changes

Jan-Schweizer and others added 2 commits November 29, 2023 09:41

Merge branch 'main' into fix/handle-unmanaged-files-in-vacuum-command

dc74dcc

Merge branch 'main' into fix/handle-unmanaged-files-in-vacuum-command

96a5e0a

rtyler previously approved these changes Nov 30, 2023

View reviewed changes

ion-elgreco and others added 8 commits November 30, 2023 08:36

Merge branch 'main' into fix/handle-unmanaged-files-in-vacuum-command

5ea77fd

fix: prune each merge bin with only 1 file (delta-io#1902)

f90b48c

# Description This PR prunes each merge bin with only 1 file, even though there are multiple merge bins. # Related Issue(s) - closes delta-io#1901 # Documentation This PR adds test_idempotent_with_multiple_bins() for testing.

chore: update python version (delta-io#1934)

d518f40

# Description Prepare for next release # Related Issue(s)  # Documentation

Support os.PathLike for table references

2733f3d

add type param

f4b9e91

Happify linter

b946d07

Jan-Schweizer dismissed rtyler’s stale review via b946d07 December 2, 2023 12:29

Jan-Schweizer requested a review from ion-elgreco as a code owner December 2, 2023 12:29

github-actions bot added the binding/python Issues for the Python package label Dec 2, 2023

Merge branch 'main' into fix/handle-unmanaged-files-in-vacuum-command

d4642bf

github-actions bot removed the binding/python Issues for the Python package label Dec 2, 2023

Jan-Schweizer and others added 2 commits December 14, 2023 19:41

Merge branch 'main' into fix/handle-unmanaged-files-in-vacuum-command

04e3811

Merge branch 'main' into fix/handle-unmanaged-files-in-vacuum-command

27c2b53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add handling for unmanaged files to vacuum command #1817

fix: add handling for unmanaged files to vacuum command #1817

Jan-Schweizer commented Nov 7, 2023 •

edited

github-actions bot commented Nov 7, 2023

wjones127 left a comment

wjones127 left a comment

roeap commented Nov 10, 2023

Jan-Schweizer Nov 11, 2023 •

edited

Jan-Schweizer Nov 11, 2023

Jan-Schweizer Nov 15, 2023

rtyler Nov 30, 2023

rtyler Nov 30, 2023

fix: add handling for unmanaged files to vacuum command #1817

Are you sure you want to change the base?

fix: add handling for unmanaged files to vacuum command #1817

Conversation

Jan-Schweizer commented Nov 7, 2023 • edited

Description

Related Issue(s)

github-actions bot commented Nov 7, 2023

wjones127 left a comment

Choose a reason for hiding this comment

wjones127 left a comment

Choose a reason for hiding this comment

roeap commented Nov 10, 2023

Jan-Schweizer Nov 11, 2023 • edited

Choose a reason for hiding this comment

Jan-Schweizer Nov 11, 2023

Choose a reason for hiding this comment

Jan-Schweizer Nov 15, 2023

Choose a reason for hiding this comment

rtyler Nov 30, 2023

Choose a reason for hiding this comment

rtyler Nov 30, 2023

Choose a reason for hiding this comment

Jan-Schweizer commented Nov 7, 2023 •

edited

Jan-Schweizer Nov 11, 2023 •

edited