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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
