You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Hashim1999164 (via GitHub)" <gi...@apache.org> on 2026/07/29 14:16:09 UTC
[PR] refactor: use Arrow timezone type [datafusion-comet]
Hashim1999164 opened a new pull request, #5129:
URL: https://github.com/apache/datafusion-comet/pull/5129
## Which issue does this PR close?
Closes #5088.
## Rationale for this change
Arrow 58 exposes `arrow::array::timezone::Tz` when the existing `chrono-tz` feature is enabled. Keeping a copied implementation in `spark-expr` creates two parallel timezone types and duplicates upstream code.
## What changes are included in this PR?
- replace the local timezone type in string, temporal, and utility code with Arrow's public `Tz`
- remove the copied `timezone.rs` module
- remove the now-unused direct `chrono-tz` dependency from `spark-expr`
## How are these changes tested?
- `cargo test -p datafusion-comet-spark-expr --lib` (538 passed)
- `cargo clippy -p datafusion-comet-spark-expr --all-targets -- -D warnings`
- `cargo fmt --all -- --check`
No new tests are needed because this is a type substitution with equivalent parsing behavior, and the existing timezone, fixed-offset, DST, and timestamp parsing tests exercise the affected paths.
## AI assistance
AI assistance was used to locate consumers of the copied type and draft the mechanical import substitutions. I reviewed the full diff and ran the validation above.
Made with [Cursor](https://cursor.com)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: use Arrow timezone type [datafusion-comet]
Posted by "Hashim1999164 (via GitHub)" <gi...@apache.org>.
Hashim1999164 commented on PR #5129:
URL: https://github.com/apache/datafusion-comet/pull/5129#issuecomment-5159657814
Dropped the `spark_expr::timezone` re-export and the unused `pub use` in `native/core/src/execution/mod.rs`, as suggested.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: use Arrow timezone type [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: use Arrow timezone type [datafusion-comet]
Posted by "Hashim1999164 (via GitHub)" <gi...@apache.org>.
Hashim1999164 commented on PR #5129:
URL: https://github.com/apache/datafusion-comet/pull/5129#issuecomment-5196305641
Merged latest `apache/main` into this branch to clear the conflict and retrigger CI.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: use Arrow timezone type [datafusion-comet]
Posted by "Hashim1999164 (via GitHub)" <gi...@apache.org>.
Hashim1999164 commented on PR #5129:
URL: https://github.com/apache/datafusion-comet/pull/5129#issuecomment-5231658268
Ran `cargo fmt --all` so the contiguous `mod` declarations in `native/core/src/execution/mod.rs` are sorted again after dropping the re-export. Also removed the unused `chrono-tz` pin from `native/Cargo.toml` workspace dependencies, leaving the Arrow `chrono-tz` feature as the source of that crate.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: use Arrow timezone type [datafusion-comet]
Posted by "Hashim1999164 (via GitHub)" <gi...@apache.org>.
Hashim1999164 commented on PR #5129:
URL: https://github.com/apache/datafusion-comet/pull/5129#issuecomment-5226582037
Merged latest `apache/main` and resolved the `native/spark-expr/Cargo.toml` conflict (kept `datafusion-spark` for the new make_interval support, still using Arrow timezone instead of a direct `chrono-tz` dependency).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: use Arrow timezone type [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5129:
URL: https://github.com/apache/datafusion-comet/pull/5129#issuecomment-5152052303
@Hashim1999164 could you merge latest from apache/main to fix the conflict? This will also trigger CI.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: use Arrow timezone type [datafusion-comet]
Posted by "Hashim1999164 (via GitHub)" <gi...@apache.org>.
Hashim1999164 commented on PR #5129:
URL: https://github.com/apache/datafusion-comet/pull/5129#issuecomment-5159589424
Merged latest `apache/main` and resolved the import conflict in `native/spark-expr/src/conversion_funcs/string.rs` (kept the new trim helpers from main and continued using `arrow::array::timezone::Tz`).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: use Arrow timezone type [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #5129:
URL: https://github.com/apache/datafusion-comet/pull/5129#issuecomment-5403700054
Merge, thanks @Hashim1999164 for the contribution and @andygrove for the review!!!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: use Arrow timezone type [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5129:
URL: https://github.com/apache/datafusion-comet/pull/5129#issuecomment-5133404498
@Hashim1999164 there are some CI failures - could you take a look?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: use Arrow timezone type [datafusion-comet]
Posted by "Hashim1999164 (via GitHub)" <gi...@apache.org>.
Hashim1999164 commented on PR #5129:
URL: https://github.com/apache/datafusion-comet/pull/5129#issuecomment-5145912003
Thanks for the ping. CI was failing because `native/core` still imports `datafusion_comet_spark_expr::timezone` after the local module was removed. I re-exported `arrow::array::timezone` from spark-expr so that path keeps working.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: use Arrow timezone type [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao merged PR #5129:
URL: https://github.com/apache/datafusion-comet/pull/5129
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org