laskoviymishka commented on code in PR #3282:
URL: https://github.com/apache/iceberg-rust/pull/3282#discussion_r4153121219
##########
crates/iceberg/src/arrow/record_batch_projector.rs:
##########
@@ -271,6 +309,77 @@ mod test {
assert_eq!(projected_inner_int_array.values(), &[4, 5, 6]);
}
+ #[test]
+ fn test_record_batch_projector_top_level_nullable_column() {
+ let iceberg_schema = IcebergSchema::builder()
+ .with_schema_id(0)
+ .with_fields(vec![
+ NestedField::optional(1, "id",
Type::Primitive(PrimitiveType::Int)).into(),
+ ])
+ .build()
+ .unwrap();
+ let projector =
+
RecordBatchProjector::from_iceberg_schema(Arc::new(iceberg_schema),
&[1]).unwrap();
+ let input = Arc::new(Int32Array::from(vec![Some(10), None, Some(30)]))
as ArrayRef;
+
+ let projected = projector.project_column(&[input]).unwrap();
+ let projected_array =
projected[0].as_any().downcast_ref::<Int32Array>().unwrap();
+
+ assert_eq!(projected_array.value(0), 10);
+ assert_eq!(projected_array.null_count(), 1);
+ assert!(projected_array.is_null(1));
+ assert_eq!(projected_array.value(2), 30);
+ }
+
+ #[test]
+ fn test_record_batch_projector_nested_nullable_leaf_without_parent_nulls()
{
+ let (projector, schema, inner_field, leaf_field) = nested_projector();
+ let leaf = Arc::new(Int32Array::from(vec![Some(10), None, Some(30)]))
as ArrayRef;
+ let inner = Arc::new(StructArray::new(
+ Fields::from(vec![leaf_field]),
+ vec![leaf.clone()],
+ None,
+ )) as ArrayRef;
+ let outer = Arc::new(StructArray::new(
+ Fields::from(vec![inner_field]),
+ vec![inner],
+ Some(NullBuffer::new_valid(3)),
Review Comment:
This test doesn't actually exercise the early-return it's meant to guard.
`new_valid(3)` is all-valid, and `StructArray::new` filters all-valid buffers
away at construction — so `parent_null_buffer` stays `None` because Arrow
discarded the buffer, not because the fast path recognized an all-valid parent.
The `ptr_eq` below passes for the wrong reason, and it'd flip to a false red
the day Arrow relaxes that normalization.
I'd swap this to `None` and add a case where both ancestors are `None` and
the leaf carries its own nulls — that drives the early-return directly without
leaning on Arrow's internal filtering.
##########
crates/iceberg/src/arrow/record_batch_projector.rs:
##########
@@ -176,21 +176,32 @@ impl RecordBatchProjector {
}
fn get_column_by_field_index(batch: &[ArrayRef], field_index: &[usize]) ->
Result<ArrayRef> {
+ if let [index] = field_index {
+ return Ok(batch[*index].clone());
Review Comment:
I'd drop this arm entirely — for a one-element `field_index` the general
path already returns the identical single-column clone (the `rev_iterator`
yields the only element as the initial `array`, the loop never runs, and the
`None` early-return hands it straight back). So it's dead complexity, and it
adds a fresh unchecked `batch[*index]` panic for no perf gain, since that
`None` path is already O(1). The pre-existing `next().unwrap()` just below
leans on the same constructor invariant, which is fine to leave as-is.
##########
crates/iceberg/src/arrow/record_batch_projector.rs:
##########
@@ -201,13 +212,40 @@ impl RecordBatchProjector {
mod test {
use std::sync::Arc;
- use arrow_array::{ArrayRef, Int32Array, RecordBatch, StringArray,
StructArray};
+ use arrow_array::{Array, ArrayRef, Int32Array, RecordBatch, StringArray,
StructArray};
+ use arrow_buffer::NullBuffer;
use arrow_schema::{DataType, Field, Fields, Schema};
use crate::arrow::record_batch_projector::RecordBatchProjector;
use crate::spec::{NestedField, PrimitiveType, Schema as IcebergSchema,
Type};
use crate::{Error, ErrorKind};
+ fn nested_projector() -> (RecordBatchProjector, Arc<Schema>, Field, Field)
{
+ let leaf_field = Field::new("leaf", DataType::Int32, true);
+ let inner_field = Field::new(
+ "inner",
+ DataType::Struct(Fields::from(vec![leaf_field.clone()])),
+ true,
+ );
+ let outer_field = Field::new(
+ "outer",
+ DataType::Struct(Fields::from(vec![inner_field.clone()])),
+ true,
+ );
+ let schema = Arc::new(Schema::new(vec![outer_field.clone()]));
Review Comment:
`outer_field` isn't used after this, so the clone just allocates a discarded
`Field` — `Schema::new(vec![outer_field])` works.
##########
crates/iceberg/src/arrow/record_batch_projector.rs:
##########
@@ -176,21 +176,32 @@ impl RecordBatchProjector {
}
fn get_column_by_field_index(batch: &[ArrayRef], field_index: &[usize]) ->
Result<ArrayRef> {
+ if let [index] = field_index {
+ return Ok(batch[*index].clone());
+ }
+
let mut rev_iterator = field_index.iter().rev();
let mut array = batch[*rev_iterator.next().unwrap()].clone();
- let mut null_buffer = array.logical_nulls();
+ let mut parent_null_buffer = None;
for idx in rev_iterator {
- array = array
+ let struct_array = array
.as_any()
.downcast_ref::<StructArray>()
.ok_or(Error::new(
ErrorKind::Unexpected,
"Cannot convert Array to StructArray",
- ))?
- .column(*idx)
- .clone();
- null_buffer = NullBuffer::union(null_buffer.as_ref(),
array.logical_nulls().as_ref());
+ ))?;
+ parent_null_buffer = NullBuffer::union(
+ parent_null_buffer.as_ref(),
+ struct_array.logical_nulls().as_ref(),
+ );
+ array = struct_array.column(*idx).clone();
}
+ let Some(parent_null_buffer) = parent_null_buffer else {
+ return Ok(array);
Review Comment:
Returning `array` as-is here skips the `logical_nulls()` → physical-buffer
materialization the old path always did. For the primitive leaves Iceberg
projects that's a no-op (`logical_nulls() == nulls()`), so it's safe — but
`project_column` is `pub`, and for array types where logical nulls exceed the
physical buffer (a dictionary with a nullable value array, say) the fast path
now returns fewer physical nulls than the old rebuild would have. A one-line
comment noting the fast path assumes leaf `logical_nulls() == nulls()` would
make the narrowed contract intentional rather than a latent surprise.
--
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]
