Add datafusion-cli features to allow cloud support to be omitted#23413
Add datafusion-cli features to allow cloud support to be omitted#23413pepijnve wants to merge 1 commit into
Conversation
There is one potential impact here which is worth noting, users that enable |
ratmice
left a comment
There was a problem hiding this comment.
In case we do do want to try and avoid lint warnings, I think these should do it.
| fmt::{Debug, Display}, | ||
| sync::Arc, | ||
| }; | ||
| use datafusion_common::exec_err; |
There was a problem hiding this comment.
If you do want to remove this unused import complaint when compiling without default features, it is possible to get rid of the clippy lint on this one with a line like:
| use datafusion_common::exec_err; | |
| #[cfg(any(feature = "s3", feature = "gcs"))] | |
| use datafusion_common::exec_err; |
| resolve_region: bool, | ||
| ) -> Result<Arc<dyn ObjectStore>, DataFusionError> { | ||
| let store: Arc<dyn ObjectStore> = match scheme { | ||
| #[cfg(feature = "s3")] |
There was a problem hiding this comment.
Not sure how to add the suggestion in the github UI, it sadly ends up as a massive diff that doesn't look right. However, maybe you can fix these if you feel it's worthwhile with the following additions to the get_object_store.
#[cfg_attr(not(any(feature = "s3", feature = "gcs")), expect(unused_variables))]
table_options: &TableOptions,
#[cfg_attr(not(any(feature = "s3", feature = "gcs")), expect(unused_variables))]
resolve_region: bool,
| // Register the store for this URL. Here we don't have access | ||
| // to any command options so the only choice is to use an empty collection | ||
| match scheme { | ||
| #[cfg(feature = "s3")] |
There was a problem hiding this comment.
Only way I could think of to disable this one was,
#[cfg_attr(not(any(feature = "s3", feature = "gcs")), expect(unused_mut))]
let mut builder = SessionStateBuilder::from(state.clone());
I had done exactly this at first, but reverted it because I felt I was cluttering the code with a bunch of cruft only to satisfy the linter. Happy to put those back if that's preferred. |
Indeed, perhaps this does warrant a short upgrade note. I had kind of assumed since there were no default features up until now that most users would not explicitly have opted out of default features. |
Which issue does this PR close?
Rationale for this change
By adding opt-out features for s3, gcs and http, users of the datafusion-cli crate can choose to omit these dependencies. This reduces the dependency graph and final binary size.
What changes are included in this PR?
http,gcs, ands3todatafusion-clis cargo filedatafusion-clicode where necessaryAre these changes tested?
Are there any user-facing changes?
No, the new features are part of the default feature set so this should have little effect for existing users.