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

Consider adopting IOx ObjectStore abstraction #2489

Closed
wjones127 opened this issue May 8, 2022 · 9 comments · Fixed by #2677
Closed

Consider adopting IOx ObjectStore abstraction #2489

wjones127 opened this issue May 8, 2022 · 9 comments · Fixed by #2677
Assignees
Labels
enhancement New feature or request

Comments

@wjones127
Copy link
Member

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

In another issue @alamb and @tustvold suggested we might want to use the IOx ObjectStore implementation.

A few nice points I'll mention about the IOx one:

  • They have some nice path utilities, including a CloudPath struct. That seems nicer than the current one with &str paths.
  • Has implementations for S3, GCS, Azure Blob Storage included in the repo. There is no HDFS support yet.
  • Has implementations of put() for writing. There doesn't seem to be streaming write support (multi-part upload).

There are a few differences in the API:

Current API: https://github.com/apache/arrow-datafusion/blob/dfdeb42d7d646cffcf3cff26beefcecffc6cbe62/data-access/src/object_store/mod.rs#L77

IOx API: https://github.com/influxdata/influxdb_iox/blob/94e9ac610acfb94870154d976f66a4d4111b5668/object_store/src/lib.rs#L74

  • The IOx list() implementation evaluated prefixes on path segments: "Prefixes are evaluated on a path segment basis, i.e. foo/bar/ is a prefix of foo/bar/x but not of foo/bar_baz/x."
  • IOx doesn't have a synchronous read implementation.

There of course exist other repos that this has implications for:

From what I've seen, it seems like we could reasonably shift to simply use the IOx ObjectStore. But if there's a good reason, we could also reuse useful parts of the implementation to keep the existing API.

cc @matthewmturner @kyotoYaho @roeap

@wjones127 wjones127 added the enhancement New feature or request label May 8, 2022
@matthewmturner
Copy link
Contributor

matthewmturner commented May 9, 2022

Indeed the IOx ObjectStore implementation looks both robust and feature rich - and has the added benefit of being actively used. While datafusion-objectstore-s3 has been released on crates i am not aware of any production workloads leveraging it.

I also prefer the more generic API interface in the IOx implementation, I had actually planned on proposing something similar on #2445.

From an s3 perspective the only things that come to mind are:

  • Rusoto is used rather than official AWS Rust Sdk. If thats good enough for IOx then im sure its fine for now, but i think would be good to move to AWS Rust Sdk sooner than later.
  • We can just double check that our existing test suite in datafusion-objectstore-s3 works without issue. In particular for connecting to services such as MinIO (which is S3 compatible storage) - I have spoken to them and know they were watching the project.

As for the actual implementation, given the IOx implementation already has AWS, GCP, and Azure functionality features could we just create datafusion-objectstore in datafusion-contrib and let people choose what they want by choosing the relevant feature?

@alamb
Copy link
Contributor

alamb commented May 9, 2022

I believe @tustvold is actively working on preparing the iox code for crates.io release in https://github.com/influxdata/influxdb_iox/pull/4534

@tustvold
Copy link
Contributor

tustvold commented May 9, 2022

Yeah as alluded to by @alamb, my plan is to get the iox code released to crates.io so that DataFusion could use it.

There would then be a couple of potential courses of action for DataFusion:

@tustvold
Copy link
Contributor

I've released the crate to crates.io - https://crates.io/crates/object_store, I'm going to take a stab at integrating this into Datafusion over this weekend. Hopefully I'll get something up as a workable draft

@thinkharderdev
Copy link
Contributor

Yeah as alluded to by @alamb, my plan is to get the iox code released to crates.io so that DataFusion could use it.

There would then be a couple of potential courses of action for DataFusion:

Wrt fetching to local disk, we have an implementation of (datafusion) ObjectStore in our project which adopts the S3A approach to minimize the number of small range requests. Basically, we set a minimum chunk size for S3 reads (usually 64K). If a read of less than 64K is requested, we go ahead and fetch 64K and buffer it in memory. Subsequent reads that fall within that buffer are returned from the in-memory buffer. This minimizes the overhead of small range requests from the PageIterator while still avoiding reads of columns not required for the query.

@tustvold
Copy link
Contributor

tustvold commented May 16, 2022

Yeah, buffered prefetch is one way to mitigate the small read problem. However, it does not allow for coalescing adjacent reads - i.e. you will still likely end up with one request per column chunk unless you have tiny columns.

TBC my preference is for 3, which mirrors the new vectored API if S3a, but I'm currently working on 2 first to ensure there aren't any fundamental integration issues.

@alamb
Copy link
Contributor

alamb commented May 16, 2022

I believe @tustvold is working on this issue

@thinkharderdev
Copy link
Contributor

Yeah, buffered prefetch is one way to mitigate the small read problem. However, it does not allow for coalescing adjacent reads - i.e. you will still likely end up with one request per column chunk unless you have tiny columns.

TBC my preference is for 3, which mirrors the new vectored API if S3a, but I'm currently working on 2 first to ensure there aren't any fundamental integration issues.

Cool. In our case the buffered prefetch helps marginally (but we also have a lot of sparse columns so it is a slightly special case which does a reasonable job at coalescing adjacent reads).

apache/arrow-rs#1605 looks like a really good idea. We're also working on trying to optimize S3 reads at the moment so if there's any way I can help please let me know!

@jychen7
Copy link
Contributor

jychen7 commented May 28, 2022

Has implementations for S3, GCS, Azure Blob Storage included in the repo

that's great, at a time, I want to create datafusion-objectstore-gcs similar to s3/azure, since I mostly play with GCP. And want to try how datafusion and also ballista can query GCS

tustvold added a commit to tustvold/arrow-datafusion that referenced this issue Jun 1, 2022
alamb added a commit that referenced this issue Jul 4, 2022
* Switch to object_store crate (#2489)

* Test fixes

* Update to object_store 0.2.0

* More windows pacification

* Fix windows test

* Fix windows test_prefix_path

* More windows fixes

* Simplify ListingTableUrl::strip_prefix

* Review feedback

* Update to latest arrow-rs

* Use ParquetRecordBatchStream

* Simplify predicate pruning

* Add host to ObjectStoreRegistry

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
6 participants