feat: make object_store support optional - #25144
Conversation
|
I'm not sure if this PR is the correct direction, the other route I'm considering is reviev #14854 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25144 +/- ##
==========================================
- Coverage 81.86% 81.85% -0.01%
==========================================
Files 1130 1130
Lines 418783 418818 +35
Branches 418783 418818 +35
==========================================
+ Hits 342819 342838 +19
- Misses 55748 55763 +15
- Partials 20216 20217 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alamb
left a comment
There was a problem hiding this comment.
Thank you for this PR @Xuanwo
I am worried about the the approach of adding cfgs over the codebase
#[cfg(feature = "object_store")]I worry it will both:
- Make it hard to read / maintain
- Not really allow DataFusion to be used without object_store (It may but I am not sure)
What I would like to suggest is that we build this change by example -- specifically, start with an example (e.g. reading parquet from OpenDAL directly without a ObjectStore adapter, or some other suitable example)
Then we can figure out what APIs in DataFusion need to be adapted (e.g. make a Trait with a default ObjectStore based implementation, that can be overridden by downstream users)
What do you think?
|
For example, I think the approach you proposed in Sounds more promising |
|
Yes, I have exactly the same feeling while I'm building this. Will revisit my old idea and put efforts in that way. |
Which issue does this PR close?
Closes #25145.
Rationale for this change
Applications that provide their own
TableProviderandExecutionPlanimplementations currently depend onobject_storeeven with DataFusion's default features disabled. This prevents embedders from excluding an unused storage implementation or avoiding a secondobject_storeversion alongside their own storage layer.What changes are included in this PR?
Add a default-enabled
object_storefeature for built-in file sources and sinks, listing tables, the runtime object store registry, and file caches. Forward the feature through the existing execution dependency chain so direct child-crate users retain storage support by default. No new DataFusion packages or replacement storage abstractions are introduced.Custom providers, in-memory tables, query planning and execution, memory management, and local spill support remain available without this feature. Logical-only Substrait conversion no longer pulls in
object_store; built-in Parquet, Avro, and physical Substrait retain their storage dependencies.What is the testing strategy for this PR?
The existing feature-check CI compiles DataFusion with default features, no default features, and individual features. This PR adds the explicit
object_storecase and a directcargo tree -p datafusion --no-default-features --features sql --target all --edges allassertion that the enabled dependency graph contains noobject_storepackage. A local control check confirmed that this assertion rejects a build with storage enabled.Manual validation with an independent downstream consumer covered custom file access through
TableProviderandExecutionPlan, projection, filtering, joins, aggregation, sorting, and limit. Storage-enabled configurations also exercised built-in CSV/Parquet operations, registry lookup, and cache settings.Local formatting, full-workspace lint and documentation checks, and
cargo clippy --all-targets --all-features -- -D warningspassed.Are there any user-facing changes?
Default builds keep their existing storage APIs and concrete
object_storetypes, including direct child-crate consumers.API change for existing builds with default features disabled: callers using storage APIs must add
object_storeto their features, unless an enabled feature such asparquetoravroalready enables it. The same requirement applies to child crates.To opt out:
To retain storage with defaults disabled:
The crate configuration guide documents migration and feature unification: another dependency can re-enable storage. With storage disabled, built-in file APIs are unavailable, file
COPY TOfails explicitly, and file cache settings are unavailable.