andygrove commented on PR #5129:
URL:
https://github.com/apache/datafusion-comet/pull/5129#issuecomment-5226863859
Thanks for making both of those changes, the re-export removal and the merge
resolution both look right to me. I built the branch and the substitution
itself is in good shape. 627 tests pass in `datafusion-comet-spark-expr`,
`cargo check --workspace --all-targets` is clean, and so is clippy. I have also
approved the workflow runs, so CI should start reporting now.
One thing needs fixing before this can go in. `cargo fmt --all -- --check`
now fails on `native/core/src/execution/mod.rs`:
```
pub use datafusion_comet_shuffle as shuffle;
-pub(crate) mod sort;
-pub(crate) mod spark_plan;
mod memory_pools;
+pub(crate) mod sort;
pub(crate) mod spark_config;
+pub(crate) mod spark_plan;
```
This is fallout from dropping the re-export, so it is my suggestion that
caused it. The deleted `pub use datafusion_comet_spark_expr::timezone;` used to
sit between the `mod` declarations and split them into two separate contiguous
blocks. `rustfmt` sorts each contiguous run independently, and removing that
line merged the two runs into one that is no longer in order. `cargo fmt --all`
sorts it out in two lines. Worth re-running the fmt check locally after, since
your description understandably reports it clean from before that commit.
While you are in there, could you also drop `chrono-tz = { version = "0.10"
}` from `[workspace.dependencies]` in `native/Cargo.toml`? `spark-expr` was the
last member crate referencing it, so that pin is now dead. The `chrono-tz`
feature on `arrow` is what actually brings the crate in. I would rather not
leave an unreferenced pin sitting there, because it makes it easy for someone
to re-add a direct `chrono-tz` dependency at a version that drifts from
whatever `arrow` resolves to, which is how you end up with two `chrono_tz::Tz`
types again. I checked and `Cargo.lock` resolves a single `chrono-tz 0.10.4`
today, so removing it changes nothing. I deleted the line locally and `cargo
check --workspace --all-targets` builds clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
