Add a custom implementation `LocalFileSystem::list_with_offset` #7019

corwinjoy · 2025-01-24T23:39:19Z

Which issue does this PR close?

Closes #7018.

Rationale for this change

Performance enhancement needed for network file system as described in the PR.

What changes are included in this PR?

Add an implementation LocalFileSystem::list_with_offset so that it can intelligently scan files and drastically reduce the number of statx system calls. Basically, we add a prefilter to pull only what is needed in-place.

Are there any user-facing changes?

None.

Co-authored-by: Adam Reeve <[email protected]>

corwinjoy · 2025-01-24T23:41:31Z

The initial work for this PR was done by @dplasto, who reported the issue and provided an initial implementation. Further suggestions were provided by @adamreeve .

tustvold · 2025-01-25T16:16:31Z

It would appear this is legitimately failing CI

alamb · 2025-01-25T17:13:53Z

Thanks @corwinjoy and @tustvold -- marking this as a draft as we work through the test failures

corwinjoy · 2025-01-25T23:11:50Z

I'm not sure what is happening with the CI. It seems to be failing formatting. But, I ran 'cargo format' and clippy as specified in Contributing.md so I am not sure what is wrong.

…

On Sat, Jan 25, 2025, 9:14 AM Andrew Lamb ***@***.***> wrote: Thanks @corwinjoy <https://github.com/corwinjoy> and @tustvold <https://github.com/tustvold> -- marking this as a draft as we work through the test failures — Reply to this email directly, view it on GitHub <#7019 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADBF42SFNON4OYCDVEG36AT2MPA6RAVCNFSM6AAAAABV2WCNBSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMJUGAZTINRXGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

etseidl · 2025-01-26T02:07:51Z

I'm not sure what is happening with the CI. It seems to be failing formatting. But, I ran 'cargo format' and clippy as specified in Contributing.md so I am not sure what is wrong.

Are you running from the arrow-rs directory or the object_store directory. When I run cargo fmt --all -- --check from the object_store directory I get similar errors to CI.

corwinjoy · 2025-01-26T02:59:58Z

Ah, thanks for the clarification @etseidl! From the guidelines I didn't realize that I had to run these commands in the object-store rather than the arrow-rs directory. Doing that, I do get suggested changes which I have applied. Hopefully this will clear the CI checks.

alamb · 2025-01-27T09:44:14Z

Ah, thanks for the clarification @etseidl! From the guidelines I didn't realize that I had to run these commands in the object-store rather than the arrow-rs directory. Doing that, I do get suggested changes which I have applied. Hopefully this will clear the CI checks.

The emulator tests appear to be failing: https://github.com/apache/arrow-rs/actions/runs/12970828706/job/36180799417?pr=7019

---- local::tests::test_path_with_offset stdout ----
thread 'local::tests::test_path_with_offset' panicked at src/local.rs:1440:66:
called `Result::unwrap()` on an `Err` value: Generic { store: "LocalFileSystem", source: UnableToCanonicalize { path: "../testing/data/arrow-ipc-file", source: Os { code: 2, kind: NotFound, message: "No such file or directory" } } }
stack backtrace:

…or testing rather than pointing to existing dir.

corwinjoy · 2025-01-27T23:49:27Z

OK. I have changed this test to use a temporary test and files which should make it more portable. We will see what the emulator says about this.

alamb · 2025-01-29T12:20:42Z

OK. I have changed this test to use a temporary test and files which should make it more portable. We will see what the emulator says about this.

Looks like the tests are good

alamb · 2025-01-29T12:21:21Z

This PR looks ready for review to me. @corwinjoy shall we mark it as ready for review?

corwinjoy · 2025-01-29T16:53:00Z

@alamb Yes please. Mark it as ready for review.

alamb

Thank you @corwinjoy -- I think this code is better than what is on main.

I am somewhat concerned that the listing with offset is going to result in non deterministic results but I don't think this PR makes that any better/worse than what is on main

Thanks again

alamb · 2025-02-06T18:22:51Z

object_store/src/local.rs

+            None => config.root.to_file_path().unwrap(),
+        };
+
+        let walkdir = WalkDir::new(root_path)


I did some digging and I don't think WalkDir guarantees anything about the sort order of the directory entries

thus if the OS returns a different order on the next call to list_with_offset this may end up with a different result

However I think the existing default impl of list_with_offset (which just calls list() and takes the offset) has the same problem so I don't think it needs to be fixed in this PR

It does look to me like you can explicitly set an order via https://docs.rs/walkdir/latest/walkdir/struct.WalkDir.html#method.sort_by

@alamb I agree with all this and thanks for the helpful comments! I think your concern about not having a deterministic order is a good one and agree this is the existing behavior. I also think we should change this, but propose we do that in a follow-up PR since it does change the behavior and could (potentially) break an assumption somewhere. I think we can just use https://docs.rs/walkdir/latest/walkdir/struct.WalkDir.html#method.sort_by_file_name
which should be fine and I believe matches what the offset filter expects.

alamb · 2025-02-06T18:24:44Z

object_store/src/local.rs

+            .map(|x| String::from(x.filename().unwrap()))
+            .collect();
+
+        // Check result with direct filesystem read


