[SPARK-57103][SQL] Add Comparable to TimestampNanosVal#56187
Closed
stevomitric wants to merge 2 commits into
Closed
[SPARK-57103][SQL] Add Comparable to TimestampNanosVal#56187stevomitric wants to merge 2 commits into
stevomitric wants to merge 2 commits into
Conversation
### What changes were proposed in this pull request? Make `TimestampNanosVal` implement `Comparable<TimestampNanosVal>`, ordered lexicographically by `epochMicros` then `nanosWithinMicro`. This is the first of a planned three-PR breakdown of SPARK-57103 (ordering + compare + hash for nanosecond timestamp types). Follow-ups will (1) wire `PhysicalDataType.ordering` for the two nanos physical types and (2) extend the hash expressions (`hash`, `xxhash64`, `murmur3`). ### Why are the changes needed? `TimestampNanosVal` (added in SPARK-56981) is the physical value class for `TimestampNTZNanosType(p)` / `TimestampLTZNanosType(p)`. Without a `compareTo`, Catalyst cannot give it an `Ordering`, which blocks `ORDER BY`, sort-merge join, sort-based `GROUP BY`, `DISTINCT`, and any other operator that needs total order on the type. ### Does this PR introduce _any_ user-facing change? No. `TimestampNanosVal` is `@Unstable` and the new nanos types remain gated by `spark.sql.timestampNanosTypes.enabled`. This PR only adds a `compareTo` on the value class. ### How was this patch tested? Six new unit tests in `TimestampNanosSuite` covering: - primary/secondary ordering on `(epochMicros, nanosWithinMicro)`, - consistency with `equals` (`a.compareTo(b) == 0` iff `a.equals(b)`), - `Long.MIN_VALUE` / `Long.MAX_VALUE` boundaries (Long.compare protects against the subtraction-overflow pitfall), - pre-epoch negative values, - antisymmetry and transitivity, - `Arrays.sort` integration end-to-end. `build/mvn -pl common/unsafe -am test -Dtest=TimestampNanosSuite`: Tests run: 12, Failures: 0, Errors: 0, Skipped: 0. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 4.7)
stevomitric
changed the title
Contributor
Author
|
@MaxGekk please take a look. |
MaxGekk
reviewed
May 28, 2026
Member
MaxGekk
left a comment
MaxGekk
left a comment
There was a problem hiding this comment.
Could you add a test for null. According to the doc of Comparable, compareTo shall throw NPE on null, see
Throws:
NullPointerException – if the specified object is null
Address review feedback from @MaxGekk: assert that TimestampNanosVal.compareTo(null) throws NullPointerException, matching the Comparable contract. The behavior was already correct (NPE on `that.epochMicros` dereference); this change only adds the test.
MaxGekk
approved these changes
May 29, 2026
Contributor
Author
Added. Waiting for CI. |
Member
|
+1, LGTM. Merging to master/4.x. |
MaxGekk
pushed a commit
that referenced
this pull request
May 29, 2026
### What changes were proposed in this pull request? Make `TimestampNanosVal` implement `Comparable<TimestampNanosVal>`, ordered lexicographically by `epochMicros` then `nanosWithinMicro`. This is the first of SPARK-57103 (ordering + compare + hash for nanosecond timestamp types). Follow-ups will - wire `PhysicalDataType.ordering` for the two nanos physical types - extend the hash expressions (`hash`, `xxhash64`, `murmur3`). ### Why are the changes needed? `TimestampNanosVal` (added in SPARK-56981) is the physical value class for `TimestampNTZNanosType(p)` / `TimestampLTZNanosType(p)`. Without a `compareTo`, Catalyst cannot give it an `Ordering`, which blocks `ORDER BY`, sort-merge join, sort-based `GROUP BY`, `DISTINCT`, and any other operator that needs total order on the type. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? New UT in this PR. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 4.7) Closes #56187 from stevomitric/stevomitric/SPARK-57103-compare. Authored-by: Stevo Mitric <stevomitric2000@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit 0b0ffb7) Signed-off-by: Max Gekk <max.gekk@gmail.com>
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
2 participants
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.
What changes were proposed in this pull request?
Make
TimestampNanosValimplementComparable<TimestampNanosVal>, ordered lexicographically byepochMicrosthennanosWithinMicro.This is the first of SPARK-57103 (ordering + compare + hash for nanosecond timestamp types). Follow-ups will
PhysicalDataType.orderingfor the two nanos physical typeshash,xxhash64,murmur3).Why are the changes needed?
TimestampNanosVal(added in SPARK-56981) is the physical value class forTimestampNTZNanosType(p)/TimestampLTZNanosType(p). Without acompareTo, Catalyst cannot give it anOrdering, which blocksORDER BY, sort-merge join, sort-basedGROUP BY,DISTINCT, and any other operator that needs total order on the type.Does this PR introduce any user-facing change?
No.
How was this patch tested?
New UT in this PR.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.7)