alamb commented on code in PR #19413:
URL: https://github.com/apache/datafusion/pull/19413#discussion_r2635576725
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/bytes_view.rs:
##########
@@ -269,30 +277,28 @@ impl<B: ByteViewType> ByteViewGroupValueBuilder<B> {
return false;
}
+ // get the full values and compare
let exist_full = {
let byte_view = ByteView::from(exist_view);
- self.value(
- byte_view.buffer_index as usize,
- byte_view.offset as usize,
- byte_view.length as usize,
- )
+ let buffer_index = byte_view.buffer_index as usize;
+ let offset = byte_view.offset as usize;
+ let length = byte_view.length as usize;
+ debug_assert!(buffer_index <= self.completed.len());
+
+ unsafe {
+ if buffer_index < self.completed.len() {
+ let block = self.completed.get_unchecked(buffer_index);
+ block.as_slice().get_unchecked(offset..offset + length)
+ } else {
+ self.in_progress.get_unchecked(offset..offset + length)
+ }
+ }
};
let input_full: &[u8] = unsafe {
array.value_unchecked(rhs_row).as_ref() };
exist_full == input_full
}
}
- fn value(&self, buffer_index: usize, offset: usize, length: usize) ->
&[u8] {
Review Comment:
I inlined this function at the callsite and converted it to unsafe - I felt
it was easier to understand why this is safe when it was next to its use
--
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]
