Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: omit common prefixes in azure list_objs #683

Merged
merged 2 commits into from
Jul 11, 2022

Conversation

roeap
Copy link
Collaborator

@roeap roeap commented Jul 10, 2022

Description

Adls gen2 with hierarchical namespaces enabled has a notion of "directories" in an object store, this is in contrast to other object store implementations. This PR brings the behaviour in line with other storage backend implementations in this crate.

leaving this as a draft to see what else needs to be done for azure to be en par with local and s3 in #682. All backend related vacuum tests for azure are passing with this in #682.

Related Issue(s)

towards #647

Documentation

@Blajda
Copy link
Collaborator

Blajda commented Jul 10, 2022

Hi @roeap, The vacuum tests panic with the following error message:

`thread 'test_non_partitioned_table' panicked at 'called `Result::unwrap()` on an `Err` value: DeltaTable(Generic("Parent path `adls2://deltarstests/delta-rs-test-1657486937-37104` is not a prefix of path `adls2://delta-rs-test-1657486937-37104/adls2://deltarstests/delta-rs-test-1657486937-37104`"))', rust/tests/vacuum_test.rs:123:35`

@roeap roeap force-pushed the fix-azure-common-prefixes branch from 9ff1705 to 0d2ad79 Compare July 10, 2022 21:44
@roeap
Copy link
Collaborator Author

roeap commented Jul 10, 2022

@Blajda - how about now? :)

@roeap roeap force-pushed the fix-azure-common-prefixes branch 2 times, most recently from 430faed to c0dc9f9 Compare July 10, 2022 21:50
@Blajda
Copy link
Collaborator

Blajda commented Jul 10, 2022

Still fails due to the prefix but it's closer. Just missing the account name

thread 'test_ignored_files' panicked at 'called `Result::unwrap()` on an `Err` value: DeltaTable(Generic("Parent path `adls2://deltarstests/delta-rs-test-1657490502-18515` is not a prefix of path `adls2://delta-rs-test-1657490502-18515/.dotfile`"))', rust/tests/vacuum_test.rs:219:35

@roeap roeap force-pushed the fix-azure-common-prefixes branch from c0dc9f9 to e7c616e Compare July 10, 2022 22:13
@roeap
Copy link
Collaborator Author

roeap commented Jul 10, 2022

One more iteration ...

@Blajda
Copy link
Collaborator

Blajda commented Jul 10, 2022

Yeah that works. Everything passes now expect for test_non_managed_files all the backends are aligned now 🥳

@roeap roeap marked this pull request as ready for review July 10, 2022 22:24
@roeap roeap enabled auto-merge (squash) July 10, 2022 22:26
Copy link
Collaborator

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but had a readability suggestion that I think is worth including.

modified: p.last_modified,
size: Some(p.content_length),
})
Ok(paths) => Either::Left(stream::iter(paths.into_iter().flat_map(|p| {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh I didn't know flat_map worked on Option. Would have guessed to use filter_map usually, and it might be a bit clearer?

Suggested change
Ok(paths) => Either::Left(stream::iter(paths.into_iter().flat_map(|p| {
Ok(paths) => Either::Left(stream::iter(paths.into_iter().filter_map(|p| {

@roeap roeap requested a review from wjones127 July 11, 2022 07:03
@roeap roeap merged commit 9229dee into delta-io:main Jul 11, 2022
@roeap roeap deleted the fix-azure-common-prefixes branch July 11, 2022 20:12
@Blajda Blajda mentioned this pull request Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants