You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org> on 2025/01/30 19:19:22 UTC
[PR] [SPARK-51042][SQL] Read and write the month and days fields of intervals with one call in Unsafe* classes [spark]
jonathan-albrecht-ibm opened a new pull request, #49737:
URL: https://github.com/apache/spark/pull/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 ac07cea234f4fb687442aafa8b6d411695110a4e 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.
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### 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
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
No
<!--
Note that it means *any* user-facing change including all aspects such as new features, bug fixes, or other behavior changes. Documentation-only updates are not considered user-facing changes.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
Existing unit tests on big and little endian platforms
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
No
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51042][SQL] Read and write the month and days fields of intervals with one call in Unsafe* classes [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #49737: [SPARK-51042][SQL] Read and write the month and days fields of intervals with one call in Unsafe* classes
URL: https://github.com/apache/spark/pull/49737
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51042][SQL] Read and write the month and days fields of intervals with one call in Unsafe* classes [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #49737:
URL: https://github.com/apache/spark/pull/49737#issuecomment-2640820183
@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.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51042][SQL] Read and write the month and days fields of intervals with one call in Unsafe* classes [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #49737:
URL: https://github.com/apache/spark/pull/49737#issuecomment-2654434532
@MaxGekk, @cloud-fan Thanks for considering this to be backported. Should I open a new PR against branch-3.5? The cherry-pick of a79ba486766bd47c26879231cd4292ca79c926fa applies cleanly on branch-3.5
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51042][SQL] Read and write the month and days fields of intervals with one call in Unsafe* classes [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #49737:
URL: https://github.com/apache/spark/pull/49737#issuecomment-2642629670
+1 to backport
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51042][SQL] Read and write the month and days fields of intervals with one call in Unsafe* classes [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #49737:
URL: https://github.com/apache/spark/pull/49737#issuecomment-2640461861
+1, LGTM. Merging to master/4.0.
Thank you, @jonathan-albrecht-ibm.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51042][SQL] Read and write the month and days fields of intervals with one call in Unsafe* classes [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #49737:
URL: https://github.com/apache/spark/pull/49737#issuecomment-2642463370
> Would it be possible to merge this to branch-3.5 as well?
I think so. In the public doc we declare:
```
Spark runs on both Windows and UNIX-like systems (e.g. Linux, Mac OS), and it should run on any platform that runs a supported version of Java.
```
see https://spark.apache.org/docs/latest/
> Fixes big endian platforms where CalendarIntervals are not read or written correctly in Unsafe* classes
Apparently, we can consider the changes as a bug fix. cc @dongjoon-hyun @cloud-fan WDYT?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51042][SQL] Read and write the month and days fields of intervals with one call in Unsafe* classes [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #49737:
URL: https://github.com/apache/spark/pull/49737#issuecomment-2640466606
@jonathan-albrecht-ibm Congratulations with your first contribution to Apache Spark!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51042][SQL] Read and write the month and days fields of intervals with one call in Unsafe* classes [spark]
Posted by "jonathan-albrecht-ibm (via GitHub)" <gi...@apache.org>.
jonathan-albrecht-ibm commented on PR #49737:
URL: https://github.com/apache/spark/pull/49737#issuecomment-2640748852
@MaxGekk Thanks for reviewing and merging!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51042][SQL] Read and write the month and days fields of intervals with one call in Unsafe* classes [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #49737:
URL: https://github.com/apache/spark/pull/49737#issuecomment-2654465462
+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 `branch-3.5`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org