feat: Allow datafusion-ffi to opt out of proto parquet - #22951
Conversation
github-actions
Bot
added
proto
Xuanwo
changed the title
martin-g
left a comment
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.
github-actions
Bot
added
the
development-process
|
I added a CI check for this path:
cc @martin-g, please take another look, thank you! |
|
@adriangb can you also maybe take a look at this PR in light of the work you are doing to refactor the proto also cc @timsaucer |
timsaucer
left a comment
timsaucer
left a comment
There was a problem hiding this comment.
Only one issue I see and it may be on my misreading.
| #[cfg(not(feature = "parquet"))] | ||
| for (key, value) in options.iter().filter_map(|(k, v)| { | ||
| let (prefix, key) = k.split_once(".")?; | ||
| (prefix == "parquet").then(|| (key, v)) | ||
| }) { | ||
| table_options.parquet.set(key, value).unwrap_or_else(|err| { | ||
| log::warn!("Error parsing parquet table option: {err}") | ||
| }); | ||
| } |
There was a problem hiding this comment.
Am I reading this wrong? It looks like this is gating backwards - this code block is enabled when we don't have the parquet feature, right?
There was a problem hiding this comment.
yep, I want to print some warnings while users passing parquet options but parquet didn't enabled. I'm open to just remove it.
There was a problem hiding this comment.
I recommend removing this code snippet. I ran a test and when we do set valid parquet options the warning does not fire because table_options.parquet.set() passes. Plus there are a bunch of default table options for parquet that do pass as well and we cannot tell a priori if they're the default values or if the user has set the value. After all, a user could intentionally set a value to a default and the intent of this snippet would be to warn when setting these values.
In the end, maybe I'm being pedantic but I recommend just removing this bit.
There was a problem hiding this comment.
Thanks for the review @timsaucer. That makes sense. I have removed, PTAL.
5 participants
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.