[SPARK-56822][SQL] Zero the reserved payload when writing a null nanosecond timestamp to UnsafeRow - #58746
Open
stevomitric wants to merge 2 commits into
Open
Conversation
…second timestamp to UnsafeRow ### What changes were proposed in this pull request? `UnsafeWriter.write(int, TimestampNanosVal)` stores a nanosecond timestamp as a 16-byte variable-length payload (modeled on `CalendarInterval`) and reserves that space even for a null value so the slot can be updated in place later. On the null branch it set the null bit but left the reserved 16 bytes untouched. The writer's buffer is reused across rows, so a null nanosecond value inherited whatever bytes the previously written row left in that slot. Two logically-equal null rows could therefore produce different `UnsafeRow` byte contents, breaking the invariant that equal rows encode identically -- the invariant that `UnsafeRow` hashing and equality rely on. This is the root cause of a nullable nanosecond-timestamp GROUP BY / join key splitting its NULLs across multiple groups. The fix zeroes the reserved payload on the null branch, mirroring the in-place update path (`UnsafeRow.setTimestampNTZNanos` / `setTimestampLTZNanos`), which already calls `TimestampNanosRowValues.zeroPayload`. ### Why are the changes needed? Correctness: a null nanosecond timestamp must encode canonically so that null grouping/join keys compare and hash identically. Without it, `GROUP BY` (and any key-based operator) over a nullable `TIMESTAMP_NTZ(p)` / `TIMESTAMP_LTZ(p)` column can silently scatter NULL rows across several groups. The array path (`UnsafeArrayWriter`) is unaffected: it writes a null element through `setNull8Bytes`, which zeroes the element's offset-and-size slot (size 0). ### Does this PR introduce any user-facing change? No change in a default configuration -- the nanosecond timestamp types are behind the `spark.sql.timestampNanosTypes.enabled` preview flag (off by default). With the flag enabled, null nanosecond keys now group and compare canonically. ### How was this patch tested? New `UnsafeRowConverterSuite` test asserting that two null nanosecond projections built after different non-null values are byte-identical, in both the interpreted and the codegen paths. It fails without this change and passes with it. Co-authored-by: Isaac <no-reply@databricks.com>
…nce in the comment The added comment referenced a nonexistent `UnsafeRow#setTimestampNanos`; the method it mirrors is the private `UnsafeRow#setTimestampNanosPayload`, which zeroes the payload on the null-update path. Correct the cross-reference so it is greppable. Co-authored-by: Isaac <no-reply@databricks.com>
Contributor
Author
|
cc @uros-b PTAL. |
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
1 participant
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?
UnsafeWriter.write(int, TimestampNanosVal)stores a nanosecond timestamp as a 16-byte variable-length payload (modeled onCalendarInterval) and reserves that space even for a null value so the slot can be updated in place later. On the null branch it set the null bit but left the reserved 16 bytes untouched. The writer's buffer is reused across rows, so a null nanosecond value inherited whatever bytes the previously written row left in that slot.Two logically-equal null rows could therefore produce different
UnsafeRowbyte contents, breaking the invariant that equal rows encode identically -- the invariant thatUnsafeRowhashing and equality rely on. This is the root cause of a nullable nanosecond-timestamp GROUP BY / join key splitting its NULLs across multiple groups.The fix zeroes the reserved payload on the null branch, mirroring the in-place update path (
UnsafeRow.setTimestampNTZNanos/setTimestampLTZNanos), which already callsTimestampNanosRowValues.zeroPayload.Why are the changes needed?
Correctness: a null nanosecond timestamp must encode canonically so that null grouping/join keys compare and hash identically. Without it,
GROUP BY(and any key-based operator) over a nullableTIMESTAMP_NTZ(p)/TIMESTAMP_LTZ(p)column can silently scatter NULL rows across several groups. The array path (UnsafeArrayWriter) is unaffected: it writes a null element throughsetNull8Bytes, which zeroes the element's offset-and-size slot (size 0).Does this PR introduce any user-facing change?
No change in a default configuration -- the nanosecond timestamp types are behind the
spark.sql.timestampNanosTypes.enabledpreview flag (off by default). With the flag enabled, null nanosecond keys now group and compare canonically.How was this patch tested?
New
UnsafeRowConverterSuitetest asserting that two null nanosecond projections built after different non-null values are byte-identical, in both the interpreted and the codegen paths. It fails without this change and passes with it.What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?