Ensure we return a sorted listing when using a local client #344

nicklan · 2024-09-19T01:10:29Z

This ensures that calls to list_files are returned sorted if a LocalFileSystem ObjectStore is used.

It's not clean for a few reasons:

We have to materialize the entire iter in order to sort it
We can't easily detect if we're local just by using type_of or similar. We have a foreign trait object that doesn't have an as_any on it, so we can force the reference into an Any which would allow us to use is, and we can't add an implementation of something like as_any because dyn ObjectStore isn't Sized. So this resorts to formating the object at creation (since ObjectStore requires Display) and checking if it starts with LocalFileSystem....

Adds a test that when using local client things come back sorted now. Without these changes the test failed.

I think we should merge (or something like it) and then also see about adding a list_sorted and list_with_offset_sorted to ObjectStore

nicklan · 2024-09-19T01:16:05Z

kernel/src/scan/mod.rs

@@ -627,10 +627,6 @@ mod tests {

    #[test]
    fn test_scan_data() {
-        use tracing_subscriber::EnvFilter;


this was left over that shouldn't be here, and was causing test failures because you shouldn't init in a test like this

codecov · 2024-09-19T01:17:38Z

Codecov Report

Attention: Patch coverage is 93.54839% with 6 lines in your changes missing coverage. Please review.

Project coverage is 78.34%. Comparing base (9b2e7e3) to head (af8c042).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/engine/default/filesystem.rs	91.83%	0 Missing and 4 partials ⚠️
kernel/src/snapshot.rs	87.50%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #344      +/-   ##
==========================================
+ Coverage   78.22%   78.34%   +0.12%     
==========================================
  Files          49       50       +1     
  Lines       10253    10341      +88     
  Branches    10253    10341      +88     
==========================================
+ Hits         8020     8102      +82     
- Misses       1783     1785       +2     
- Partials      450      454       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

scovich

Code seems fine, if we decide that ordered listing is the way to go. Given that S3 directory buckets don't support ordered listing, I wonder if we need to seriously consider hardening kernel against disorder.

HOWEVER -- everything I can think of would require materializing the listing result, and if we do that unconditionally we penalize stores that do provide ordered listing. Also, it's probably simpler overall to ensure the listing is ordered up front, so the rest of kernel doesn't have to worry about it?

kernel/src/engine/default/filesystem.rs

OussamaSaoudi-db

Looks good to me!

zachschuermann

lgtm just added a question on expanding this to all non-s3-normal-bucket object_store implementations

zachschuermann · 2024-09-25T17:14:04Z

kernel/src/engine/default/filesystem.rs

@@ -72,7 +80,14 @@ impl<E: TaskExecutor> FileSystemClient for ObjectStoreFileSystemClient<E> {
            }
        });

-        Ok(Box::new(receiver.into_iter()))
+        if self.is_local {


it seems like we might need to just detect if it "isn't S3 non-directory bucket" and then keep expanding from there? Seems safer to do a bit of extra sorting for now and just remove it as we discover different object_stores can do sorting? (or better yet we do some upstream fixes to expose a new API to communicate sorting)

This will be huge for building analytical engines, as it will leave them in a dilemma about whether to trust the iterators to return the sorted entries. Do you know if any conversations are going on upstream about this?

Yep! I added some context here #344 (comment). But I think we want to guarantee ordered from our listing.

I tend to agree with @zachschuermann here -- but it's probably important to at least have answers for AWS, GCP, and Azure if we're going that route?

Actually... thinking more... we don't currently rely on ordered listing, so unrecognized cloud stores will just keep today's bad behavior?

Another reason to assume sorting is needed unless proven otherwise: It's not clear that Azure supports an efficient list_with_offset, which makes me worry it also might not return ordered results:

Some stores, such as S3 and GCS, may be able to push offset down to reduce the number of network requests required

Yet another reason: object_store supports various adapters which would throw off name- or type-based checks, so any such check needs to fail-safe not fail-dangerous.

hackintoshrao · 2024-10-05T02:01:41Z

@scovich: Should I base the PR #322 on these changes? Are we ignoring the directory buckets listing, unsorted in S3 for now?
Can we assume that the listing iterators return sorted entries and that any need for materialization required for non-sorted stores will be handled the same way as in this PR?

scovich · 2024-10-05T15:08:46Z

@nicklan -- what is left for this to be able to merge? Are we concerned about the approach? Worried it's somehow incorrect? Something else?

nicklan · 2024-10-10T22:25:12Z

Sorry for the delay here! This this is ready now. I did some work to figure out if we need to do anything for non-local stores. As far as I can tell, aws, azure, and gcp all return sorted listings.

AWS: ListObjectsV2 states: "For general purpose buckets, ListObjectsV2 returns objects in lexicographical order based on their key names." (Directory buckets are out of scope for now)
Azure: Docs state here: "A listing operation returns an XML response that contains all or part of the requested list. The operation returns entities in alphabetical order."
GCP: The main doc doesn't indicate order, but this page does say: "This page shows you how to list the objects stored in your Cloud Storage buckets, which are ordered in the list lexicographically by name."

So it should just be local that we need for now. I've made a follow-up issue to check if the hdfs integration returns in order (I suspect it does) (TODO: Link issue)

zachschuermann

just a couple questions but seems fine :) and maybe we want to add the comment that you had about each cloud's storing as an actual code comment for posterity?

