Uh oh!
There was an error while loading. Please reload this page.
Fix broken serde feature - #15124
Conversation
Weijun-H
commented
Mar 10, 2025
I am surprised that CI did not notice it. Could we add a test for that? |
| "datafusion-sql/recursive_protection", | ||
| ] | ||
| serde = ["dep:serde"] | ||
| serde = ["dep:serde", "arrow-schema/serde"] |
There was a problem hiding this comment.
it would be great to comment why serde needed here
There was a problem hiding this comment.
Added comment, if it's not what you proposed, please tell what I should write in there)
vadimpiven
commented
Mar 11, 2025
@Weijun-H added test, please check that it is in the correct place. Also I was not sure if I should make a separate pipeline invocation, please let me know if I should |
Test failed because of “no space left on device” error from runner. Can someone please restart it? |
vadimpiven
commented
Mar 11, 2025
Merged main to trigger CI |
| rust-version: stable | ||
| - name: Run tests (excluding doctests) | ||
| run: cargo test --profile ci --exclude datafusion-examples --exclude ffi_example_table_provider --exclude datafusion-benchmarks --workspace --lib --tests --bins --features avro,json,backtrace,integration-tests | ||
| - name: Run serde feature test |
There was a problem hiding this comment.
I don't think we need to run the tests -- we just need to check that the code compiles
Perhaps we can add the check here:
datafusion/.github/workflows/rust.yml
Lines 116 to 118 in 04d823b
And adjust the comments to say it works with a subset
# cargo check datafusion to ensure that the datafusion crate can be built with only a# subset of the function packages enabled.# cargo check datafusion to ensure that the datafusion crate can be built with only a# subset of the packages enabled.I'll make this change and push to your branch to save some more back and forth cycles
alamb
left a comment
There was a problem hiding this comment.
Thank you @vadimpiven
I pushed a commit to move the test into an existing test binary and run it as part of the CI tests
| // specific language governing permissions and limitations | ||
| // under the License. | ||
| /// Ensure `serde` feature from `arrow-schema` crate is re-exported. |
There was a problem hiding this comment.
I consolidated this into the core_integration binary so we didn't have to build a new target to test this
andrewlamb@Andrews-MacBook-Pro-2:~/Software/datafusion$ cargo test --test core_integration --features=serde -- serde
Compiling datafusion v46.0.0 (/Users/andrewlamb/Software/datafusion/datafusion/core)
Finished `test` profile [unoptimized + debuginfo] target(s) in 8.36s
Running tests/core_integration.rs (target/debug/deps/core_integration-acc990c95fa5aa49)
running 1 test
test serde::ensure_serde_support ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 567 filtered out; finished in 0.00s
andrewlamb@Andrews-MacBook-Pro-2:~/Software/datafusion$ cargo test --test core_integration -- serde
Compiling datafusion v46.0.0 (/Users/andrewlamb/Software/datafusion/datafusion/core)
Finished `test` profile [unoptimized + debuginfo] target(s) in 2.68s
Running tests/core_integration.rs (target/debug/deps/core_integration-370a90f8f594db8d)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 567 filtered out; finished in 0.00s
| mod physical_optimizer; | ||
| /// Run all tests that are found in the `serde` directory | ||
| mod serde; |
alamb
commented
Mar 11, 2025
Thanks again @vadimpiven and @Weijun-H |
* Fix broked `serde` feature * Test `serde` feature * consolidate serde test into core_integration, update run --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Which issue does this PR close?
Rationale for this change
PR #14597 have removed the line
datafusion/datafusion/core/Cargo.toml
Line 77 in 3f900ac
which broke
serdefeature. Reverting this change.What changes are included in this PR?
serdefeature updateAre these changes tested?
Added test and pipeline invocation.
Are there any user-facing changes?
No user-facing changes.