Repository navigation
fix(spark): floor instead of truncate in unix_millis/unix_seconds for pre-epoch timestamp - #26129
Open
harsh-ande wants to merge 1 commit into
Open
harsh-ande wants to merge 1 commit into
harsh-ande wants to merge 1 commit into
Conversation
… pre-epoch timestamps
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
The Spark-compatible
unix_millisandunix_secondsfunctions 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_millisof a timestamp 1 µs before the epoch is-1in Spark and0in DataFusion. Results for non-negative timestamps and exact multiples already match.The cause is that
SparkUnixTimestamp::simplifyrewrites the call as a cast to a coarserTimestampunit, and Arrow's cast truncates toward zero. Spark computes these values withMath.floorDiv(TimestampToLongBase).What changes are included in this PR?
In
SparkUnixTimestamp::simplify(datafusion/spark/src/function/datetime/unix.rs):Int64ticks are divided by the unit ratio (1e3, 1e6 or 1e9). Then1is subtracted when the remainder is negative:ticks / d - CAST(ticks % d < 0 AS BIGINT). This is an exact floor division over the fulli64range, with no intermediate unit conversion, so it adds no new overflow cases.What is the testing strategy for this PR?
New sqllogictest cases in
datafusion/sqllogictest/test_files/spark/datetime/unix.slt:unix_millisandunix_seconds.Timestamp(Nanosecond)to millis and micros,Timestamp(Millisecond)to seconds, and an unchanged conversion to a finer unit (Millisecondto micros).i64::MINmicroseconds 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 -- --checkandcargo clippy -p datafusion-spark --all-targets -- -D warningspassAre there any user-facing changes?
Yes, as a bug fix.
unix_millisandunix_secondsnow return the Spark-compatible (floored) value for pre-epoch timestamps with a sub-unit component. No public API changes, so noapi changelabel 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.