zachschuermann · 2024-10-11T01:29:57Z

kernel/src/engine/default/mod.rs

    }

    pub fn new(store: Arc<DynObjectStore>, prefix: Path, task_executor: Arc<E>) -> Self {
+        // HACK to check if we're using a LocalFileSystem from ObjectStore


make an issue for follow-up to fix in the future?

Well, I don't think there's a better way to do it, or I would have :)

zachschuermann · 2024-10-11T02:29:21Z

kernel/src/snapshot.rs

+        commit_files
+    );
+    // We assume listing returned ordered, we want reverse order
+    let commit_files = commit_files.into_iter().rev().collect();


curious: it says .rev() works on double-ended iterators - is that what's going on here? This still has to be O(N) right? makes me wonder if we lose that much just by sorting like we used to..?

I think reversing is still gonna be faster than a full sort. Or, at least not slower.

nicklan · 2024-10-11T20:06:37Z

maybe we want to add the comment that you had about each cloud's storing as an actual code comment for posterity?

Good call, done

kernel/src/engine/default/filesystem.rs

scovich · 2024-10-14T11:54:08Z

kernel/src/engine/default/filesystem.rs

@@ -72,7 +80,14 @@ impl<E: TaskExecutor> FileSystemClient for ObjectStoreFileSystemClient<E> {
            }
        });

-        Ok(Box::new(receiver.into_iter()))
+        if self.is_local {


I tend to agree with @zachschuermann here -- but it's probably important to at least have answers for AWS, GCP, and Azure if we're going that route?

Actually... thinking more... we don't currently rely on ordered listing, so unrecognized cloud stores will just keep today's bad behavior?

scovich · 2024-10-14T11:57:25Z

kernel/src/engine/default/filesystem.rs

@@ -72,7 +80,14 @@ impl<E: TaskExecutor> FileSystemClient for ObjectStoreFileSystemClient<E> {
            }
        });

-        Ok(Box::new(receiver.into_iter()))
+        if self.is_local {


Another reason to assume sorting is needed unless proven otherwise: It's not clear that Azure supports an efficient list_with_offset, which makes me worry it also might not return ordered results:

Some stores, such as S3 and GCS, may be able to push offset down to reduce the number of network requests required

scovich · 2024-10-14T12:02:19Z

kernel/src/engine/default/filesystem.rs

@@ -72,7 +80,14 @@ impl<E: TaskExecutor> FileSystemClient for ObjectStoreFileSystemClient<E> {
            }
        });

