jhorstmann commented on code in PR #1048:
URL: https://github.com/apache/arrow-rs/pull/1048#discussion_r889509084
##########
arrow/src/array/ord.rs:
##########
@@ -92,13 +93,22 @@ where
let left_values = StringArray::from(left.values().data().clone());
let right_values = StringArray::from(right.values().data().clone());
- Box::new(move |i: usize, j: usize| {
- let key_left = left_keys.value(i).to_usize().unwrap();
- let key_right = right_keys.value(j).to_usize().unwrap();
- let left = left_values.value(key_left);
- let right = right_values.value(key_right);
- left.cmp(right)
- })
+ // only compare by keys if both arrays actually point to the same value
buffers
+ if left.is_ordered() && ArrayData::ptr_eq(left_values.data(),
right_values.data()) {
Review Comment:
I might be missing something, but I think this is the only place where we
check whether to sort by keys. Could probably make this even stricter and check
that `left` and `right` are the same pointer, then both would be guaranteed to
have the same `is_ordered` value. The way it is now, in theory `right` could be
marked as `is_ordered` but use the same dictionary values as another array
which is not marked.
--
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]
