comphead commented on code in PR #6238:
URL: https://github.com/apache/datafusion-comet/pull/6238#discussion_r4146845917
##########
native/core/src/execution/operators/iceberg_write.rs:
##########
@@ -1133,6 +1155,44 @@ fn contains_float(data_type: &DataType) -> bool {
}
}
+/// `true` when `array` holds a float or double under a list or map whose
child array reaches
+/// past the rows `array` covers. Slicing a list or map narrows only its
offsets and leaves the
+/// child whole, and the NaN-count visitor walks the whole child. A struct's
children are sliced
+/// with it, so a struct only matters for the lists and maps inside it. A
container this does not
+/// inspect is treated as reaching past whenever it holds a float, which errs
toward gathering.
+fn floats_outside_window(array: &dyn Array) -> bool {
+ match array.data_type() {
+ DataType::List(field) => {
+ let list = array.as_list::<i32>();
+ contains_float(field.data_type())
+ && reaches_past(list.value_offsets(), list.values().as_ref())
+ }
+ DataType::LargeList(field) => {
Review Comment:
Nit: I don't think this `LargeList` arm is reachable.
`decorate_batch_with_field_ids` casts every column to the schema from
`iceberg::arrow::schema_to_arrow_schema`, which only produces `List` and `Map`,
so `RowSlicer` should never see a `LargeList`. Would it make sense to drop the
arm and let it fall through to the conservative `other` arm? Then
`reaches_past` could take `&[i32]` and lose the generic and the
`OffsetSizeTrait` import.
##########
native/core/src/execution/operators/iceberg_write.rs:
##########
@@ -972,6 +974,11 @@ impl ClusteredBatchSplitter {
/// safe, because `StructArray::slice` slices them, so the only schemas that
need the fix are the
/// ones with a float or double under a list or map; those ranges go through
`take`, which gathers
/// the referenced children into fresh compacted arrays.
+///
+/// A range that covers its whole batch is handed on as-is, which is exact
only if the batch itself
Review Comment:
Question: the root cause seems to be that iceberg-rust's
`NanValueCountVisitor` reads list elements and map keys and values from the
whole child arrays, so it ignores the parent's offset window. Is there an
upstream iceberg-rust issue for that? I couldn't find one, but I may have
missed it. If there isn't, it might be worth filing one and linking it here, so
`compact` and the gathers can be dropped once the pinned revision picks up a
fix.
--
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]
