refactor: use Arrow timezone type

[Hashim1999164]

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.

[andygrove]

@Hashim1999164 there are some CI failures - could you take a look?

[Hashim1999164]

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.

[andygrove]

@Hashim1999164 could you merge latest from apache/main to fix the conflict? This will also trigger CI.

[andygrove]

Thanks for working on this. One thing worth calling out that makes the change more valuable than the description suggests: spark-expr/src/kernels/temporal.rs was already importing arrow::array::timezone::Tz on main, while utils.rs and the two conversion_funcs files used the copied crate::timezone::Tz. So the crate really did have two distinct Tz types in play at once. Good to have that collapsed to one.

I also wanted to check the "equivalent parsing behavior" claim rather than take it on faith, since there are no new tests. The conclusion is that the substitution is safe, but the two parsers are not quite identical:

• The old Comet Tz::from_str dispatched on a leading +/- and then used chrono's %:z / %#z, which enforce minutes 00-59.
• Arrow's version uses a strict byte parser for [+-]HH:MM, [+-]HHMM and [+-]HH, and only validates that each character is a digit. So "+09:99" now parses as +10:39 where it previously errored. A malformed offset-looking string also now falls through to a chrono-tz name lookup instead of failing immediately, which changes the error text.

Neither difference is reachable in practice, which is why I agree no new tests are needed here. cast_string_to_timestamp and cast_date_to_timestamp receive the session timezone, and Spark validates that before it is serialized. Inside extract_offset_suffix every offset goes through parse_sign_offset (hours 0-18, minutes 0-59) and is then re-formatted canonically by tz_from_offset_secs, so only well-formed [+-]HH:MM ever reaches Tz::from_str. The one path that passes raw input straight through is the named IANA branch, and that requires a / in the name, which both implementations route to chrono-tz identically.

One thing I would like to see changed before this merges. Could we drop the re-export added in the follow-up commit?

// native/spark-expr/src/lib.rs
// Re-export Arrow timezone so downstream crates keep `spark_expr::timezone`.
pub use arrow::array::timezone;

The only thing referencing spark_expr::timezone is native/core/src/execution/mod.rs:30:

pub use datafusion_comet_spark_expr::timezone;

and that pub use has no consumers of its own anywhere in the native tree. I tried deleting both lines and cargo check --workspace --all-targets builds clean, so the downstream crates the comment refers to do not actually exist.

I think removing both is better than keeping the shim. #5088 is about not maintaining two parallel timezone types, and leaving datafusion_comet_spark_expr::timezone::Tz alive as a second public name for arrow::array::timezone::Tz keeps half of that around. It is also a bit inconsistent with the rest of the PR, which moved every in-crate call site over to importing from arrow directly.

The branch will also need a merge from main. #5150 touched the imports at the top of conversion_funcs/string.rs, so there is a small conflict there. Keeping main's trim import, dropping timezone from the crate:: list and keeping your use arrow::array::timezone::Tz; resolves it. I tried that locally and cargo fmt --all -- --check is clean with 589 tests passing in datafusion-comet-spark-expr.

[Hashim1999164]

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).

[Hashim1999164]

Dropped the spark_expr::timezone re-export and the unused pub use in native/core/src/execution/mod.rs, as suggested.

[Hashim1999164]

Merged latest apache/main into this branch to clear the conflict and retrigger CI.

[Hashim1999164]

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).

[andygrove]

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.

[Hashim1999164]

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.