feat: Allow datafusion-ffi to opt out of proto parquet#22951
Conversation
martin-g
left a comment
There was a problem hiding this comment.
There are no new tests / CI jobs verifying that cargo check -p datafusion-ffi --no-default-features still builds.
| let formats = vec![ConfigFileType::CSV, ConfigFileType::JSON]; | ||
| #[cfg(feature = "parquet")] | ||
| let formats = { | ||
| let mut formats = formats; | ||
| formats.push(ConfigFileType::PARQUET); | ||
| formats | ||
| }; |
There was a problem hiding this comment.
| let formats = vec![ConfigFileType::CSV, ConfigFileType::JSON]; | |
| #[cfg(feature = "parquet")] | |
| let formats = { | |
| let mut formats = formats; | |
| formats.push(ConfigFileType::PARQUET); | |
| formats | |
| }; | |
| let formats = [ | |
| ConfigFileType::CSV, | |
| ConfigFileType::JSON, | |
| #[cfg(feature = "parquet")] | |
| ConfigFileType::PARQUET, | |
| ]; |
There was a problem hiding this comment.
Are there any downsides of not doing the changes below when the parquet feature is not enabled ?
Since there is no usage of Parquet types I think it would be better to still support ConfigFileType::PARQUET as before.
|
|
||
| [dev-dependencies] | ||
| datafusion-proto = { workspace = true } | ||
| datafusion-proto = { workspace = true, default-features = true } |
There was a problem hiding this comment.
This is redundant since default-features = true is the default behavior anyway.
Maybe list the used features explicitly:
| datafusion-proto = { workspace = true, default-features = true } | |
| datafusion-proto = { workspace = true, features = ["parquet"] } |
| datafusion-expr = { workspace = true } | ||
| datafusion-physical-expr-adapter = { workspace = true } | ||
| datafusion-proto = { workspace = true } | ||
| datafusion-proto = { workspace = true, default-features = true } |
| datafusion-physical-optimizer = { path = "datafusion/physical-optimizer", version = "54.0.0" } | ||
| datafusion-physical-plan = { path = "datafusion/physical-plan", version = "54.0.0" } | ||
| datafusion-proto = { path = "datafusion/proto", version = "54.0.0" } | ||
| datafusion-proto = { path = "datafusion/proto", version = "54.0.0", default-features = false } |
There was a problem hiding this comment.
This changes the default behavior for all members of the workspace, no just for -ffi. Is this intentional ?
The -ffi crate already does it at https://github.com/apache/datafusion/pull/22951/changes#diff-c3dbf9bcba2d884b87eb3587e66d470700dd7e8e75f1630765e1da4b42a81c5aR66
Which issue does this PR close?
N/A
Rationale for this change
datafusion-ffiinheritsdatafusion-protofrom the workspace. Becausedatafusion-protoenables itsparquetfeature by default, downstream crates that depend ondatafusion-ffialso pull indatafusion-datasource-parquet,parquet, and parquet compression dependencies even when they do not need parquet-aware proto support.Cargo features are additive, so downstream users cannot opt out by disabling default features on their own direct
datafusion-protodependency if thedatafusion-ffi -> datafusion-protoedge has already enabled defaults.What changes are included in this PR?
This PR makes the workspace
datafusion-protodependency default todefault-features = false, then lets crates opt into the previous behavior explicitly.For
datafusion-ffi, the previous default behavior is preserved with a new defaultparquetfeature. Downstream users that want to avoid parquet dependencies can now usedatafusion-ffiwithdefault-features = false.The FFI session table option bridge now gates
ConfigFileType::PARQUETusage behind thedatafusion-ffi/parquetfeature, while still preserving parquet table option values in no-default builds.Are these changes tested?
Yes. Existing
datafusion-ffitests pass with default features and with--no-default-features. The feature graph was also checked to confirm that defaultdatafusion-ffistill enables parquet, whiledatafusion-ffi --no-default-featuresdoes not includedatafusion-datasource-parquet.Are there any user-facing changes?
Default behavior is unchanged. Users can now opt out of parquet-aware proto support from
datafusion-ffiby disablingdatafusion-ffidefault features.