-        Ok(Box::new(receiver.into_iter()))
+        if self.is_local {


Yet another reason: object_store supports various adapters which would throw off name- or type-based checks, so any such check needs to fail-safe not fail-dangerous.

scovich · 2024-10-14T12:04:06Z

kernel/src/engine/default/mod.rs

+        // - Azure: Docs state
+        //   [here](https://learn.microsoft.com/en-us/rest/api/storageservices/enumerating-blob-resources):
+        //   "A listing operation returns an XML response that contains all or part of the requested
+        //   list. The operation returns entities in alphabetical order."


Nice find! I couldn't find that documented before.

But what about non-blob azure storage? Isn't there ADLS gen 1 and ADLS gen 2 as well? Or are they supersets of the blob store API?

Good question on adls gen 2. I think that it doesn't matter because alds gen 2 is built on blob store, so the rest apis should be the same.

zachschuermann

has_ordered_listing LGTM. highlighting concern from slack we will need to follow up on: if we have S3-compat filesystem e.g. minio but that doesn't return ordered listing then we wouldn't be able to trust s3:// url to mean ordered listing.

nicklan · 2024-10-18T17:43:32Z

Filed #410 to follow-up

nicklan marked this pull request as draft September 19, 2024 01:10

nicklan commented Sep 19, 2024

View reviewed changes

scovich approved these changes Sep 19, 2024

View reviewed changes

kernel/src/engine/default/filesystem.rs Outdated Show resolved Hide resolved

kernel/src/engine/default/filesystem.rs Show resolved Hide resolved

nicklan force-pushed the default-local-listing-sorted branch 2 times, most recently from 9513ed0 to 4b19ed3 Compare September 19, 2024 20:58

nicklan marked this pull request as ready for review September 19, 2024 21:16

nicklan requested review from zachschuermann and OussamaSaoudi-db September 19, 2024 21:16

OussamaSaoudi-db approved these changes Sep 19, 2024

View reviewed changes

nicklan added the merge hold Don't allow the PR to merge label Sep 19, 2024

zachschuermann approved these changes Sep 25, 2024

View reviewed changes

zachschuermann mentioned this pull request Oct 10, 2024

Fixing snapshot creation for earlier versions than the latest checkpoint #322

Open

github-actions bot added the breaking-change Change that will require a version bump label Oct 10, 2024

nicklan requested review from scovich and zachschuermann October 10, 2024 22:26

nicklan removed the merge hold Don't allow the PR to merge label Oct 10, 2024

zachschuermann approved these changes Oct 11, 2024

View reviewed changes

scovich reviewed Oct 14, 2024

View reviewed changes

Nick Lanham and others added 7 commits October 15, 2024 09:34

Ensure we return a sorted listing when using a local client

50ef862

lint

fefc6ec

dbg commit to try and fix windows

808ffe8

remove debugging code

897cf0a

sort_unstable

12aad92

fix for merge

27e349a

Just reverse the vec as we're assuming the listing is sorted now

2a84547

nicklan added 4 commits October 15, 2024 09:34

debug_assert that list_from returns sorted

0a22329

fmt

3b80415

add big comment

5198a39

switch to has_ordered_listing

2e03a28

nicklan force-pushed the default-local-listing-sorted branch from 63571a4 to 2e03a28 Compare October 15, 2024 23:04

zachschuermann approved these changes Oct 15, 2024

View reviewed changes

Merge branch 'main' into default-local-listing-sorted

341ac9c

Merge branch 'main' into default-local-listing-sorted

af8c042

nicklan merged commit 6205711 into delta-incubator:main Oct 18, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure we return a sorted listing when using a local client #344

Ensure we return a sorted listing when using a local client #344

nicklan commented Sep 19, 2024 •

edited

Loading

nicklan Sep 19, 2024

codecov bot commented Sep 19, 2024 •

edited

Loading

scovich left a comment

OussamaSaoudi-db left a comment

zachschuermann left a comment

zachschuermann Sep 25, 2024

hackintoshrao Oct 5, 2024 •

edited

Loading

nicklan Oct 10, 2024

scovich Oct 14, 2024

scovich Oct 14, 2024

scovich Oct 14, 2024

hackintoshrao commented Oct 5, 2024 •

edited

Loading

scovich commented Oct 5, 2024

nicklan commented Oct 10, 2024 •

edited

Loading

zachschuermann left a comment

zachschuermann Oct 11, 2024

nicklan Oct 11, 2024

zachschuermann Oct 11, 2024

nicklan Oct 11, 2024

nicklan commented Oct 11, 2024

scovich Oct 14, 2024

scovich Oct 14, 2024

scovich Oct 14, 2024

scovich Oct 14, 2024

nicklan Oct 15, 2024

zachschuermann left a comment

nicklan commented Oct 18, 2024

Ensure we return a sorted listing when using a local client #344

Ensure we return a sorted listing when using a local client #344

Conversation

nicklan commented Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

scovich left a comment

Choose a reason for hiding this comment

OussamaSaoudi-db left a comment

Choose a reason for hiding this comment

zachschuermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackintoshrao Oct 5, 2024 • edited Loading

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

hackintoshrao commented Oct 5, 2024 • edited Loading

scovich commented Oct 5, 2024

nicklan commented Oct 10, 2024 • edited Loading

zachschuermann 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

nicklan commented Oct 11, 2024

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

zachschuermann left a comment

Choose a reason for hiding this comment

nicklan commented Oct 18, 2024

nicklan commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

hackintoshrao Oct 5, 2024 •

edited

Loading

hackintoshrao commented Oct 5, 2024 •

edited

Loading

nicklan commented Oct 10, 2024 •

edited

Loading