[SPARK-51093][SQL][TESTS] Fix minor endianness issues in tests. #49812
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
ArrayBasedMapBuilderSuite: The output of the UnsafeRow.toString() is based on the underlying bytes and is endian dependent. Add an expected value for big endian platforms. WriteDistributionAndOrderingSuite: Casting the id of type Int to Long doesn't work on big endian platforms because the BucketFunction calls UnsafeRow.getLong() for that column. That happens to work on little endian since an int field is stored in the first 4 bytes of the 8 byte field so positive ints are layed out the same as positive longs ie. little endian order. On big endian, the layout of UnsafeRow int fields does not happen to match the layout of long fields for the same number. Change the type of the id column to Long so that it matches what BucketFunction expects. Signed-off-by: Jonathan Albrecht <jonathan.albrecht@ibm.com>
jonathan-albrecht-ibm
changed the title
MaxGekk
reviewed
Feb 5, 2025
Member
MaxGekk
left a comment
MaxGekk
left a comment
There was a problem hiding this comment.
Could you add the [TESTS] tag to PR's title, please.
MaxGekk
reviewed
Feb 5, 2025
| condition = "DUPLICATED_MAP_KEY", | ||
| parameters = Map( | ||
| "key" -> "[0,1]", | ||
| "key" -> keyAsString, |
Member
There was a problem hiding this comment.
Can't you just replace [0,1] by unsafeRow.toString?
Contributor
Author
There was a problem hiding this comment.
Thanks @MaxGekk! Yes, that's much better. I've pushed the change
jonathan-albrecht-ibm
changed the title
…t literal expected values Signed-off-by: Jonathan Albrecht <jonathan.albrecht@ibm.com>
Contributor
Author
|
The CI failed on a pyspark mllib test with: So its not related to this change. All other tests look like they passed if I'm reading it correctly. |
Member
Highly likely you are right, but this particular GitHub action stopped and didn't run the rest tests that might be related to your changes. May I ask you to re-run only the failed GA (you can do that in UI). |
Contributor
Author
|
Thanks @MaxGekk, I didn't know I could do that. I reran the failing build and it passed this time |
Member
|
+1, LGTM. Merging to master/4.0. |
MaxGekk
pushed a commit
that referenced
this pull request
Feb 7, 2025
### What changes were proposed in this pull request? Fix minor endianness issues in the following tests. ArrayBasedMapBuilderSuite: The output of the UnsafeRow.toString() is based on the underlying bytes and is endian dependent. Add an expected value for big endian platforms. Add an expected value for big endian platforms. WriteDistributionAndOrderingSuite: Casting the id of type Int to Long doesn't work on big endian platforms because the BucketFunction calls UnsafeRow.getLong() for that column. That happens to work on little endian since an int field is stored in the first 4 bytes of the 8 byte field so positive ints are layed out the same as positive longs ie. little endian order. On big endian, the layout of UnsafeRow int fields does not happen to match the layout of long fields for the same number. Change the type of the id column to Long so that it matches what BucketFunction expects. Change the type of the id column to Long so that it matches what BucketFunction expects. ### Why are the changes needed? Allow tests to pass on big endian platforms ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Ran existing tests on amd64 (little endian) and s390x (big endian) ### Was this patch authored or co-authored using generative AI tooling? No Closes #49812 from jonathan-albrecht-ibm/master-endian-testEndianness. Authored-by: Jonathan Albrecht <jonathan.albrecht@ibm.com> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit f5f7c36) Signed-off-by: Max Gekk <max.gekk@gmail.com>
Contributor
Author
|
@MaxGekk Thanks for reviewing and merging! |
Member
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Thank you, @jonathan-albrecht-ibm and @MaxGekk .
BTW, can we have this in branch-3.5 too?
Member
I didn't merge to 3.5 because the changes cause conflicts in |
dongjoon-hyun
pushed a commit
that referenced
this pull request
Feb 10, 2025
This is the branch-3.5 backport of #49812 ### What changes were proposed in this pull request? Fix minor endianness issues in the following tests. ArrayBasedMapBuilderSuite: The output of the UnsafeRow.toString() is based on the underlying bytes and is endian dependent. Add an expected value for big endian platforms. Add an expected value for big endian platforms. WriteDistributionAndOrderingSuite: Casting the id of type Int to Long doesn't work on big endian platforms because the BucketFunction calls UnsafeRow.getLong() for that column. That happens to work on little endian since an int field is stored in the first 4 bytes of the 8 byte field so positive ints are layed out the same as positive longs ie. little endian order. On big endian, the layout of UnsafeRow int fields does not happen to match the layout of long fields for the same number. Change the type of the id column to Long so that it matches what BucketFunction expects. Change the type of the id column to Long so that it matches what BucketFunction expects. ### Why are the changes needed? Allow tests to pass on big endian platforms ### Does this PR introduce any user-facing change? No ### How was this patch tested? Ran existing tests on amd64 (little endian) and s390x (big endian) ### Was this patch authored or co-authored using generative AI tooling? No Closes #49866 from jonathan-albrecht-ibm/branch-3.5-endian-testEndianness. 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
### What changes were proposed in this pull request? Fix minor endianness issues in the following tests. ArrayBasedMapBuilderSuite: The output of the UnsafeRow.toString() is based on the underlying bytes and is endian dependent. Add an expected value for big endian platforms. Add an expected value for big endian platforms. WriteDistributionAndOrderingSuite: Casting the id of type Int to Long doesn't work on big endian platforms because the BucketFunction calls UnsafeRow.getLong() for that column. That happens to work on little endian since an int field is stored in the first 4 bytes of the 8 byte field so positive ints are layed out the same as positive longs ie. little endian order. On big endian, the layout of UnsafeRow int fields does not happen to match the layout of long fields for the same number. Change the type of the id column to Long so that it matches what BucketFunction expects. Change the type of the id column to Long so that it matches what BucketFunction expects. ### Why are the changes needed? Allow tests to pass on big endian platforms ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Ran existing tests on amd64 (little endian) and s390x (big endian) ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#49812 from jonathan-albrecht-ibm/master-endian-testEndianness. Authored-by: Jonathan Albrecht <jonathan.albrecht@ibm.com> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit 9237eae) 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
3 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?
Fix minor endianness issues in the following tests.
ArrayBasedMapBuilderSuite: The output of the UnsafeRow.toString() is based on the underlying bytes and is endian dependent. Add an expected value for big endian platforms. Add an expected value for big endian platforms.
WriteDistributionAndOrderingSuite: Casting the id of type Int to Long doesn't work on big endian platforms because the BucketFunction calls UnsafeRow.getLong() for that column. That happens to work on little endian since an int field is stored in the first 4 bytes of the 8 byte field so positive ints are layed out the same as positive longs ie. little endian order. On big endian, the layout of UnsafeRow int fields does not happen to match the layout of long fields for the same number. Change the type of the id column to Long so that it matches what BucketFunction expects. Change the type of the id column to Long so that it matches what BucketFunction expects.
Why are the changes needed?
Allow tests to pass on big endian platforms
Does this PR introduce any user-facing change?
No
How was this patch tested?
Ran existing tests on amd64 (little endian) and s390x (big endian)
Was this patch authored or co-authored using generative AI tooling?
No