You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nandorKollar (via GitHub)" <gi...@apache.org> on 2025/07/29 19:53:23 UTC
[PR] Arrow: test case for fixed type [iceberg]
nandorKollar opened a new pull request, #13700:
URL: https://github.com/apache/iceberg/pull/13700
(no comment)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: test case for fixed type [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13700:
URL: https://github.com/apache/iceberg/pull/13700#discussion_r2250442901
##########
arrow/src/test/java/org/apache/iceberg/arrow/vectorized/TestArrowReader.java:
##########
@@ -865,6 +896,8 @@ private List<GenericRecord> createConstantRecordsForDate(Schema schema, LocalDat
rec.setField("uuid_nullable", uuid);
rec.setField("decimal", new BigDecimal("14.20"));
rec.setField("decimal_nullable", new BigDecimal("14.20"));
+ rec.setField("fixed", ("abcdefg").getBytes(StandardCharsets.UTF_8));
Review Comment:
```suggestion
rec.setField("fixed", "abcdefg".getBytes(StandardCharsets.UTF_8));
```
##########
arrow/src/test/java/org/apache/iceberg/arrow/vectorized/TestArrowReader.java:
##########
@@ -865,6 +896,8 @@ private List<GenericRecord> createConstantRecordsForDate(Schema schema, LocalDat
rec.setField("uuid_nullable", uuid);
rec.setField("decimal", new BigDecimal("14.20"));
rec.setField("decimal_nullable", new BigDecimal("14.20"));
+ rec.setField("fixed", ("abcdefg").getBytes(StandardCharsets.UTF_8));
+ rec.setField("fixed_nullable", ("abcdefg").getBytes(StandardCharsets.UTF_8));
Review Comment:
```suggestion
rec.setField("fixed_nullable", "abcdefg".getBytes(StandardCharsets.UTF_8));
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: test case for fixed type [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13700:
URL: https://github.com/apache/iceberg/pull/13700#discussion_r2247191137
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/ArrowReader.java:
##########
@@ -91,9 +92,8 @@
* <li>Columns with constant values are physically encoded as a dictionary. The Arrow vector type
* is int32 instead of the type as per the schema. See
* https://github.com/apache/iceberg/issues/2484.
- * <li>Data types: {@link Types.ListType}, {@link Types.MapType}, {@link Types.StructType}, {@link
- * 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
Review Comment:
decimal type support also exists already, so we can remove that comment
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: test case for fixed type [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #13700:
URL: https://github.com/apache/iceberg/pull/13700#issuecomment-3143314950
> @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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: test case for fixed type [iceberg]
Posted by "nandorKollar (via GitHub)" <gi...@apache.org>.
nandorKollar commented on code in PR #13700:
URL: https://github.com/apache/iceberg/pull/13700#discussion_r2247707658
##########
arrow/src/main/java/org/apache/iceberg/arrow/vectorized/ArrowReader.java:
##########
@@ -91,9 +92,8 @@
* <li>Columns with constant values are physically encoded as a dictionary. The Arrow vector type
* is int32 instead of the type as per the schema. See
* https://github.com/apache/iceberg/issues/2484.
- * <li>Data types: {@link Types.ListType}, {@link Types.MapType}, {@link Types.StructType}, {@link
- * 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
Review Comment:
Yes, that's right, I remove this comment in my other PR: #13562
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: test case for fixed type [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13700:
URL: https://github.com/apache/iceberg/pull/13700#discussion_r2247201361
##########
arrow/src/test/java/org/apache/iceberg/arrow/vectorized/TestArrowReader.java:
##########
@@ -828,6 +857,8 @@ private List<GenericRecord> createIncrementalRecordsForDate(
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});
Review Comment:
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));
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: test case for fixed type [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13700:
URL: https://github.com/apache/iceberg/pull/13700#discussion_r2247203023
##########
arrow/src/test/java/org/apache/iceberg/arrow/vectorized/TestArrowReader.java:
##########
@@ -865,6 +896,8 @@ private List<GenericRecord> createConstantRecordsForDate(Schema schema, LocalDat
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});
Review Comment:
```suggestion
rec.setField("fixed", "abcd".getBytes(StandardCharsets.UTF_8));
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: test case for fixed type [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #13700:
URL: https://github.com/apache/iceberg/pull/13700#discussion_r2247204723
##########
arrow/src/test/java/org/apache/iceberg/arrow/vectorized/TestArrowReader.java:
##########
@@ -698,7 +720,9 @@ private void writeTable(boolean constantRecords) throws Exception {
Types.NestedField.required(24, "uuid", Types.UUIDType.get()),
Types.NestedField.optional(25, "uuid_nullable", Types.UUIDType.get()),
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)),
Review Comment:
nit: maybe also use different lengths for both fields
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: test case for fixed type [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #13700:
URL: https://github.com/apache/iceberg/pull/13700#issuecomment-3140597682
@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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Arrow: test case for fixed type [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #13700:
URL: https://github.com/apache/iceberg/pull/13700
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org