feat: feature-gate sqllogictests datafusion-substrait behind optional 'substrait' feature#21268
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes the datafusion-substrait integration in the datafusion-sqllogictest crate opt-in behind a new substrait feature flag to reduce compile time and dependency weight for the default build.
Changes:
- Make
datafusion-substraitan optional dependency and add asubstraitfeature indatafusion/sqllogictest/Cargo.toml. - Gate the Substrait round-trip engine module and public re-exports behind
#[cfg(feature = "substrait")]. - Add a non-
substraitfallback for the Substrait round-trip runner and update CI to enable the feature when running that mode.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| datafusion/sqllogictest/src/lib.rs | Gate the public re-export of DataFusionSubstraitRoundTrip behind the substrait feature. |
| datafusion/sqllogictest/src/engines/mod.rs | Gate the Substrait round-trip engine module and re-export behind the substrait feature. |
| datafusion/sqllogictest/Cargo.toml | Make datafusion-substrait optional and introduce a substrait feature enabling it. |
| datafusion/sqllogictest/bin/sqllogictests.rs | Conditionally import/use the Substrait engine; provide a runtime error fallback when feature is disabled. |
| .github/workflows/rust.yml | Update Substrait round-trip CI job to pass --features substrait. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
CI failure in |
sqllogictests datafusion-substrait behind optional 'substrait' feature
|
The benchmark results result seems to pass on main; https://github.com/apache/datafusion/actions/runs/23868036699/job/69592615386 I'll update this branch to try and get a clean CI run |
alamb
left a comment
There was a problem hiding this comment.
Makes sense to me -- thank yoU @zhuqi-lucas
FYI @gabotechs
I also verified that I can still run my core testing flow
nice cargo test --profile=ci --test sqllogictestsLocally and it passes
Only when I tried to run the substrait roundtrip tests did it fail (as expected)
andrewlamb@Andrews-MacBook-Pro-3:~/Software/datafusion2$ nice cargo test --profile=ci --test sqllogictests -- --substrait-round-trip
Finished `ci` profile [unoptimized] target(s) in 0.21s
Running bin/sqllogictests.rs (target/ci/deps/sqllogictests-703664fca1f0af0f)
Running with 16 test threads (available parallelism: 16)
Completed 416 test files in 0 seconds Execution error: Cannot run substrait round-trip: the 'substrait' feature is not enabled
Execution error: Cannot run substrait round-trip: the 'substrait' feature is not enabled
Execution error: Cannot run substrait round-trip: the 'substrait' feature is not enabled
Execution error: Cannot run substrait round-trip: the 'substrait' feature is not enabled
Execution error: Cannot run substrait round-trip: the 'substrait' feature is not enabled66f11df to
732cabb
Compare
…eature Makes datafusion-substrait an optional dependency in datafusion-sqllogictest, gated behind a new 'substrait' feature flag. This saves compile time when substrait is not needed (the common case for most developers). Changes: - datafusion-substrait is now optional in sqllogictest/Cargo.toml - Added 'substrait' feature flag - Gated substrait imports, module, and round-trip function behind cfg - Added no-op fallback when feature is disabled - Updated CI to pass --features substrait for round-trip test
732cabb to
fd91871
Compare
|
The CI all green now, thanks @alamb for review. Merged to main now. |
…al 'substrait' feature (apache#21268) ## Which issue does this PR close? - Closes apache#21269 ## Rationale for this change `datafusion-substrait` is a heavyweight dependency that most developers and users don't need. Making it optional saves compile time for the common case. ## What changes are included in this PR? - `datafusion-substrait` is now an optional dependency in `datafusion-sqllogictest/Cargo.toml` - New `substrait` feature flag gates the dependency - `#[cfg(feature = "substrait")]` gates the module, imports, and `run_test_file_substrait_round_trip` function - No-op fallback function returns an error when the feature is disabled - CI workflow updated to pass `--features substrait` for the round-trip test ## Are these changes tested? - Compiles without `substrait` feature (default) - Compiles with `--features substrait` - CI substrait round-trip test updated to enable the feature ## Are there any user-facing changes? No — this only affects the sqllogictest binary, not the datafusion library itself. The substrait round-trip test requires `--features substrait` to run.
Which issue does this PR close?
Rationale for this change
datafusion-substraitis a heavyweight dependency that most developers and users don't need. Making it optional saves compile time for the common case.What changes are included in this PR?
datafusion-substraitis now an optional dependency indatafusion-sqllogictest/Cargo.tomlsubstraitfeature flag gates the dependency#[cfg(feature = "substrait")]gates the module, imports, andrun_test_file_substrait_round_tripfunction--features substraitfor the round-trip testAre these changes tested?
substraitfeature (default)--features substraitAre there any user-facing changes?
No — this only affects the sqllogictest binary, not the datafusion library itself. The substrait round-trip test requires
--features substraitto run.