mbutrovich commented on code in PR #2647:
URL: https://github.com/apache/iceberg-rust/pull/2647#discussion_r3625514547
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -619,7 +621,113 @@ impl RecordBatchTransformer {
})
.collect()
}
+}
+
+/// Look up an Iceberg field id from an Arrow field's `PARQUET:field_id`
metadata.
+fn arrow_field_id(field: &Field) -> Option<i32> {
+ field
+ .metadata()
+ .get(PARQUET_FIELD_ID_META_KEY)
+ .and_then(|s| s.parse::<i32>().ok())
+}
+
+/// Mirrors iceberg-java logic, resolve by field ID instead of position.
+fn cast_schema_to_target(array: &ArrayRef, target_type: &DataType) ->
Result<ArrayRef> {
+ match target_type {
+ DataType::Struct(target_children) => {
+ let source = array.as_struct_opt().ok_or_else(|| {
+ Error::new(
+ ErrorKind::Unexpected,
+ format!(
+ "expected a struct array to promote to
{target_type:?}, got {:?}",
+ array.data_type()
+ ),
+ )
+ })?;
+ let mut source_by_id: HashMap<i32, usize> = HashMap::new();
+ for (idx, field) in source.fields().iter().enumerate() {
+ if let Some(id) = arrow_field_id(field) {
+ source_by_id.insert(id, idx);
+ }
+ }
+
+ let len = source.len();
+ let mut new_columns: Vec<ArrayRef> =
Vec::with_capacity(target_children.len());
+ for target_child in target_children.iter() {
+ let matched = arrow_field_id(target_child)
+ .and_then(|id| source_by_id.get(&id))
+ .copied();
+ match matched {
+ Some(src_idx) => new_columns.push(cast_schema_to_target(
+ source.column(src_idx),
+ target_child.data_type(),
+ )?),
+ None =>
new_columns.push(new_null_array(target_child.data_type(), len)),
Review Comment:
At the top level, an absent field flows through `ColumnSource::Add`, then
`create_column`, then `create_primitive_array_repeated`, which applies the
spec's Column Projection rules: `initial-default` is honored, and a required
field with no default errors (see the existing
`schema_evolution_required_field_absent_without_default_errors` test at
`:915`). This `None` arm instead calls `new_null_array` unconditionally, so a
nested child that is required-without-default silently becomes null, and one
with an `initial-default` is nulled instead of defaulted. That diverges from
the identical case one level up.
I suspect nested `initial-default` and required-enforcement are not wired
anywhere yet (the `Add` path only carries a top-level `PrimitiveLiteral`), so
fully unifying the two paths may be a larger change than this PR wants. But
since it is the same spec policy applied inconsistently by depth, would it be
worth either routing nested absent-children through the same machinery, or, if
that is out of scope, a comment making the limitation explicit so the
divergence is intentional rather than silent?
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -619,7 +621,113 @@ impl RecordBatchTransformer {
})
.collect()
}
+}
+
+/// Look up an Iceberg field id from an Arrow field's `PARQUET:field_id`
metadata.
+fn arrow_field_id(field: &Field) -> Option<i32> {
+ field
+ .metadata()
+ .get(PARQUET_FIELD_ID_META_KEY)
+ .and_then(|s| s.parse::<i32>().ok())
+}
+
+/// Mirrors iceberg-java logic, resolve by field ID instead of position.
+fn cast_schema_to_target(array: &ArrayRef, target_type: &DataType) ->
Result<ArrayRef> {
+ match target_type {
+ DataType::Struct(target_children) => {
+ let source = array.as_struct_opt().ok_or_else(|| {
+ Error::new(
+ ErrorKind::Unexpected,
+ format!(
+ "expected a struct array to promote to
{target_type:?}, got {:?}",
+ array.data_type()
+ ),
+ )
+ })?;
+ let mut source_by_id: HashMap<i32, usize> = HashMap::new();
+ for (idx, field) in source.fields().iter().enumerate() {
+ if let Some(id) = arrow_field_id(field) {
+ source_by_id.insert(id, idx);
+ }
+ }
+
+ let len = source.len();
+ let mut new_columns: Vec<ArrayRef> =
Vec::with_capacity(target_children.len());
+ for target_child in target_children.iter() {
+ let matched = arrow_field_id(target_child)
+ .and_then(|id| source_by_id.get(&id))
+ .copied();
+ match matched {
+ Some(src_idx) => new_columns.push(cast_schema_to_target(
+ source.column(src_idx),
+ target_child.data_type(),
+ )?),
+ None =>
new_columns.push(new_null_array(target_child.data_type(), len)),
+ }
+ }
+ Ok(Arc::new(StructArray::new(
+ target_children.clone(),
+ new_columns,
+ source.nulls().cloned(),
+ )))
+ }
+ DataType::List(target_element) => {
+ let source = array
+ .as_list_opt::<i32>()
+ .ok_or_else(|| list_err(array, target_type))?;
+ let values = cast_schema_to_target(source.values(),
target_element.data_type())?;
+ Ok(Arc::new(ListArray::new(
+ target_element.clone(),
+ source.offsets().clone(),
+ values,
+ source.nulls().cloned(),
+ )))
+ }
+ DataType::LargeList(target_element) => {
+ let source = array
+ .as_list_opt::<i64>()
+ .ok_or_else(|| list_err(array, target_type))?;
+ let values = cast_schema_to_target(source.values(),
target_element.data_type())?;
+ Ok(Arc::new(LargeListArray::new(
+ target_element.clone(),
+ source.offsets().clone(),
+ values,
+ source.nulls().cloned(),
+ )))
+ }
+ DataType::Map(target_entries, sorted) => {
+ let source = array
+ .as_map_opt()
+ .ok_or_else(|| list_err(array, target_type))?;
+ let entries = cast_schema_to_target(
+ &(Arc::new(source.entries().clone()) as ArrayRef),
Review Comment:
`&(Arc::new(source.entries().clone()) as ArrayRef)` (`:704`) clones the
entries `StructArray`, re-wraps it just to feed the generic entry point, then
`entries.as_struct().clone()` (`:707`) clones it back. These clones are shallow
(`StructArray` clone bumps the Arc-backed nulls and child arrays; the only
allocation is a small `Vec<ArrayRef>` sized to the field count, no array data
is copied), so this is about readability, not cost. The round-trip through
`ArrayRef` and back to `&StructArray` reads as forced. If the struct arm's body
is factored into `fn cast_struct(source: &StructArray, target_children:
&Fields) -> Result<StructArray>` (the struct arm wraps its result in `Arc`),
the map arm can call `cast_struct(source.entries(), entry_fields)?` directly
with no re-box. This also combines with the `:647` change.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -619,7 +621,113 @@ impl RecordBatchTransformer {
})
.collect()
}
+}
+
+/// Look up an Iceberg field id from an Arrow field's `PARQUET:field_id`
metadata.
+fn arrow_field_id(field: &Field) -> Option<i32> {
+ field
+ .metadata()
+ .get(PARQUET_FIELD_ID_META_KEY)
+ .and_then(|s| s.parse::<i32>().ok())
+}
+
+/// Mirrors iceberg-java logic, resolve by field ID instead of position.
+fn cast_schema_to_target(array: &ArrayRef, target_type: &DataType) ->
Result<ArrayRef> {
+ match target_type {
+ DataType::Struct(target_children) => {
+ let source = array.as_struct_opt().ok_or_else(|| {
+ Error::new(
+ ErrorKind::Unexpected,
+ format!(
+ "expected a struct array to promote to
{target_type:?}, got {:?}",
+ array.data_type()
+ ),
+ )
+ })?;
+ let mut source_by_id: HashMap<i32, usize> = HashMap::new();
+ for (idx, field) in source.fields().iter().enumerate() {
+ if let Some(id) = arrow_field_id(field) {
+ source_by_id.insert(id, idx);
+ }
+ }
+
+ let len = source.len();
+ let mut new_columns: Vec<ArrayRef> =
Vec::with_capacity(target_children.len());
+ for target_child in target_children.iter() {
+ let matched = arrow_field_id(target_child)
+ .and_then(|id| source_by_id.get(&id))
+ .copied();
+ match matched {
+ Some(src_idx) => new_columns.push(cast_schema_to_target(
+ source.column(src_idx),
+ target_child.data_type(),
+ )?),
+ None =>
new_columns.push(new_null_array(target_child.data_type(), len)),
+ }
+ }
+ Ok(Arc::new(StructArray::new(
Review Comment:
`StructArray::new` (and `ListArray::new` `:680`, `LargeListArray::new`
`:692`, `MapArray::new` `:708`) panic on a child-length or field-count
mismatch. In practice the invariants hold locally (one child per target field,
and recursion / `new_null_array` preserve `source.len()`), so these will not
fire on data, only on a future bug in this function.
The codebase is split. `value.rs:800`/`:972` use the panicking `new` for
analogous nested-null assembly, but `create_column` in this same file (`:705`)
uses `RunArray::try_new(...).map_err(|e| Error::new(..).with_source(e))?`.
Since `cast_schema_to_target` already returns `Result` and sits next to
`create_column`, I would lean toward `try_new` plus `map_err`. It is internally
consistent, and it turns a future invariant break into a propagated
`iceberg::Error` rather than a panic. This also matches the convention
elsewhere in the crate of converting failures into `iceberg::Error` rather than
panicking. The `value.rs` `new` calls look like the exception. Reasonable to
switch?
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -610,7 +612,7 @@ impl RecordBatchTransformer {
ColumnSource::Promote {
target_type,
source_index,
- } => cast(&*columns[*source_index], target_type)?,
+ } => cast_schema_to_target(&columns[*source_index],
target_type)?,
Review Comment:
This `Promote` arm is now the entry point, but nothing drives it through
`process_record_batch`; the five new tests call `cast_schema_to_target`
directly. The tests cover one Column Projection rule well (absent id becomes
null, recursively through struct/list/map/large-list). The other corners the
spec enumerates for struct evolution are not covered:
| Spec case | Source | Covered? | In scope? |
|---|---|---|---|
| Add nested field becomes `null` | Column Projection rule 4 | Yes | n/a |
| Rename (same id, new name) | Column Projection example; Schema Evolution |
No | Yes (core mechanism) |
| Primitive promotion in a nested child (int to long, float to double,
decimal widen, v3 date to timestamp) | Schema Evolution promotion table | No
(`_ => cast`) | Yes (core mechanism) |
| End-to-end via `process_record_batch` | n/a | No | Yes (wiring) |
| Null parent row (`source.nulls()` survives) | n/a | No | Yes (this PR's
line) |
| Reorder / delete field | Schema Evolution | Partial / no | Optional, low
cost |
| name-mapping for id-less columns | Column Projection rule 2 | No | Guard
only (see `:627`) |
| `initial-default` before null | Column Projection rule 3 | No | Out of
scope (see `:665`) |
Rename-by-id and nested primitive promotion are the highest-value additions.
They are exactly what a by-id reconciler exists to get right, and both run
entirely through this function. Are any of these already covered on the Comet
side, or worth adding here?
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -619,7 +621,113 @@ impl RecordBatchTransformer {
})
.collect()
}
+}
+
+/// Look up an Iceberg field id from an Arrow field's `PARQUET:field_id`
metadata.
+fn arrow_field_id(field: &Field) -> Option<i32> {
+ field
+ .metadata()
+ .get(PARQUET_FIELD_ID_META_KEY)
+ .and_then(|s| s.parse::<i32>().ok())
+}
+
+/// Mirrors iceberg-java logic, resolve by field ID instead of position.
+fn cast_schema_to_target(array: &ArrayRef, target_type: &DataType) ->
Result<ArrayRef> {
+ match target_type {
+ DataType::Struct(target_children) => {
+ let source = array.as_struct_opt().ok_or_else(|| {
+ Error::new(
+ ErrorKind::Unexpected,
+ format!(
+ "expected a struct array to promote to
{target_type:?}, got {:?}",
+ array.data_type()
+ ),
+ )
+ })?;
+ let mut source_by_id: HashMap<i32, usize> = HashMap::new();
+ for (idx, field) in source.fields().iter().enumerate() {
+ if let Some(id) = arrow_field_id(field) {
+ source_by_id.insert(id, idx);
+ }
+ }
+
+ let len = source.len();
+ let mut new_columns: Vec<ArrayRef> =
Vec::with_capacity(target_children.len());
+ for target_child in target_children.iter() {
+ let matched = arrow_field_id(target_child)
+ .and_then(|id| source_by_id.get(&id))
+ .copied();
+ match matched {
+ Some(src_idx) => new_columns.push(cast_schema_to_target(
+ source.column(src_idx),
+ target_child.data_type(),
+ )?),
+ None =>
new_columns.push(new_null_array(target_child.data_type(), len)),
+ }
+ }
+ Ok(Arc::new(StructArray::new(
+ target_children.clone(),
+ new_columns,
+ source.nulls().cloned(),
+ )))
+ }
+ DataType::List(target_element) => {
+ let source = array
+ .as_list_opt::<i32>()
+ .ok_or_else(|| list_err(array, target_type))?;
+ let values = cast_schema_to_target(source.values(),
target_element.data_type())?;
+ Ok(Arc::new(ListArray::new(
+ target_element.clone(),
+ source.offsets().clone(),
+ values,
+ source.nulls().cloned(),
+ )))
+ }
+ DataType::LargeList(target_element) => {
+ let source = array
+ .as_list_opt::<i64>()
+ .ok_or_else(|| list_err(array, target_type))?;
+ let values = cast_schema_to_target(source.values(),
target_element.data_type())?;
+ Ok(Arc::new(LargeListArray::new(
+ target_element.clone(),
+ source.offsets().clone(),
+ values,
+ source.nulls().cloned(),
+ )))
+ }
+ DataType::Map(target_entries, sorted) => {
+ let source = array
+ .as_map_opt()
+ .ok_or_else(|| list_err(array, target_type))?;
+ let entries = cast_schema_to_target(
+ &(Arc::new(source.entries().clone()) as ArrayRef),
+ target_entries.data_type(),
+ )?;
+ let entries_struct = entries.as_struct().clone();
+ Ok(Arc::new(MapArray::new(
+ target_entries.clone(),
+ source.offsets().clone(),
+ entries_struct,
+ source.nulls().cloned(),
+ *sorted,
+ )))
+ }
+ _ => Ok(cast(array.as_ref(), target_type)?),
+ }
+}
+
+fn list_err(array: &ArrayRef, target_type: &DataType) -> Error {
+ Error::new(
+ ErrorKind::Unexpected,
+ format!(
+ "expected a list/map array to promote to {target_type:?}, got
{:?}",
+ array.data_type()
+ ),
+ )
+}
+
+impl RecordBatchTransformer {
Review Comment:
`arrow_field_id`, `cast_schema_to_target`, and `list_err` are inserted
between two `impl RecordBatchTransformer` blocks (the impl closes before `:627`
and reopens here at `:730` for `create_column`). The file's existing free fn
(`constants_map`) sits above the impls. Grouping these three there (or below
the impl) avoids splitting it in half.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -663,18 +771,209 @@ mod test {
use std::collections::HashMap;
use std::sync::Arc;
+ use arrow_array::cast::AsArray;
+ use arrow_array::types::Int32Type;
use arrow_array::{
- Array, Date32Array, Float32Array, Float64Array, Int32Array,
Int64Array, RecordBatch,
- StringArray,
+ Array, ArrayRef, Date32Array, Float32Array, Float64Array, Int32Array,
Int64Array,
+ LargeListArray, ListArray, MapArray, RecordBatch, StringArray,
StructArray,
};
- use arrow_schema::{DataType, Field, Schema as ArrowSchema};
+ use arrow_buffer::OffsetBuffer;
+ use arrow_schema::{DataType, Field, Fields, Schema as ArrowSchema};
use parquet::arrow::PARQUET_FIELD_ID_META_KEY;
use crate::arrow::record_batch_transformer::{
- RecordBatchTransformer, RecordBatchTransformerBuilder,
+ RecordBatchTransformer, RecordBatchTransformerBuilder,
cast_schema_to_target,
};
use crate::spec::{Literal, NestedField, PrimitiveType, Schema, Struct,
Type};
+ fn with_id(field: Field, id: i32) -> Field {
Review Comment:
`with_id` (`:789`) and `field_with_id` (`:796`) duplicate the existing
`simple_field(name, ty, nullable, value)` (`:1137`), which already attaches the
field-id metadata (with a `&str` id vs. the new `i32`). Reusing `simple_field`
(or replacing it) keeps one helper for one purpose and matches the surrounding
test style.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -619,7 +621,113 @@ impl RecordBatchTransformer {
})
.collect()
}
+}
+
+/// Look up an Iceberg field id from an Arrow field's `PARQUET:field_id`
metadata.
+fn arrow_field_id(field: &Field) -> Option<i32> {
Review Comment:
The spec's resolution order for a field id absent from a file is partition
metadata, then name-mapping, then `initial-default`, then null. Position is not
in that list, so dropping the positional cast is correct. But the by-id match
here has no fallback when a source child carries no `PARQUET:field_id`:
`arrow_field_id` returns `None`, `source_by_id` ends up empty, and the struct
arm nulls **every** target child (see `:665`), silently dropping data that is
in the file.
That path is reachable. `apply_name_mapping_to_arrow_schema`
(`crates/iceberg/src/arrow/reader/projection.rs:395`) and
`add_fallback_field_ids_to_arrow_schema` (`:451`) only assign ids to
**top-level** fields and never recurse (comment at `:449`: "Why only
top-level"), even though the spec requires name mapping to descend into
`element`/`key`/`value`/struct children. And the failure mode regresses:
today's positional `cast` on that path errors (arrow's struct-to-struct cast
fails on the mismatch, which is the #2617 symptom), whereas this change turns
it into a silent wrong answer.
The full fix (recursive name-mapping) lives in `projection.rs` and is
clearly its own PR. But since we would be trading a visible error for silent
data loss, would a **guard here** make sense? When a source struct has children
but none carry field ids, return an `Error` (or a documented fallback) rather
than nulling everything, plus a test pinning it. Did you already consider this?
Related reuse point: `arrow_field_id` is the fifth copy of "parse
`PARQUET:field_id` from a `Field`". See `schema.rs:328`
(`get_field_id_from_metadata`, `pub(super)` so already reachable here),
`record_batch_projector.rs:97`, and the inline parse at
`record_batch_transformer.rs:644`. It is also the only one that swallows a
parse error as `None` (returns `Option<i32>`), which is part of how malformed
metadata would silently null a column rather than erroring. Could we reuse a
shared helper? The projector's `Result<Option<i32>>` shape fits best: `None`
means absent so null-fill, `Err` means malformed so report it.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -619,7 +621,113 @@ impl RecordBatchTransformer {
})
.collect()
}
+}
+
+/// Look up an Iceberg field id from an Arrow field's `PARQUET:field_id`
metadata.
+fn arrow_field_id(field: &Field) -> Option<i32> {
+ field
+ .metadata()
+ .get(PARQUET_FIELD_ID_META_KEY)
+ .and_then(|s| s.parse::<i32>().ok())
+}
+
+/// Mirrors iceberg-java logic, resolve by field ID instead of position.
+fn cast_schema_to_target(array: &ArrayRef, target_type: &DataType) ->
Result<ArrayRef> {
+ match target_type {
+ DataType::Struct(target_children) => {
+ let source = array.as_struct_opt().ok_or_else(|| {
+ Error::new(
+ ErrorKind::Unexpected,
+ format!(
+ "expected a struct array to promote to
{target_type:?}, got {:?}",
+ array.data_type()
+ ),
+ )
+ })?;
+ let mut source_by_id: HashMap<i32, usize> = HashMap::new();
Review Comment:
This is the one I would most want to resolve. `cast_schema_to_target` runs
on the `Promote` arm inside `transform_columns`, i.e. once per `RecordBatch`.
For every struct node it allocates a fresh `HashMap<i32, usize>` (`:647`) and,
via `arrow_field_id`, re-parses `PARQUET:field_id` strings for every source
child (`:650`) and every target child (`:658`). None of that varies across
batches. The source and target schemas are fixed by the time
`BatchTransform::Modify` is cached, so this is pure repeated allocation plus
integer parsing on the per-batch path.
The file already has the pattern to avoid it: top-level field-id resolution
happens once in `generate_transform_operations` /
`build_field_id_to_arrow_schema_map` and is cached as index-based
`ColumnSource`s, and `transform_columns` then just gathers by precomputed
index. Would it be worth resolving the nested mapping the same way? Precompute
a small nested plan (per target child, the matched source index or "absent")
once when the transform is built, and store it on `ColumnSource::Promote`, so
the per-batch pass is just index lookups and array assembly with no hashing or
parsing. It is a bit more up-front structure, but it moves all the repeated
work off the per-batch path and keeps this consistent with how the rest of the
transformer caches its plan. Two lower-priority points of the same kind: the
`target_children.clone()` and offset clones at each level are static too and
would live in that cached plan, and absent children could use the REE/constant
representation the top-level
`Add` path uses via `create_column` rather than materializing a full-length
null buffer every batch.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -619,7 +621,113 @@ impl RecordBatchTransformer {
})
.collect()
}
+}
+
+/// Look up an Iceberg field id from an Arrow field's `PARQUET:field_id`
metadata.
+fn arrow_field_id(field: &Field) -> Option<i32> {
+ field
+ .metadata()
+ .get(PARQUET_FIELD_ID_META_KEY)
+ .and_then(|s| s.parse::<i32>().ok())
+}
+
+/// Mirrors iceberg-java logic, resolve by field ID instead of position.
+fn cast_schema_to_target(array: &ArrayRef, target_type: &DataType) ->
Result<ArrayRef> {
Review Comment:
Two small things on `cast_schema_to_target`:
- The PR description calls this `promote_array_to_target`. `promote_*` is
clearer than `cast_*` since it serves `ColumnSource::Promote` and does more
than a cast. Align the name?
- The doc at `:634` (`/// Mirrors iceberg-java logic, resolve by field ID
instead of position.`) is a comma splice, whereas surrounding docs are full
sentences. Maybe: `/// Reconciles a source array to the target type by matching
nested fields on field id rather than position, mirroring iceberg-java's nested
readers.`
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -619,7 +621,113 @@ impl RecordBatchTransformer {
})
.collect()
}
+}
+
+/// Look up an Iceberg field id from an Arrow field's `PARQUET:field_id`
metadata.
+fn arrow_field_id(field: &Field) -> Option<i32> {
+ field
+ .metadata()
+ .get(PARQUET_FIELD_ID_META_KEY)
+ .and_then(|s| s.parse::<i32>().ok())
+}
+
+/// Mirrors iceberg-java logic, resolve by field ID instead of position.
+fn cast_schema_to_target(array: &ArrayRef, target_type: &DataType) ->
Result<ArrayRef> {
+ match target_type {
+ DataType::Struct(target_children) => {
+ let source = array.as_struct_opt().ok_or_else(|| {
+ Error::new(
+ ErrorKind::Unexpected,
+ format!(
+ "expected a struct array to promote to
{target_type:?}, got {:?}",
+ array.data_type()
+ ),
+ )
+ })?;
+ let mut source_by_id: HashMap<i32, usize> = HashMap::new();
+ for (idx, field) in source.fields().iter().enumerate() {
+ if let Some(id) = arrow_field_id(field) {
+ source_by_id.insert(id, idx);
+ }
+ }
+
+ let len = source.len();
+ let mut new_columns: Vec<ArrayRef> =
Vec::with_capacity(target_children.len());
+ for target_child in target_children.iter() {
+ let matched = arrow_field_id(target_child)
+ .and_then(|id| source_by_id.get(&id))
+ .copied();
+ match matched {
+ Some(src_idx) => new_columns.push(cast_schema_to_target(
+ source.column(src_idx),
+ target_child.data_type(),
+ )?),
+ None =>
new_columns.push(new_null_array(target_child.data_type(), len)),
+ }
+ }
+ Ok(Arc::new(StructArray::new(
+ target_children.clone(),
+ new_columns,
+ source.nulls().cloned(),
+ )))
+ }
+ DataType::List(target_element) => {
+ let source = array
+ .as_list_opt::<i32>()
Review Comment:
The two arms (`:677` and `:689`) are identical except for
`as_list_opt::<i32>` vs `::<i64>` and `ListArray` vs `LargeListArray`. Since
those are `GenericListArray<i32>` and `<i64>`, a small helper generic over
`OffsetSizeTrait` collapses them into one body both arms call, so the
recursion/offset/nulls logic only exists once:
```rust
fn cast_list<O: OffsetSizeTrait>(source: &GenericListArray<O>,
target_element: &FieldRef) -> Result<ArrayRef> {
let values = cast_schema_to_target(source.values(),
target_element.data_type())?;
Ok(Arc::new(GenericListArray::<O>::new(
target_element.clone(), source.offsets().clone(), values,
source.nulls().cloned())))
}
```
--
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]
