[SPARK-51042][SQL] Read and write the month and days fields of intervals with one call in Unsafe* classes #49737
Conversation
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
…m.putLong() instead of two calls to Platform.putInt(). This makes writing intervals consistent with the way they are written and read in other places like UnsafeRow.java and should have the same or better performance compared to the original code. It also makes the code endian independent so it works on big and little endian platforms. After this change, all interval related tests are able to pass on big endian platforms. Signed-off-by: Jonathan Albrecht <jonathan.albrecht@ibm.com>
MaxGekk
approved these changes
Feb 6, 2025
Member
MaxGekk
left a comment
MaxGekk
left a comment
There was a problem hiding this comment.
I think it is ok to align the implementation to UnsafeRow.java, and fix a big-endian issue.
Member
|
+1, LGTM. Merging to master/4.0. |
MaxGekk
pushed a commit
that referenced
this pull request
Feb 6, 2025
…als with one call in Unsafe* classes ### What changes were proposed in this pull request? Write the month and days fields of intervals with one call to Platform.put/getLong() instead of two calls to Platform.put/getInt(). In commit ac07cea there was a performance improvement to reading a writing CalendarIntervals in UnsafeRow. This makes writing intervals consistent with UnsafeRow and has better performance compared to the original code. This also fixes big endian platforms where the old (two calls to getput) and new methods of reading and writing CalendarIntervals do not order the bytes in the same way. Currently CalendarInterval related tests in Catalyst and SQL are failing on big endian platforms. There is no effect on little endian platforms (byte order is not affected) except for performance improvement. ### Why are the changes needed? * Improves performance reading and writing CalendarIntervals in Unsafe* classes * Fixes big endian platforms where CalendarIntervals are not read or written correctly in Unsafe* classes ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing unit tests on big and little endian platforms ### Was this patch authored or co-authored using generative AI tooling? No Closes #49737 from jonathan-albrecht-ibm/master-endian-interval. Authored-by: Jonathan Albrecht <jonathan.albrecht@ibm.com> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit a79ba48) Signed-off-by: Max Gekk <max.gekk@gmail.com>
Member
|
@jonathan-albrecht-ibm Congratulations with your first contribution to Apache Spark! |
Contributor
Author
|
@MaxGekk Thanks for reviewing and merging! |
Contributor
Author
|
@MaxGekk Would it be possible to merge this to branch-3.5 as well? I have also built and tested it on a local build of 3.5.4. More generally, should I mention that I'd like to merge a PR back as far as branch-3.5 in the PR description or in the JIRA or somewhere else? I didn't see any info on how to ask for which branches to merge to and not sure what the policies are. |
Member
I think so. In the public doc we declare: see https://spark.apache.org/docs/latest/
Apparently, we can consider the changes as a bug fix. cc @dongjoon-hyun @cloud-fan WDYT? |
Contributor
|
+1 to backport |
Contributor
Author
|
@MaxGekk, @cloud-fan Thanks for considering this to be backported. Should I open a new PR against branch-3.5? The cherry-pick of a79ba48 applies cleanly on branch-3.5 |
Member
|
+1 for backporting. Thank you for pinging me, @MaxGekk . To @jonathan-albrecht-ibm , yes, we need a new PR in order to make it sure that it passes in |
jonathan-albrecht-ibm
added a commit
to linux-on-ibm-z/spark
that referenced
this pull request
Feb 12, 2025
…als with one call in Unsafe* classes ### What changes were proposed in this pull request? Write the month and days fields of intervals with one call to Platform.put/getLong() instead of two calls to Platform.put/getInt(). In commit ac07cea there was a performance improvement to reading a writing CalendarIntervals in UnsafeRow. This makes writing intervals consistent with UnsafeRow and has better performance compared to the original code. This also fixes big endian platforms where the old (two calls to getput) and new methods of reading and writing CalendarIntervals do not order the bytes in the same way. Currently CalendarInterval related tests in Catalyst and SQL are failing on big endian platforms. There is no effect on little endian platforms (byte order is not affected) except for performance improvement. ### Why are the changes needed? * Improves performance reading and writing CalendarIntervals in Unsafe* classes * Fixes big endian platforms where CalendarIntervals are not read or written correctly in Unsafe* classes ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing unit tests on big and little endian platforms ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#49737 from jonathan-albrecht-ibm/master-endian-interval. Authored-by: Jonathan Albrecht <jonathan.albrecht@ibm.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>
dongjoon-hyun
pushed a commit
that referenced
this pull request
Feb 12, 2025
…ntervals with one call in Unsafe* classes This is the branch-3.5 backport of #49737 ### What changes were proposed in this pull request? Write the month and days fields of intervals with one call to Platform.put/getLong() instead of two calls to Platform.put/getInt(). In commit ac07cea there was a performance improvement to reading a writing CalendarIntervals in UnsafeRow. This makes writing intervals consistent with UnsafeRow and has better performance compared to the original code. This also fixes big endian platforms where the old (two calls to getput) and new methods of reading and writing CalendarIntervals do not order the bytes in the same way. Currently CalendarInterval related tests in Catalyst and SQL are failing on big endian platforms. There is no effect on little endian platforms (byte order is not affected) except for performance improvement. ### Why are the changes needed? * Improves performance reading and writing CalendarIntervals in Unsafe* classes * Fixes big endian platforms where CalendarIntervals are not read or written correctly in Unsafe* classes ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing unit tests on big and little endian platforms ### Was this patch authored or co-authored using generative AI tooling? No Closes #49912 from jonathan-albrecht-ibm/branch-3.5-endian-interval. Authored-by: Jonathan Albrecht <jonathan.albrecht@ibm.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
zifeif2
pushed a commit
to zifeif2/spark
that referenced
this pull request
Nov 14, 2025
…als with one call in Unsafe* classes ### What changes were proposed in this pull request? Write the month and days fields of intervals with one call to Platform.put/getLong() instead of two calls to Platform.put/getInt(). In commit 86d3e80 there was a performance improvement to reading a writing CalendarIntervals in UnsafeRow. This makes writing intervals consistent with UnsafeRow and has better performance compared to the original code. This also fixes big endian platforms where the old (two calls to getput) and new methods of reading and writing CalendarIntervals do not order the bytes in the same way. Currently CalendarInterval related tests in Catalyst and SQL are failing on big endian platforms. There is no effect on little endian platforms (byte order is not affected) except for performance improvement. ### Why are the changes needed? * Improves performance reading and writing CalendarIntervals in Unsafe* classes * Fixes big endian platforms where CalendarIntervals are not read or written correctly in Unsafe* classes ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing unit tests on big and little endian platforms ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#49737 from jonathan-albrecht-ibm/master-endian-interval. Authored-by: Jonathan Albrecht <jonathan.albrecht@ibm.com> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit 05ebdf1) Signed-off-by: Max Gekk <max.gekk@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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?
Write the month and days fields of intervals with one call to Platform.put/getLong() instead of two calls to Platform.put/getInt().
In commit ac07cea there was a performance improvement to reading a writing CalendarIntervals in UnsafeRow. This makes writing intervals consistent with UnsafeRow and has better performance compared to the original code.
This also fixes big endian platforms where the old (two calls to getput) and new methods of reading and writing CalendarIntervals do not order the bytes in the same way. Currently CalendarInterval related tests in Catalyst and SQL are failing on big endian platforms.
There is no effect on little endian platforms (byte order is not affected) except for performance improvement.
Why are the changes needed?
Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing unit tests on big and little endian platforms
Was this patch authored or co-authored using generative AI tooling?
No