Skip to content

Add S3Error#12

Merged
matthewmturner merged 15 commits intodatafusion-contrib:mainfrom
matthewmturner:add_s3_error
Jan 27, 2022
Merged

Add S3Error#12
matthewmturner merged 15 commits intodatafusion-contrib:mainfrom
matthewmturner:add_s3_error

Conversation

@matthewmturner
Copy link
Copy Markdown
Collaborator

Rebase

@matthewmturner
Copy link
Copy Markdown
Collaborator Author

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

@matthewmturner
Copy link
Copy Markdown
Collaborator Author

@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
Copy link
Copy Markdown
Member

houqp commented Jan 8, 2022

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
Copy link
Copy Markdown

yjshen commented Jan 8, 2022

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

@matthewmturner
Copy link
Copy Markdown
Collaborator Author

@houqp @yjshen thank you, both

@matthewmturner
Copy link
Copy Markdown
Collaborator Author

@seddonm1 would you mind checking this out?

@seddonm1
Copy link
Copy Markdown
Collaborator

@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
Copy link
Copy Markdown
Collaborator Author

@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
Copy link
Copy Markdown
Collaborator

@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
Copy link
Copy Markdown
Collaborator Author

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

@matthewmturner matthewmturner merged commit 0ed47da into datafusion-contrib:main Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace datafusion Error/Result with new S3Error / Result

4 participants