I think this depends on the file system returning the same order each time. Again this is probably ok but it seems like it could result in a non deterministic test failure over time

Actually, it does not depend on this because later, in line 1485 we sort both result sets before comparing. So the initial sort order that gets returned does not matter.

I was thinking that if the underlying directory list was different, then the values after skipping the first 10 (for example) might be different, even if we sorted the values that came out.

But since this doesn't seem to happen in practice I am not going to worry too much about it

I've not had time to review this in detail but a couple of points that may or may not be relevant:

We do not guarantee the order of list results - https://docs.rs/object_store/latest/object_store/trait.ObjectStore.html#tymethod.list

Filesystems on many systems do not yield a consistent ordering - it is normally stable in the absence of mutation, but this isn't a strict guarantee

Or to phrase it either way, we shouldn't need to sort, nor should we rely on data being sorted

For context on why we don't guarantee any ordering, it is because many stores, e.g. S3 Express, don't

Makes sense -- thank you

To be clear I don't think this PR changes anything about the object_store operation in the face of inconsistent ordering between calls (but I was speculating on what would happen if the ordering returned by the underlying systems was different between calls)

object_store/src/local.rs

alamb · 2025-02-07T12:30:57Z

I'll plan to merge this PR in a day or two unless there are any more comments or anyone would like longer to review

alamb · 2025-02-08T16:27:44Z

Thanks again @corwinjoy

corwinjoy and others added 7 commits January 23, 2025 17:16

Initial change from Daniel.

8149625

Upgrade unit test to be more generic.

f767a2b

Add comments on why we have filter

7edce5b

Cleanup unit tests.

85adfce

Update object_store/src/local.rs

0f55193

Co-authored-by: Adam Reeve <[email protected]>

Add changes suggested by Adam.

2010783

Cleanup match error.

0059956

github-actions bot added the object-store Object Store Interface label Jan 24, 2025

alamb marked this pull request as draft January 25, 2025 17:13

corwinjoy added 2 commits January 25, 2025 18:53

Apply formatting changes suggested by cargo +stable fmt --all.

8ba9a89

Apply cosmetic changes suggested by clippy.

e58e10f

Upgrade test_path_with_offset to create temporary directory + files f…

4e3a697

…or testing rather than pointing to existing dir.

alamb marked this pull request as ready for review January 29, 2025 21:53

alamb approved these changes Feb 6, 2025

View reviewed changes

alamb merged commit 5bf4a6c into apache:main Feb 8, 2025
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a custom implementation `LocalFileSystem::list_with_offset` #7019

Add a custom implementation `LocalFileSystem::list_with_offset` #7019

corwinjoy commented Jan 24, 2025

corwinjoy commented Jan 24, 2025

tustvold commented Jan 25, 2025

alamb commented Jan 25, 2025

corwinjoy commented Jan 25, 2025 via email

etseidl commented Jan 26, 2025

corwinjoy commented Jan 26, 2025 •

edited

Loading

alamb commented Jan 27, 2025

corwinjoy commented Jan 27, 2025

alamb commented Jan 29, 2025

alamb commented Jan 29, 2025

corwinjoy commented Jan 29, 2025

alamb left a comment

alamb Feb 6, 2025

alamb Feb 6, 2025

corwinjoy Feb 7, 2025

alamb Feb 6, 2025

corwinjoy Feb 7, 2025

alamb Feb 7, 2025

tustvold Feb 7, 2025 •

edited

Loading

alamb Feb 7, 2025

alamb commented Feb 7, 2025

alamb commented Feb 8, 2025

Add a custom implementation LocalFileSystem::list_with_offset #7019

Add a custom implementation LocalFileSystem::list_with_offset #7019

Conversation

corwinjoy commented Jan 24, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

corwinjoy commented Jan 24, 2025

tustvold commented Jan 25, 2025

alamb commented Jan 25, 2025

corwinjoy commented Jan 25, 2025 via email

etseidl commented Jan 26, 2025

corwinjoy commented Jan 26, 2025 • edited Loading

alamb commented Jan 27, 2025

corwinjoy commented Jan 27, 2025

alamb commented Jan 29, 2025

alamb commented Jan 29, 2025

corwinjoy commented Jan 29, 2025

alamb left a comment

Choose a reason for hiding this comment

alamb Feb 6, 2025

Choose a reason for hiding this comment

alamb Feb 6, 2025

Choose a reason for hiding this comment

corwinjoy Feb 7, 2025

Choose a reason for hiding this comment

alamb Feb 6, 2025

Choose a reason for hiding this comment

corwinjoy Feb 7, 2025

Choose a reason for hiding this comment

alamb Feb 7, 2025

Choose a reason for hiding this comment

tustvold Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

alamb Feb 7, 2025

Choose a reason for hiding this comment

alamb commented Feb 7, 2025

alamb commented Feb 8, 2025

Add a custom implementation `LocalFileSystem::list_with_offset` #7019

Add a custom implementation `LocalFileSystem::list_with_offset` #7019

corwinjoy commented Jan 26, 2025 •

edited

Loading

tustvold Feb 7, 2025 •

edited

Loading