Copilot commented on code in PR #20055:
URL: https://github.com/apache/datafusion/pull/20055#discussion_r2738903090
##########
datafusion/physical-expr-common/src/binary_view_map.rs:
##########
@@ -291,6 +291,7 @@ where
// Extract length from the view (first 4 bytes of u128 in
little-endian)
let len = view_u128 as u32;
+ let value: &[u8] = values.value(i).as_ref();
Review Comment:
The byte slice is fetched unconditionally at line 294 even for inline
strings (≤12 bytes) that will be compared using only the u128 view. For inline
strings that already exist in the map, this fetch is unnecessary since the
comparison at line 310 uses only the view. Consider deferring the fetch to only
when needed: after the fast-path inline comparison fails, or when inserting a
new value. This would further optimize the common case of duplicate inline
strings.
##########
datafusion/physical-expr-common/src/binary_view_map.rs:
##########
@@ -328,8 +329,7 @@ where
} else {
&in_progress[offset..offset + stored_len]
Review Comment:
The condition checking which buffer to read from should be more defensive.
If `buffer_index > completed.len()`, this would incorrectly try to access
`in_progress` with potentially invalid offsets. While this shouldn't happen
with the current append logic, consider adding an assertion or explicit error
handling to catch any future bugs. For example: `else if buffer_index ==
completed.len()` followed by `else { panic!("Invalid buffer_index") }`.
```suggestion
} else if buffer_index == completed.len() {
&in_progress[offset..offset + stored_len]
} else {
panic!("Invalid buffer_index: {}", buffer_index);
```
--
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]
