advancedxy commented on code in PR #2668:
URL: https://github.com/apache/iceberg-rust/pull/2668#discussion_r3457567533
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -140,6 +154,13 @@ pub(crate) enum ColumnSource {
target_type: DataType,
value: Option<PrimitiveLiteral>,
},
+
+ // A struct column where each child is a constant primitive value.
+ // Used for the _partition metadata column.
+ AddStructConstant {
Review Comment:
> In iceberg-rust, the constant_fields: HashMap<i32, Datum> infrastructure
only supports primitive Datum values — Datum has no struct variant. Adding
struct support to Datum would be a significant separate effort (touching the
spec, serde, and value layers).
I see the trade off now. I think it's better to choose your approach now to
avoid touching too much internals.
However, I think iceberg-rust already has Struct literal in
crates/iceberg/src/spec/values/literal.rs, we can use that to replace the
child_values?
And BTW, taking a step forward, the AddStructConstant is essentially the
same as
```rust
Add {
target_type: DataType,
value: Option<PrimitiveLiteral>,
},
```
which should also be refactor to use `Option<Literal>` in value.
Maybe, we can create a follow-up issue now to refactor these operations. And
we can go with your approach for now.
##########
crates/iceberg/src/arrow/value.rs:
##########
@@ -909,6 +909,38 @@ pub(crate) fn create_primitive_array_repeated(
let vals: Vec<Option<&[u8]>> = vec![None; num_rows];
Arc::new(BinaryArray::from_opt_vec(vals))
}
+ (DataType::LargeBinary, Some(PrimitiveLiteral::Binary(value))) => {
+ Arc::new(LargeBinaryArray::from_vec(vec![value; num_rows]))
+ }
+ (DataType::LargeBinary, None) => {
+ let vals: Vec<Option<&[u8]>> = vec![None; num_rows];
+ Arc::new(LargeBinaryArray::from_opt_vec(vals))
+ }
+ (DataType::FixedSizeBinary(len),
Some(PrimitiveLiteral::Binary(value))) => {
+ let repeated: Vec<&[u8]> = vec![value.as_slice(); num_rows];
+
Arc::new(FixedSizeBinaryArray::try_from_iter(repeated.into_iter()).map_err(|e| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ format!("Failed to create FixedSizeBinary({len}) array:
{e}"),
+ )
+ })?)
+ }
+ (DataType::FixedSizeBinary(len), None) => {
+ let repeated: Vec<Option<&[u8]>> = vec![None; num_rows];
+
Arc::new(FixedSizeBinaryArray::try_from_sparse_iter_with_size(repeated.into_iter(),
*len).map_err(|e| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ format!("Failed to create null FixedSizeBinary({len})
array: {e}"),
+ )
+ })?)
+ }
Review Comment:
> This is needed, I think. When a table is partitioned by a Binary, UUID, or
Time column, create_primitive_array_repeated must handle those Arrow types to
produce the child arrays of the _partition struct.
thanks for the explanation.
> the default type_to_arrow_type mapping for Iceberg's Binary type should
produce LargeBinary (matching Java)
Actually, java maps Iceberg's Binary to Arrow's Binary at least in the java
arrow reader, see
https://github.com/apache/iceberg/blob/main/arrow/src/main/java/org/apache/iceberg/arrow/ArrowSchemaUtil.java#L134
and I think parquet-rs reads the binary column in parquet as binary rather
than large binary as well.
> However, that change affects all Binary column reads (not just
_partition), so it should probably be a follow up.
Yes, it should be addressed in a follow-up.
> The LargeBinary arm in create_primitive_array_repeated ensures we handle
it correctly if/when that mapping change lands.
That totally makes sense.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
