-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
split datafusion-object-store module #2065
Conversation
Hi @jimexist, @matthewmturner, @yjshen, @alamb, could you help review this patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me -- thank you @yahoNanJing
cc @yjshen @rdettai @matthewmturner and @tustvold
Overall looks good. I think my only point, similar to what we discussed before, is on some of the That being said, i have given this more thought and given how the above mentioned functionalities leverage physical plan, etc. i do think it makes sense to have datafusion wrap these functionalities together. just walking through my thought process out loud :) thanks for this. |
🤔 maybe it is time for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
🎉 |
Which issue does this PR close?
Closes #1772.
Rationale for this change
By splitting the object store into a separate module, the object stores in datafusion-contrib no longer need to depend on the whole datafusion crate. They only need to depend on this separated module. What's more, if we hope to introduce the object stores in datafusion-contrib as some featuers in the datafusion crate, the issue of dependency cyclic can be avoided.
What changes are included in this PR?
The reason of still keeping ObjectStoreRegistry in the datafusion crate is
Are there any user-facing changes?