Data, Spark: Fix FIXED type conversion in format model TCK - #17482
Open
joyhaldar wants to merge 1 commit into
Open
Data, Spark: Fix FIXED type conversion in format model TCK#17482joyhaldar wants to merge 1 commit into
joyhaldar wants to merge 1 commit into
Conversation
joyhaldar
marked this pull request as ready for review
August 3, 2026 03:30
joyhaldar
force-pushed
the
spark-fixed-type-converter
branch
from
August 3, 2026 03:30
71520b3 to
f19ea71
Compare
joyhaldar
changed the title
Guosmilesmile
approved these changes
Aug 3, 2026
Guosmilesmile
left a comment
Guosmilesmile
left a comment
Contributor
There was a problem hiding this comment.
Thank you very much for your PR. Overall, LGTM!
| if (value instanceof byte[]) { | ||
| return (byte[]) value; | ||
| } | ||
| return ByteBuffers.toByteArray((ByteBuffer) value); |
| if (value instanceof byte[]) { | ||
| return (byte[]) value; | ||
| } | ||
| return ByteBuffers.toByteArray((ByteBuffer) value); |
| if (value instanceof byte[]) { | ||
| return (byte[]) value; | ||
| } | ||
| return ByteBuffers.toByteArray((ByteBuffer) value); |
joyhaldar
force-pushed
the
spark-fixed-type-converter
branch
from
August 3, 2026 03:46
f19ea71 to
5dca7d0
Compare
Contributor
Author
Addressed the nits, thank you very much for the review! |
uros-b
approved these changes
Aug 3, 2026
pvary
reviewed
Aug 3, 2026
Comment on lines
+111
to
+115
| if (value instanceof byte[]) { | ||
| return (byte[]) value; | ||
| } | ||
|
|
||
| return ByteBuffers.toByteArray((ByteBuffer) value); |
Contributor
There was a problem hiding this comment.
Maybe:
Suggested change
| if (value instanceof byte[]) { | |
| return (byte[]) value; | |
| } | |
| return ByteBuffers.toByteArray((ByteBuffer) value); | |
| return value instanceof byte[] bytes ? bytes : ByteBuffers.toByteArray((ByteBuffer) value); |
or
Suggested change
| if (value instanceof byte[]) { | |
| return (byte[]) value; | |
| } | |
| return ByteBuffers.toByteArray((ByteBuffer) value); | |
| if (value instanceof byte[] bytes) { | |
| return bytes; | |
| } else if (value instanceof ByteBuffer buffer) { | |
| return ByteBuffers.toByteArray(buffer); | |
| } | |
| throw new UnsupportedOperationException( | |
| "Unsupported binary value class: " + value.getClass().getName()); | |
Contributor
Author
There was a problem hiding this comment.
Thank you for your review @pvary. Went with the second option. Updated across all three Spark versions.
Co-authored-by: Joy Haldar <joy.haldar@target.com>
joyhaldar
force-pushed
the
spark-fixed-type-converter
branch
from
August 3, 2026 13:23
5dca7d0 to
c435fd9
Compare
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
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.
InternalRowConverter cast both
FIXEDandBINARYto ByteBuffer, but Iceberg's generic model represents fixed asbyte[], leading toClassCastException.FIXEDwas excluded from the Spark TCK as a result.toByteArrayhandles both, delegating to ByteBuffers.toByteArray for the buffer case.FIXEDfrom unsupportedTypeIds() in Spark3.5/4.0/4.1.fixed_with_defaultto DataGenerators.PrimitiveDefaults, exercising both branches alongside binary_with_default.Refs: GenericDataUtil.internalToGeneric, RandomGenericData.