Implement filesystem check #1103

Blajda · 2023-01-26T03:33:57Z

Description

Implementation of the filesystem check operation.

The implementation is fairly straight forward with a HEAD call being made for each active file to check if it exists.
A remove action is then made for each file that is orphaned.

An alternative solution is instead to maintain a hashset with all active files and then recursively list all files. If the file exists then remove from the set. All remaining files in the set are then considered orphaned.

Looking for feedback and if the second approach is preferred I can make the changes

Related Issue(s)

closes Support FSCK REPAIR TABLE Operation #1092

houqp · 2023-01-27T05:21:50Z

Instead of heading each individual file, wouldn't it be more efficient to issue an prefixed list call to gather all current files in the table? This way, we can reduce API call count by about 1000x.

Blajda · 2023-01-28T21:11:45Z

@houqp I modified the process to instead use a list call but I'm not quite sure on how I would take advantage of prefixes in this case. For partitioned tables it could perform a list per partition path but I think non-partitioned tables would still require a full list on the root.

For non-partitioned tables would it be possible to perform a list without including the files in _delta_log?

houqp · 2023-01-29T22:06:04Z

@Blajda the prefix filter can only be applied with the table root unfortunately, so it will always include the _detal_log folder unfortunately. The main benefit it provides is we can restrict the listing to the table we are running the operation on, not all other tables stored in the same bucket.

houqp · 2023-01-29T22:06:39Z

@roeap @wjones127 want to take a final look?

wjones127

Thank you for adding this! I've been thinking having an operation like this would let me sleep better at night :)

I think we should address the case where data files are outside the root, either explicitly not supporting or using HEAD for those ones. Also noticed a few minor issues.

rust/src/operations/filesystem_check.rs

wjones127 · 2023-01-30T00:14:21Z

rust/src/operations/filesystem_check.rs

+
+impl FileSystemCheckBuilder {
+    /// Create a new [`FileSystemCheckBuilder`]
+    pub fn new(store: Arc<DeltaObjectStore>, state: DeltaTableState) -> Self {


Should we validate that the provided state is the latest version of the table? Since I don't think this is a valid operation for earlier states? or maybe that is an overall issue with the operations module?

I think dry run of this operation would be helpful for diagnosing issues that arise from a combination of vacuum + time travel. If executed on a older version I do expect it to fail during the commit operation.
I can add a test to demonstrate that.

rust/tests/command_filesystem_check.rs

wjones127 · 2023-01-30T00:36:00Z

rust/src/operations/filesystem_check.rs

+            files_to_check.insert(active.path.to_owned(), active);
+        });
+
+        let mut files = self.store.list(None).await?;


Delta Tables are allowed to have data files outside the table root. Currently, if that is the case, this operation will remove them all from the log. I see two options:

Call HEAD on each file outside the root to check if they exist.

Error if we detect any files outside the root.

Makes sense. Do any examples of tables that use absolute paths?
I just checked the protocol and noticed absolute paths are encoded as defined in rfc2396

It states the following

An absolute URI contains the name of the scheme being used (<scheme>) followed by a colon (":") and then a string (the <scheme-specific- part>) whose interpretation depends on the scheme.

So I can segment paths based of it the scheme is set on the path and then use the correct api call for each.

Oh that's a good point! Checking for the scheme at the beginning makes sense.

We don't have any example tables right now. We don't generally support them in most operations, but we should try to keep them in mind. Hard to have example tables without setting up a public S3 bucket or something.

I'm not comfortable with supporting absolute paths without having some integration tests. Currently the operation will fail when an absolute path is encountered.

Add wjones127 suggestions Co-authored-by: Will Jones <[email protected]>

rust/src/operations/filesystem_check.rs

Blajda · 2023-02-02T01:48:05Z

@wjones127 I updated the PR to fail when an absolute path exists in the Delta log. Also added an additional test to demonstrate the operation will only work on the latest table version.

roeap

Thanks @Blajda - one small question along the lines of hoping to eventually support absolute paths in the delta log. Otherwise looks great!

roeap · 2023-02-02T06:09:38Z

rust/src/operations/filesystem_check.rs

+fn is_absolute_path(path: &str) -> DeltaResult<bool> {
+    match Url::parse(path) {
+        Ok(_) => Ok(true),
+        Err(ParseError::RelativeUrlWithoutBase) => Ok(false),
+        Err(_) => Err(DeltaTableError::Generic(format!(
+            "Unable to parse path: {}",
+            &path
+        ))),
+    }
+}


I think this will not work for local paths. One pattern we apply in object_store and here is to try and cannonicalize the path and if it fails proceed with url parsing. e.g.

