Implement ord for FixedSizeBinary types#2905
Conversation
| let right: Decimal128Array = Decimal128Array::from(right.data().clone()); | ||
| Box::new(move |i, j| left.value(i).cmp(&right.value(j))) | ||
| } | ||
| (FixedSizeBinary(_), FixedSizeBinary(_)) => { |
There was a problem hiding this comment.
FYI the comparison and sort kernels do not make use of this DynComparator, and I actually hope to at some point deprecate it in favor of the row format.
There was a problem hiding this comment.
That may be but we were running into the unhandled case in this match firing when testing apache/datafusion#3911
|
Tests added, issue link fixed. |
| use crate::error::Result; | ||
| use std::cmp::Ordering; | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
Even better would be some tests with more than one value in the list, and some tests of more than just Ordering::Less, but don't feel strongly 😅
|
Benchmark runs are scheduled for baseline = be48377 and contender = f629a2e. f629a2e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
3 participants
Which issue does this PR close?
Closes #2904
Rationale for this change
Implementing sorting / grouping in datafusion on columns of type FixedSizeBinary requires that ord be implemented for FixedSizeBinary types.