datafusion-contrib / datafusion-objectstore-s3

S3 as an ObjectStore for DataFusion
Apache License 2.0
59 stars 13 forks source link

Add S3Error #12

Closed matthewmturner closed 2 years ago

matthewmturner commented 2 years ago

Rebase

matthewmturner commented 2 years ago

Probably nothing to do here, for now, as the ObjectStore and ObjectReader traits require DatafusionError and Result

matthewmturner commented 2 years ago

@houqp im interested in your thoughts on this. should crates such as this have their own errors or should they just reuse datafusion error and result? To me it would seem there is value in changing the signatures for the ObjectStore and ObjectReader to allow any type that implements Error or io::Error. This way there is clear separation between whats an actual datafusion error or 3rd party error like this.

houqp commented 2 years ago

I think changing object store interface upstream to make it more generic makes sense, we probably want to use std::error::Error trait instead of io:Error. cc @yjshen in case he has any comment on this since he wrote the object store abstraction ;)

yjshen commented 2 years ago

+1 to use std::error::Error trait instead of io:Error upstream

matthewmturner commented 2 years ago

@houqp @yjshen thank you, both

matthewmturner commented 2 years ago

@seddonm1 would you mind checking this out?

seddonm1 commented 2 years ago

@matthewmturner looks good. I think we should wait for the official release rather than pointing to a commit?

{git = "https://github.com/apache/arrow-datafusion", rev = "7b8d72c5342610a40827f23df7d5604cf24133fd"}
matthewmturner commented 2 years ago

@seddonm1 personally, i wasnt opposed to leaving it like that until the next datafusion release just so we could continue iterating with this included in case it was needed. but i dont have strong feeling on the matter so happy to wait until next release.

seddonm1 commented 2 years ago

@matthewmturner I am ok for you to merge this if we don't publish this crate until the datafusion release and the crate updated?

matthewmturner commented 2 years ago

@seddonm1 absolutely. definitely would not publish with it pointing to a commit.