Skip to content

Conversation

@nandorKollar
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the arrow label Jul 29, 2025
@pvary
Copy link
Contributor

pvary commented Jul 31, 2025

@nastra: In #3029 you were working on the Fixed type, but abandoned it, because

TBH I'm uncertain the approach is correct or not because I'm not sure if this statement is still correct (https://github.com/apache/iceberg/pull/3029/files#diff-80bc724de9a4dd358c4544fcf00e00139145c6763a5d0280e0bd0793a0fd4003L366-L368)

OTOH, the Fixed type seems to be supported by the Arrow reader.
Do you happen to remember a bit more about the history here?

Thanks,
Peter

@nastra
Copy link
Contributor

nastra commented Aug 1, 2025

@nastra: In #3029 you were working on the Fixed type, but abandoned it, because

TBH I'm uncertain the approach is correct or not because I'm not sure if this statement is still correct (https://github.com/apache/iceberg/pull/3029/files#diff-80bc724de9a4dd358c4544fcf00e00139145c6763a5d0280e0bd0793a0fd4003L366-L368)

OTOH, the Fixed type seems to be supported by the Arrow reader. Do you happen to remember a bit more about the history here?

Back then I wasn't sure whether we could just remove FixedWidthBinaryPageReader from the codebase. Over time we did actually remove it and so I think we should be able to fully support FIXED type

* Types.FixedType} and {@link Types.DecimalType} See
* https://github.com/apache/iceberg/issues/2485 and
* <li>Data types: {@link Types.ListType}, {@link Types.MapType}, {@link Types.StructType}, and
* {@link Types.DecimalType} See https://github.com/apache/iceberg/issues/2485 and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decimal type support also exists already, so we can remove that comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right, I remove this comment in my other PR: #13562

rec.setField("uuid_nullable", uuid);
rec.setField("decimal", new BigDecimal("14.0" + i % 10));
rec.setField("decimal_nullable", new BigDecimal("14.0" + i % 10));
rec.setField("fixed", new byte[] {1, 2, 3, 4, 5, 6, (byte) i});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd maybe just define a string and convert it to its binary representation (same as we do a few lines above for bytes:

rec.setField("fixed", ("abc" + i).getBytes(StandardCharsets.UTF_8));
rec.setField("fixed_nullable", ("abcdef" + i).getBytes(StandardCharsets.UTF_8));

rec.setField("uuid_nullable", uuid);
rec.setField("decimal", new BigDecimal("14.20"));
rec.setField("decimal_nullable", new BigDecimal("14.20"));
rec.setField("fixed", new byte[] {1, 2, 3, 4, 5, 6, 7});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rec.setField("fixed", new byte[] {1, 2, 3, 4, 5, 6, 7});
rec.setField("fixed", "abcd".getBytes(StandardCharsets.UTF_8));

Types.NestedField.required(26, "decimal", Types.DecimalType.of(9, 2)),
Types.NestedField.optional(27, "decimal_nullable", Types.DecimalType.of(9, 2)));
Types.NestedField.optional(27, "decimal_nullable", Types.DecimalType.of(9, 2)),
Types.NestedField.required(28, "fixed", Types.FixedType.ofLength(7)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe also use different lengths for both fields

Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few comments

@nastra nastra merged commit 19b9eae into apache:main Aug 4, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants