alamb commented on code in PR #22172:
URL: https://github.com/apache/datafusion/pull/22172#discussion_r3248121553
##########
datafusion/physical-expr-common/src/binary_view_map.rs:
##########
@@ -339,11 +339,31 @@ where
payload
} else {
// no existing value, make a new one
- let value: &[u8] = values.value(i).as_ref();
- let payload = make_payload_fn(Some(value));
+ let (new_view, payload) = if len <= 12 {
+ // Inline path: bytes are already packed in view_u128.
+ // The inline ByteView format is [len:u32 LE][data:12
bytes zero-padded],
+ // so extracting bytes from the u128 avoids a round-trip
through
+ // values.value(i) (which reads the views buffer and
returns the same slice).
+ let view_bytes = view_u128.to_le_bytes();
+ let value = &view_bytes[4..4 + len as usize];
+ let payload = make_payload_fn(Some(value));
+ // For inline strings, the stored view is identical to the
input view:
+ // make_view(value, 0, 0) produces the same u128 as
view_u128.
+ //
+ // SAFETY: the enclosing `if len <= 12` branch establishes
+ // that view_u128 is a valid inline ByteView. Its low 32
+ // bits encode `len` (<= 12) and the next 12 bytes are the
+ // value bytes the source array produced for this row, so
+ // every required invariant of `append_inline_view` holds.
Review Comment:
this seems overly verbose -- I think the justification is that `view_u128`
was a valid view and len <= 12
```suggestion
// SAFETY: view_u128 was a valid view, and the
encosing`len <= 12`
// ensures it is inline
```
--
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]
