You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "harsh-ande (via GitHub)" <gi...@apache.org> on 2026/10/07 22:50:49 UTC
[PR] fix(spark): floor instead of truncate in unix_millis/unix_seconds for pre-epoch timestamp [datafusion]
harsh-ande opened a new pull request, #26129:
URL: https://github.com/apache/datafusion/pull/26129
## Which issue does this PR close?
- Closes #26128.
## Rationale for this change
The Spark-compatible `unix_millis` and `unix_seconds` functions return different results from Spark for timestamps before 1970-01-01 that have a sub-unit component. Spark floors the result, but DataFusion truncates toward zero. For example, `unix_millis` of a timestamp 1 µs before the epoch is `-1` in Spark and `0` in DataFusion. Results for non-negative timestamps and exact multiples already match.
The cause is that `SparkUnixTimestamp::simplify` rewrites the call as a cast to a coarser `Timestamp` unit, and Arrow's cast truncates toward zero. Spark computes these values with `Math.floorDiv` (`TimestampToLongBase`).
## What changes are included in this PR?
In `SparkUnixTimestamp::simplify` (`datafusion/spark/src/function/datetime/unix.rs`):
- When the input timestamp unit is finer than the target unit, the raw `Int64` ticks are divided by the unit ratio (1e3, 1e6 or 1e9). Then `1` is subtracted when the remainder is negative: `ticks / d - CAST(ticks % d < 0 AS BIGINT)`. This is an exact floor division over the full `i64` range, with no intermediate unit conversion, so it adds no new overflow cases.
- When the input unit is the same as or coarser than the target, the existing direct cast is kept, so results for those inputs don't change.
- NULL handling doesn't change; all operators involved propagate NULL.
## What is the testing strategy for this PR?
New sqllogictest cases in `datafusion/sqllogictest/test_files/spark/datetime/unix.slt`:
- **Runtime path:** a table column (not just literals, so constant folding isn't the only path tested) holding pre-epoch values with and without a remainder, a positive value and NULL. Each is checked for both `unix_millis` and `unix_seconds`.
- **Constant-folded path:** the same cases written as literals.
- **Other input units:** `Timestamp(Nanosecond)` to millis and micros, `Timestamp(Millisecond)` to seconds, and an unchanged conversion to a finer unit (`Millisecond` to micros).
- **Extremes:** `i64::MIN` microseconds to seconds, and a large millisecond value to seconds, to guard against overflow.
Expected values for microsecond inputs came from Spark 4.0.0 (pyspark with `spark.sql.session.timeZone=UTC`), not from this implementation. Spark has no nanosecond timestamps, so the nanosecond cases use the same floor rule.
Local runs: all `spark/*` sqllogictests, `cargo test -p datafusion-spark`, `cargo fmt --all -- --check` and `cargo clippy -p datafusion-spark --all-targets -- -D warnings` pass
## Are there any user-facing changes?
Yes, as a bug fix. `unix_millis` and `unix_seconds` now return the Spark-compatible (floored) value for pre-epoch timestamps with a sub-unit component. No public API changes, so no `api change` label is needed.
---
AI assistance: I used an AI coding assistant to help find this divergence (by differential testing against Spark) and to draft the change. A separate AI review pass caught overflow edge cases, which the tests now cover. I reproduced the behavior against Spark 4.0.0 myself and reviewed the change line by line.
--
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(spark): floor instead of truncate in unix_millis/unix_seconds for pre-epoch timestamp [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26129:
URL: https://github.com/apache/datafusion/pull/26129#issuecomment-6363420124
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-spark v55.1.0 (current)
Built [ 47.813s] (current)
Parsing datafusion-spark v55.1.0 (current)
Parsed [ 0.075s] (current)
Building datafusion-spark v55.1.0 (baseline)
Built [ 47.687s] (baseline)
Parsing datafusion-spark v55.1.0 (baseline)
Parsed [ 0.077s] (baseline)
Checking datafusion-spark v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.573s] 223 checks: 220 pass, 3 fail, 0 warn, 31 skip
--- failure function_missing: pub fn removed or renamed ---
Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/function_missing.ron
Failed in:
function datafusion_spark::function::generator::stack, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/e9571568306d563be58383750c25ff4d1afc7dd2/datafusion/spark/src/function/generator/mod.rs:24
function datafusion_spark::function::generator::expr_fn::stack, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/e9571568306d563be58383750c25ff4d1afc7dd2/datafusion/spark/src/function/generator/mod.rs:29
function datafusion_spark::expr_fn::stack, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/e9571568306d563be58383750c25ff4d1afc7dd2/datafusion/spark/src/function/generator/mod.rs:29
--- failure module_missing: pub module removed or renamed ---
Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/module_missing.ron
Failed in:
mod datafusion_spark::function::generator::stack, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/e9571568306d563be58383750c25ff4d1afc7dd2/datafusion/spark/src/function/generator/stack.rs:18
--- failure struct_missing: pub struct removed or renamed ---
Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/struct_missing.ron
Failed in:
struct datafusion_spark::function::generator::stack::SparkStack, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/e9571568306d563be58383750c25ff4d1afc7dd2/datafusion/spark/src/function/generator/stack.rs:62
Summary semver requires new major version: 3 major and 0 minor checks failed
Finished [ 98.530s] datafusion-spark
Building datafusion-sqllogictest v55.1.0 (current)
Built [ 98.109s] (current)
Parsing datafusion-sqllogictest v55.1.0 (current)
Parsed [ 0.017s] (current)
Building datafusion-sqllogictest v55.1.0 (baseline)
Built [ 98.202s] (baseline)
Parsing datafusion-sqllogictest v55.1.0 (baseline)
Parsed [ 0.018s] (baseline)
Checking datafusion-sqllogictest v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.126s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 199.393s] datafusion-sqllogictest
```
</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(spark): floor instead of truncate in unix_millis/unix_seconds for pre-epoch timestamp [datafusion]
Posted by "harsh-ande (via GitHub)" <gi...@apache.org>.
harsh-ande commented on code in PR #26129:
URL: https://github.com/apache/datafusion/pull/26129#discussion_r4366461036
##########
datafusion/sqllogictest/test_files/spark/datetime/unix.slt:
##########
@@ -132,3 +132,51 @@ NULL
query error Function 'unix_seconds' requires Timestamp, but received String \(DataType: Utf8View\)
SELECT unix_seconds('1970-01-01 00:00:01Z'::string);
+
+# Spark floors (Math.floorDiv) when converting to a coarser unit, so pre-epoch
+# timestamps with a sub-unit remainder round toward negative infinity.
+statement ok
+CREATE TABLE unix_floor_us AS SELECT arrow_cast(column1, 'Timestamp(Microsecond, None)') AS ts FROM (VALUES
Review Comment:
Added raw `-1` microsecond tagged with `+02:00`. Both `unix_millis` and `unix_seconds` return `-1`, confirming the result is independent of timezone metadata.
--
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(spark): floor instead of truncate in unix_millis/unix_seconds for pre-epoch timestamp [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #26129:
URL: https://github.com/apache/datafusion/pull/26129
--
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(spark): floor instead of truncate in unix_millis/unix_seconds for pre-epoch timestamp [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #26129:
URL: https://github.com/apache/datafusion/pull/26129#discussion_r4357824811
##########
datafusion/spark/src/function/datetime/unix.rs:
##########
@@ -154,12 +154,38 @@ impl ScalarUDFImpl for SparkUnixTimestamp {
info: &SimplifyContext,
) -> Result<ExprSimplifyResult> {
let [ts] = take_function_args(self.name(), args)?;
+ // Arrow casts to a coarser timestamp unit truncate toward zero, but Spark
+ // floors (`Math.floorDiv`). When downscaling, divide the raw ticks and
+ // subtract one for negative values with a remainder.
+ let divisor: i64 = match (ts.get_type(info.schema())?, self.time_unit) {
+ (DataType::Timestamp(TimeUnit::Millisecond, _), TimeUnit::Second)
+ | (DataType::Timestamp(TimeUnit::Microsecond, _), TimeUnit::Millisecond)
+ | (DataType::Timestamp(TimeUnit::Nanosecond, _), TimeUnit::Microsecond) => {
+ 1_000
+ }
+ (DataType::Timestamp(TimeUnit::Microsecond, _), TimeUnit::Second)
+ | (DataType::Timestamp(TimeUnit::Nanosecond, _), TimeUnit::Millisecond) => {
+ 1_000_000
+ }
+ (DataType::Timestamp(TimeUnit::Nanosecond, _), TimeUnit::Second) => {
+ 1_000_000_000
+ }
+ _ => {
+ return Ok(ExprSimplifyResult::Simplified(
+ ts.cast_to(
+ &DataType::Timestamp(self.time_unit, Some("UTC".into())),
+ info.schema(),
+ )?
+ .cast_to(&DataType::Int64, info.schema())?,
+ ));
+ }
+ };
+ let ticks = ts.cast_to(&DataType::Int64, info.schema())?;
+ let correction = (ticks.clone() % lit(divisor))
+ .lt(lit(0_i64))
+ .cast_to(&DataType::Int64, info.schema())?;
Ok(ExprSimplifyResult::Simplified(
- ts.cast_to(
- &DataType::Timestamp(self.time_unit, Some("UTC".into())),
- info.schema(),
- )?
- .cast_to(&DataType::Int64, info.schema())?,
+ ticks / lit(divisor) - correction,
))
}
Review Comment:
The `ticks.clone()` rewrite evaluates volatile arguments twice, so `unix_millis` applied to a microsecond timestamp selected by `CASE WHEN random() < 0.5 THEN -1 ELSE 1000 END` can return `0` instead of only `-1` or `1`. CSE cannot safely merge volatile expressions. Please ensure the input is evaluated once, potentially through a runtime UDF/kernel, and add deterministic volatile-UDF regression tests for this and another downscaling variant.
##########
datafusion/sqllogictest/test_files/spark/datetime/unix.slt:
##########
@@ -132,3 +132,51 @@ NULL
query error Function 'unix_seconds' requires Timestamp, but received String \(DataType: Utf8View\)
SELECT unix_seconds('1970-01-01 00:00:01Z'::string);
+
+# Spark floors (Math.floorDiv) when converting to a coarser unit, so pre-epoch
+# timestamps with a sub-unit remainder round toward negative infinity.
+statement ok
+CREATE TABLE unix_floor_us AS SELECT arrow_cast(column1, 'Timestamp(Microsecond, None)') AS ts FROM (VALUES
Review Comment:
Consider adding a timezone-tagged timestamp with negative fractional ticks, such as raw `-1` cast to `Timestamp(Microsecond, Some("+02:00"))`. Both `unix_millis` and `unix_seconds` should return `-1`, confirming that bypassing the intermediate UTC cast preserves epoch-based results independently of timezone metadata.
##########
datafusion/sqllogictest/test_files/spark/datetime/unix.slt:
##########
@@ -132,3 +132,51 @@ NULL
query error Function 'unix_seconds' requires Timestamp, but received String \(DataType: Utf8View\)
SELECT unix_seconds('1970-01-01 00:00:01Z'::string);
+
+# Spark floors (Math.floorDiv) when converting to a coarser unit, so pre-epoch
+# timestamps with a sub-unit remainder round toward negative infinity.
+statement ok
+CREATE TABLE unix_floor_us AS SELECT arrow_cast(column1, 'Timestamp(Microsecond, None)') AS ts FROM (VALUES
+ (-1), (-1000), (-1001), (-1000000), (-1000001), (1500), (NULL)
+);
+
+query III
+SELECT arrow_cast(ts, 'Int64'), unix_millis(ts), unix_seconds(ts) FROM unix_floor_us;
+----
+-1 -1 -1
+-1000 -1 -1
+-1001 -2 -1
+-1000000 -1000 -1
+-1000001 -1001 -2
+1500 1 0
+NULL NULL NULL
+
+statement ok
+DROP TABLE unix_floor_us;
+
+# Same results when the expressions are constant-folded
+query II
+SELECT
+ unix_millis(arrow_cast(-1001, 'Timestamp(Microsecond, None)')),
+ unix_seconds(arrow_cast(-1000001, 'Timestamp(Microsecond, None)'));
+----
+-2 -2
+
+# Other input units: nanoseconds floor too, milliseconds to seconds floors,
+# and conversions to a finer unit are unchanged
+query IIII
+SELECT
+ unix_millis(arrow_cast(-1, 'Timestamp(Nanosecond, None)')),
Review Comment:
Consider covering nanoseconds to seconds, the only downscaling pair missing from these tests. For example, `unix_seconds(arrow_cast(-1000000001, 'Timestamp(Nanosecond, None)'))` should return `-2`, while an exact multiple of `-1000000000` should return `-1`.
--
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(spark): floor instead of truncate in unix_millis/unix_seconds for pre-epoch timestamp [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #26129:
URL: https://github.com/apache/datafusion/pull/26129#issuecomment-6366072543
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/26129?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `64.28571%` with `5 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.73%. Comparing base ([`521ba0e`](https://app.codecov.io/gh/apache/datafusion/commit/521ba0efaf9a28fdc02d380e28a27ac72192f11a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e273296`](https://app.codecov.io/gh/apache/datafusion/commit/e273296c28b631669a8002eaa65a85f6a9c3e29f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 230 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/26129?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/spark/src/function/datetime/unix.rs](https://app.codecov.io/gh/apache/datafusion/pull/26129?src=pr&el=tree&filepath=datafusion%2Fspark%2Fsrc%2Ffunction%2Fdatetime%2Funix.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zcGFyay9zcmMvZnVuY3Rpb24vZGF0ZXRpbWUvdW5peC5ycw==) | 64.28% | [1 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #26129 +/- ##
==========================================
- Coverage 82.73% 82.73% -0.01%
==========================================
Files 1147 1147
Lines 449540 449551 +11
Branches 449540 449551 +11
==========================================
+ Hits 371930 371933 +3
- Misses 54937 54942 +5
- Partials 22673 22676 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/26129?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.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</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(spark): floor instead of truncate in unix_millis/unix_seconds for pre-epoch timestamp [datafusion]
Posted by "harsh-ande (via GitHub)" <gi...@apache.org>.
harsh-ande commented on PR #26129:
URL: https://github.com/apache/datafusion/pull/26129#issuecomment-6508341374
Thank you @alamb and @kosiew! 🙂
--
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(spark): floor instead of truncate in unix_millis/unix_seconds for pre-epoch timestamp [datafusion]
Posted by "harsh-ande (via GitHub)" <gi...@apache.org>.
harsh-ande commented on code in PR #26129:
URL: https://github.com/apache/datafusion/pull/26129#discussion_r4366459736
##########
datafusion/sqllogictest/test_files/spark/datetime/unix.slt:
##########
@@ -132,3 +132,51 @@ NULL
query error Function 'unix_seconds' requires Timestamp, but received String \(DataType: Utf8View\)
SELECT unix_seconds('1970-01-01 00:00:01Z'::string);
+
+# Spark floors (Math.floorDiv) when converting to a coarser unit, so pre-epoch
+# timestamps with a sub-unit remainder round toward negative infinity.
+statement ok
+CREATE TABLE unix_floor_us AS SELECT arrow_cast(column1, 'Timestamp(Microsecond, None)') AS ts FROM (VALUES
+ (-1), (-1000), (-1001), (-1000000), (-1000001), (1500), (NULL)
+);
+
+query III
+SELECT arrow_cast(ts, 'Int64'), unix_millis(ts), unix_seconds(ts) FROM unix_floor_us;
+----
+-1 -1 -1
+-1000 -1 -1
+-1001 -2 -1
+-1000000 -1000 -1
+-1000001 -1001 -2
+1500 1 0
+NULL NULL NULL
+
+statement ok
+DROP TABLE unix_floor_us;
+
+# Same results when the expressions are constant-folded
+query II
+SELECT
+ unix_millis(arrow_cast(-1001, 'Timestamp(Microsecond, None)')),
+ unix_seconds(arrow_cast(-1000001, 'Timestamp(Microsecond, None)'));
+----
+-2 -2
+
+# Other input units: nanoseconds floor too, milliseconds to seconds floors,
+# and conversions to a finer unit are unchanged
+query IIII
+SELECT
+ unix_millis(arrow_cast(-1, 'Timestamp(Nanosecond, None)')),
Review Comment:
Added both cases: `-1000000001` nanoseconds returns `-2` seconds, and the exact multiple `-1000000000` returns `-1`.
--
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(spark): floor instead of truncate in unix_millis/unix_seconds for pre-epoch timestamp [datafusion]
Posted by "harsh-ande (via GitHub)" <gi...@apache.org>.
harsh-ande commented on code in PR #26129:
URL: https://github.com/apache/datafusion/pull/26129#discussion_r4366458209
##########
datafusion/spark/src/function/datetime/unix.rs:
##########
@@ -154,12 +154,38 @@ impl ScalarUDFImpl for SparkUnixTimestamp {
info: &SimplifyContext,
) -> Result<ExprSimplifyResult> {
let [ts] = take_function_args(self.name(), args)?;
+ // Arrow casts to a coarser timestamp unit truncate toward zero, but Spark
+ // floors (`Math.floorDiv`). When downscaling, divide the raw ticks and
+ // subtract one for negative values with a remainder.
+ let divisor: i64 = match (ts.get_type(info.schema())?, self.time_unit) {
+ (DataType::Timestamp(TimeUnit::Millisecond, _), TimeUnit::Second)
+ | (DataType::Timestamp(TimeUnit::Microsecond, _), TimeUnit::Millisecond)
+ | (DataType::Timestamp(TimeUnit::Nanosecond, _), TimeUnit::Microsecond) => {
+ 1_000
+ }
+ (DataType::Timestamp(TimeUnit::Microsecond, _), TimeUnit::Second)
+ | (DataType::Timestamp(TimeUnit::Nanosecond, _), TimeUnit::Millisecond) => {
+ 1_000_000
+ }
+ (DataType::Timestamp(TimeUnit::Nanosecond, _), TimeUnit::Second) => {
+ 1_000_000_000
+ }
+ _ => {
+ return Ok(ExprSimplifyResult::Simplified(
+ ts.cast_to(
+ &DataType::Timestamp(self.time_unit, Some("UTC".into())),
+ info.schema(),
+ )?
+ .cast_to(&DataType::Int64, info.schema())?,
+ ));
+ }
+ };
+ let ticks = ts.cast_to(&DataType::Int64, info.schema())?;
+ let correction = (ticks.clone() % lit(divisor))
+ .lt(lit(0_i64))
+ .cast_to(&DataType::Int64, info.schema())?;
Ok(ExprSimplifyResult::Simplified(
- ts.cast_to(
- &DataType::Timestamp(self.time_unit, Some("UTC".into())),
- info.schema(),
- )?
- .cast_to(&DataType::Int64, info.schema())?,
+ ticks / lit(divisor) - correction,
))
}
Review Comment:
Replaced the duplicated-expression rewrite with runtime floor division on the already-evaluated argument. Same-unit and upscaling conversions retain the existing cast simplification. Added deterministic volatile-UDF regressions covering all six downscaling pairs with scalar and array inputs, checking both results and exactly one evaluation.
--
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