delta-rs/rust/src/builder.rs

Lines 355 to 358 in a60129b

if let Ok(path) = std::fs::canonicalize(table_uri) {

return Url::from_directory_path(path)

.map_err(|_| DeltaTableError::InvalidTableLocation(table_uri.to_string()));

}

I don't follow on why it would not work on local paths. Paths in the Delta log must follow rfc2396 which states absolute paths must have a scheme.
Do you have a example that would cause the unit test for this function to fail?

well .. in that case I am wrong! If that paths are required to have a scheme, then url parsing should always work for valid paths ... The windows case is also covered in your tests, so all is well :).

# Description Implementation of the filesystem check operation. The implementation is fairly straight forward with a HEAD call being made for each active file to check if it exists. A remove action is then made for each file that is orphaned. An alternative solution is instead to maintain a hashset with all active files and then recursively list all files. If the file exists then remove from the set. All remaining files in the set are then considered orphaned. Looking for feedback and if the second approach is preferred I can make the changes # Related Issue(s) - closes delta-io#1092 --------- Co-authored-by: Will Jones <[email protected]>

github-actions bot added binding/rust Issues for the Rust crate rust labels Jan 26, 2023

Implement filesystem check

5d32c56

Blajda force-pushed the fsck branch from 877c4c6 to 5d32c56 Compare January 26, 2023 03:40

Blajda added 4 commits January 26, 2023 20:20

Allow HTTP for AWS when useing test_utils

0747602

Remove unused imports

e944633

Allow unsafe rename for s3 test

18e7974

Set lock client to none for s3 test

e90ba53

Blajda added 2 commits January 28, 2023 15:30

Reconcile by using list instead of head

d42eb78

Update test utils to include partitioned table

48a460a

Blajda marked this pull request as ready for review January 28, 2023 20:56

Blajda requested review from houqp, xianwill, wjones127, fvaleye, roeap, rtyler and mosyp as code owners January 28, 2023 20:56

houqp previously approved these changes Jan 29, 2023

View reviewed changes

wjones127 requested changes Jan 30, 2023

View reviewed changes

Apply suggestions from code review

85886a5

Add wjones127 suggestions Co-authored-by: Will Jones <[email protected]>

Blajda dismissed houqp’s stale review via 85886a5 January 30, 2023 01:53

wjones127 reviewed Jan 30, 2023

View reviewed changes

rust/src/operations/filesystem_check.rs Outdated Show resolved Hide resolved

wjones127 and others added 3 commits January 29, 2023 19:31

Update rust/src/operations/filesystem_check.rs

3dc4b67

Resolve merge conflicts

7cbe4fa

Resolve clippy errors

558e890

Resolve clippy errors 2

46841cc

Blajda force-pushed the fsck branch from 8cb0842 to 46841cc Compare January 30, 2023 04:18

Blajda marked this pull request as draft January 30, 2023 04:31

Error on existence of absolute paths

9d19ef3

Blajda marked this pull request as ready for review February 2, 2023 01:39

roeap reviewed Feb 2, 2023

View reviewed changes

roeap approved these changes Feb 3, 2023

View reviewed changes

wjones127 approved these changes Feb 3, 2023

View reviewed changes

wjones127 merged commit ddf4f43 into delta-io:main Feb 3, 2023

Blajda deleted the fsck branch February 3, 2023 23:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement filesystem check #1103

Implement filesystem check #1103

Blajda commented Jan 26, 2023

houqp commented Jan 27, 2023

Blajda commented Jan 28, 2023

houqp commented Jan 29, 2023

houqp commented Jan 29, 2023

wjones127 left a comment

wjones127 Jan 30, 2023

Blajda Jan 30, 2023

wjones127 Jan 30, 2023

Blajda Jan 30, 2023

wjones127 Jan 30, 2023

Blajda Feb 2, 2023

Blajda commented Feb 2, 2023

roeap left a comment

roeap Feb 2, 2023

Blajda Feb 3, 2023 •

edited

Loading

roeap Feb 3, 2023

	if let Ok(path) = std::fs::canonicalize(table_uri) {
	return Url::from_directory_path(path)
	.map_err(\|_\| DeltaTableError::InvalidTableLocation(table_uri.to_string()));
	}

Implement filesystem check #1103

Implement filesystem check #1103

Conversation

Blajda commented Jan 26, 2023

Description

Related Issue(s)

houqp commented Jan 27, 2023

Blajda commented Jan 28, 2023

houqp commented Jan 29, 2023

houqp commented Jan 29, 2023

wjones127 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

Blajda commented Feb 2, 2023

roeap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blajda Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blajda Feb 3, 2023 •

edited

Loading