pingz-oai commented on code in PR #24684:
URL: https://github.com/apache/datafusion/pull/24684#discussion_r3877161511
##########
datafusion/spark/src/function/map/utils.rs:
##########
@@ -225,23 +243,38 @@ fn map_deduplicate_keys(
);
}
keys_mask_builder.append_value(true);
+ values_mask_builder.append_value(true);
key_to_output_idx.insert(key, value_indices.len());
value_indices.push(abs_value_idx);
new_last_offset += 1;
}
} else {
// The result entry is NULL — no keys/values emitted. Still pad the
- // mask so it stays aligned with `flat_keys`.
+ // masks so they stay aligned with their respective flat arrays.
keys_mask_builder.append_n(num_keys_entries, false);
+ values_mask_builder.append_n(num_values_entries, false);
}
new_offsets.push(new_last_offset);
cur_keys_offset += num_keys_entries;
cur_values_offset += num_values_entries;
}
let keys_mask = keys_mask_builder.finish();
+ let values_mask = values_mask_builder.finish();
let needed_keys = filter(&flat_keys, &keys_mask)?;
- let value_indices_array = Int32Array::from(value_indices);
- let needed_values = take(&flat_values, &value_indices_array, None)?;
+ let needed_values = if needs_value_take {
+ let value_indices_array = Int32Array::from(value_indices);
+ take(&flat_values, &value_indices_array, None)?
+ } else {
+ // Values lists can be sliced independently of keys lists. Align the
+ // relative mask with their first offset, without allocating a slice
+ // wrapper for the common case where values start at zero.
+ let flat_values = if values_start_offset == 0 {
+ Cow::Borrowed(flat_values)
+ } else {
+ Cow::Owned(flat_values.slice(values_start_offset,
values_mask.len()))
Review Comment:
[P2] Preserve large child offsets in the new filter path
A valid `map_from_arrays` input can now panic when its `LargeList` values
start at child offset `2_147_483_648` and no duplicate overwrite occurs. A key
row `[7]` and a `LargeList<Null>` value row with offsets `[2_147_483_648,
2_147_483_649]` previously produce `{7: NULL}`; this slice instead fails its
bounds assertion. Both input arrays pass Arrow's `validate_full()`, and a
`NullArray` child makes this reproducible without allocating a huge buffer.
The existing `get_list_offsets` conversion narrows the offsets to negative
`i32` values. The base's `take` still works because Arrow 59.2.0 reinterprets
its `Int32` indices as `UInt32`, recovering the intended child index. This new
path instead passes the sign-extended `usize` (`18446744071562067968` on 64-bit
builds) directly to `slice`.
Minimal helper reproduction using the caller's actual offset conversion,
inside the existing utils test module:
```rust
use arrow::array::{LargeListArray, NullArray};
let start = 1_i64 << 31;
let keys: ArrayRef = Arc::new(Int32Array::from(vec![7]));
let values: ArrayRef = Arc::new(LargeListArray::new(
Arc::new(Field::new("item", DataType::Null, true)),
OffsetBuffer::new(vec![start, start + 1].into()),
Arc::new(NullArray::new(start as usize + 1)),
None,
));
values.to_data().validate_full().unwrap();
map_from_keys_values_offsets_nulls(
&keys,
get_list_values(&values).unwrap(),
&[0, 1],
&get_list_offsets(&values).unwrap(),
None,
None,
false,
).unwrap();
```
A focused comparison of the base
(`4fcaa01c721ba18c10a5ac65446aa48ddf68e9b0`) and this head's helper bodies,
using the locked Arrow dependencies, confirms base success and head panic under
both `EXCEPTION` and `LAST_WIN`. Preserve the wide offset when slicing, or
retain the previous `take` path for narrowed negative offsets.
--
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]
