You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "vigneshsiva11 (via GitHub)" <gi...@apache.org> on 2026/01/24 16:19:57 UTC
[PR] Fix hour/minute/second handling for TimestampNTZ [datafusion-comet]
vigneshsiva11 opened a new pull request, #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265
## Which issue does this PR close?
Closes #3180
## Rationale for this change
Spark `TimestampNTZ` values represent local time without any timezone
information. However, the current Comet implementation applies timezone
conversion when evaluating `hour`, `minute`, and `second`, which leads to
incorrect results for `TimestampNTZ` inputs in non-UTC session timezones.
This change aligns Comet behavior with Spark semantics by ensuring that
timezone conversion is only applied to `Timestamp` values with an explicit
timezone, and not to `TimestampNTZ`.
## What changes are included in this PR?
- Updated the Rust implementation of `extract_date_part` used by
`hour`, `minute`, and `second` to:
- Bypass timezone conversion for `TimestampNTZ` inputs
- Preserve existing behavior for `Timestamp` inputs with timezone
- Added defensive handling for unsupported input types
## How are these changes tested?
- Verified via Rust unit tests:
```bash
cargo test -p datafusion-comet-spark-expr
--
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] fix: hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265#issuecomment-4040808712
You need to run `mvn spotless:apply` on your source to fix the formatting.
--
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] fix: hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265#issuecomment-4468892448
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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] Fix hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265#issuecomment-3796683424
## [Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/3265?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 22.10%. Comparing base ([`f09f8af`](https://app.codecov.io/gh/apache/datafusion-comet/commit/f09f8af64c6599255e116a376f4f008f2fd63b43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`86ba72d`](https://app.codecov.io/gh/apache/datafusion-comet/commit/86ba72de904c74695e1775cb8b086a2825409714?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 879 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #3265 +/- ##
=============================================
- Coverage 56.12% 22.10% -34.03%
+ Complexity 976 512 -464
=============================================
Files 119 172 +53
Lines 11743 15926 +4183
Branches 2251 2631 +380
=============================================
- Hits 6591 3520 -3071
- Misses 4012 11857 +7845
+ Partials 1140 549 -591
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/3265?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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] fix: hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "vigneshsiva11 (via GitHub)" <gi...@apache.org>.
vigneshsiva11 commented on PR #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265#issuecomment-3836015738
i have been marked as ready for 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] fix: hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265#issuecomment-3825117929
@vigneshsiva11 I have moved this to draft for now. Please mark as ready for review once feedback has been addressed. Thank you.
--
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] fix: hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265#discussion_r2787437487
##########
spark/src/test/scala/org/apache/spark/sql/comet/ParquetDatetimeRebaseSuite.scala:
##########
@@ -116,6 +116,38 @@ abstract class ParquetDatetimeRebaseSuite extends CometTestBase {
}
}
+ test("datediff works with dictionary-encoded timestamp columns") {
Review Comment:
This test appears to have nothing to do with `ParquetDatetimeRebaseSuite`. It should probably be in `CometTemporalExpressionsSuite`.
--
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] Fix hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "vigneshsiva11 (via GitHub)" <gi...@apache.org>.
vigneshsiva11 commented on PR #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265#issuecomment-3818491424
I’ve added the regression test for `datediff` with dictionary-encoded timestamp columns and pushed the update. Thanks for the guidance!
--
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] fix: hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "vigneshsiva11 (via GitHub)" <gi...@apache.org>.
vigneshsiva11 commented on PR #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265#issuecomment-4054748622
Hi @parthchandra, Thanks for pointing this out. I ran `mvn spotless:apply` to fix the formatting issues and pushed the updated changes.
--
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] fix: hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265#discussion_r2747501545
##########
native/spark-expr/src/datetime_funcs/extract_date_part.rs:
##########
@@ -75,14 +76,28 @@ macro_rules! extract_date_part {
match args {
[ColumnarValue::Array(array)] => {
- let array = array_with_timezone(
- array,
- self.timezone.clone(),
- Some(&DataType::Timestamp(
- Microsecond,
- Some(self.timezone.clone().into()),
- )),
- )?;
+ let array = match array.data_type() {
+ // TimestampNTZ → DO NOT apply timezone conversion
+ DataType::Timestamp(_, None) => array.clone(),
+
+ // Timestamp with timezone → convert from UTC to session timezone
+ DataType::Timestamp(_, Some(_)) => array_with_timezone(
+ array,
Review Comment:
@vigneshsiva11 this is a valid concern
--
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] fix: hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #3265: fix: hour/minute/second handling for TimestampNTZ
URL: https://github.com/apache/datafusion-comet/pull/3265
--
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] fix: hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265#issuecomment-4071146404
Tests are failing
```
- hour/minute/second with TimestampNTZ in non-UTC timezone *** FAILED *** (140 milliseconds)
Expected only Comet native operators, but found Project.
plan: CometSort
+- CometColumnarExchange
+- Project [COMET: hour(ts_ntz#70663, Some(UTC)) is not fully compatible with Spark (Incorrectly applies timezone conversion to TimestampNTZ inputs (https://github.com/apache/datafusion-comet/issues/3180)). To enable it anyway, set spark.comet.expression.Hour.allowIncompatible=true. For more information, refer to the Comet Compatibility Guide (https://datafusion.apache.org/comet/user-guide/compatibility.html)., minute(ts_ntz#70663, Some(UTC)) is not fully compatible with Spark (Incorrectly applies timezone conversion to TimestampNTZ inputs (https://github.com/apache/datafusion-comet/issues/3180)). To enable it anyway, set spark.comet.expression.Minute.allowIncompatible=true. For more information, refer to the Comet Compatibility Guide (https://datafusion.apache.org/comet/user-guide/compatibility.html)., second(ts_ntz#70663, Some(UTC)) is not fully compatible with Spark (Incorrectly applies timezone conversion to TimestampNTZ inputs (https://github.com/apache/datafusion-comet
/issues/3180)). To enable it anyway, set spark.comet.expression.Second.allowIncompatible=true. For more information, refer to the Comet Compatibility Guide (https://datafusion.apache.org/comet/user-guide/compatibility.html).]
+- CometSparkRowToColumnar
+- Scan ExistingRDD
```
--
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] Fix hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265#discussion_r2724324610
##########
native/spark-expr/src/datetime_funcs/extract_date_part.rs:
##########
@@ -75,14 +76,28 @@ macro_rules! extract_date_part {
match args {
[ColumnarValue::Array(array)] => {
- let array = array_with_timezone(
- array,
- self.timezone.clone(),
- Some(&DataType::Timestamp(
- Microsecond,
- Some(self.timezone.clone().into()),
- )),
- )?;
+ let array = match array.data_type() {
+ // TimestampNTZ → DO NOT apply timezone conversion
+ DataType::Timestamp(_, None) => array.clone(),
+
+ // Timestamp with timezone → convert from UTC to session timezone
+ DataType::Timestamp(_, Some(_)) => array_with_timezone(
+ array,
Review Comment:
`invoke_with_args` now pattern-matches directly on `array.data_type()` and rejects `DataType::Dictionary(_, _)` with an execution error. This is a regression for dictionary-encoded timestamp columns (the function’s `return_type` explicitly supports dictionary input, and `array_with_timezone` also has dictionary handling). Add explicit handling for `DataType::Dictionary(_, value_type)` where `value_type` is `Timestamp(_, None)` (no conversion) or `Timestamp(_, Some(_))` (apply `array_with_timezone`), instead of falling into the error branch.
--
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] fix: hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265#issuecomment-3825115113
> I’ve added the regression test for `datediff` with dictionary-encoded timestamp columns and pushed the update. Thanks for the guidance!
@vigneshsiva11 I don't see the test
--
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] fix: hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "vigneshsiva11 (via GitHub)" <gi...@apache.org>.
vigneshsiva11 commented on PR #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265#issuecomment-3912718812
@andygrove Thank you for the detailed review and feedback!
I've updated the PR to address all your concerns:
### Changes Made:
1. Dictionary-encoded timestamp support: Added proper handling for dictionary-encoded arrays (common in Parquet/Iceberg) in `extract_date_part.rs`. The code now:
- First normalizes dictionary arrays by casting to the underlying timestamp type
- Then applies the appropriate timezone logic based on whether it's TimestampNTZ or Timestamp
2. Regression test added: Added a comprehensive test in `CometTemporalExpressionSuite` (as suggested) that validates `hour/minute/second` extraction from TimestampNTZ columns across multiple session timezones (UTC, America/Los_Angeles, and Asia/Tokyo). This ensures the behavior is consistent regardless of session timezone.
3. Cleaned up PR scope: Removed the unrelated `datediff` changes that were causing test failures and belonged in a separate PR.
### Code Changes:
- [native/spark-expr/src/datetime_funcs/extract_date_part.rs](https://github.com/apache/datafusion-comet/pull/3265/files#diff-...) - Dictionary support + TimestampNTZ handling
- [spark/src/test/scala/org/apache/comet/CometTemporalExpressionSuite.scala](https://github.com/apache/datafusion-comet/pull/3265/files#diff-...) - New regression test
The PR now focuses solely on fixing issue #3180. Ready for another review when you have time!
--
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] fix: hour/minute/second handling for TimestampNTZ [datafusion-comet]
Posted by "vigneshsiva11 (via GitHub)" <gi...@apache.org>.
vigneshsiva11 commented on PR #3265:
URL: https://github.com/apache/datafusion-comet/pull/3265#issuecomment-4038095357
Hi @andygrove, Thank you for approving the changes! 🙏
I notice 94 tests are failing in CI. I checked a few logs, and they don't appear to be related to the TimestampNTZ/hour/minute/second changes in this PR. Could you help me understand if these failures are:
- Pre-existing issues in the main branch?
- CI infrastructure problems?
- Something I should fix?
Please let me know if there's anything specific I need to address. Thank you!
--
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