Avoid metadata lookup for LocalFileSystem::read_ranges and chunked_stream

[Dandandan]

Which issue does this PR close?

• Closes Avoid reading metadata for LocalFileSystem::read_ranges (and other methods) #614

Rationale for this change

This is unnecessary overhead.

What changes are included in this PR?

Only error once the file operation errors.

Are there any user-facing changes?

[Dandandan]

Doing some testing, it seems that avoiding File::open does make a difference.

Perhaps DataFusion can implement a LocalStorage that stores / caches the file descriptors for a query.

[alamb]

The emulator test doesn't look related to this PR

I think these changes make sense, but

1. There is some non trivial duplication
2. There is still a call open_file

I have some ideas on both counts. Let me propose a PR

[Dandandan]

1. There is some non trivial duplication

Agreed

There is still a call open_file

You mean outside get_ranges or...? At least as part of get_ranges I don't see anymore a metadata request.

[alamb]

1. There is some non trivial duplication

Agreed

As I played around with this I found that the challenge is that the metadata error is ignored sometimes when desired to get a better error message, but sometimes is needed. I haven't got anything yet that is great

There is still a call open_file

You mean outside get_ranges or...? At least as part of get_ranges I don't see anymore a metadata request.

Yeah, I meant in get_opts
https://github.com/apache/arrow-rs-object-store/blob/ea71bafd94fe271af74eae07751db632908608cd/src/local.rs#L421-L420

[alamb]

Am about out of time for now -- will check back later

[alamb]

I have some thoughts about this one -- will see what I can do

[alamb]

@Dandandan -- here is a proposal to simplify the error handling (I think it is somewhat tricky with all the corner cases, so refactoring into some functions makes it easier to see what is happening I think, and convince myself it is correct)

• Factor out error mapping, reduce duplication and simplify error handling Dandandan/arrow-rs-object-store#1

[alamb]

I took the liberty of pushing a commit to fix clippy and to simplify the error handling some more: 6ce2094

[alamb]

Thanks @Dandandan -- I think this (somewhat self-servingly) now looks quite good 😆

It makes sense to avoid the extra syscall to get metadata