Support SQL-compliant NaN ordering for DictionaryArray and non-DictionaryArray #2600
Conversation
|
cc @sunchao |
viirya
force-pushed
the
nan_ordering_dict_non_dict
branch
from
5b637c4
to
001bb03
Compare
| if is_nan(a) && is_nan(b) { | ||
| true | ||
| } else { | ||
| a == b | ||
| } |
There was a problem hiding this comment.
minor:
| if is_nan(a) && is_nan(b) { | |
| true | |
| } else { | |
| a == b | |
| } | |
| (is_nan(a) && is_nan(b)) || (a == b) |
| if is_nan(b) { | ||
| true | ||
| } else { | ||
| a <= b | ||
| } |
There was a problem hiding this comment.
minor:
| if is_nan(b) { | |
| true | |
| } else { | |
| a <= b | |
| } | |
| is_nan(b) || (a <= b) |
| if is_nan(a) { | ||
| true | ||
| } else { | ||
| a >= b | ||
| } |
There was a problem hiding this comment.
minor:
| if is_nan(a) { | |
| true | |
| } else { | |
| a >= b | |
| } | |
| is_nan(a) || (a >= b) |
| if is_nan(a) && is_nan(b) { | ||
| true | ||
| } else { | ||
| a == b | ||
| } |
There was a problem hiding this comment.
minor
| if is_nan(a) && is_nan(b) { | |
| true | |
| } else { | |
| a == b | |
| } | |
| (is_nan(a) && is_nan(b)) || a == b) |
| if is_nan(a) && is_nan(b) { | ||
| false | ||
| } else { | ||
| a != b | ||
| } |
There was a problem hiding this comment.
minor
| if is_nan(a) && is_nan(b) { | |
| false | |
| } else { | |
| a != b | |
| } | |
| !(is_nan(a) && is_nan(b)) && (a != b) |
| if is_nan(a) && is_nan(b) { | ||
| false | ||
| } else { | ||
| a != b | ||
| } |
There was a problem hiding this comment.
minor
| if is_nan(a) && is_nan(b) { | |
| false | |
| } else { | |
| a != b | |
| } | |
| !(is_nan(a) && is_nan(b)) && (a != b) |
| if is_nan(a) { | ||
| false | ||
| } else if is_nan(b) { | ||
| true | ||
| } else { | ||
| a < b | ||
| } |
There was a problem hiding this comment.
minor
| if is_nan(a) { | |
| false | |
| } else if is_nan(b) { | |
| true | |
| } else { | |
| a < b | |
| } | |
| !is_nan(a) && (is_nan(b) || (a < b)) |
| if is_nan(b) { | ||
| is_nan(a) | ||
| } else { | ||
| a >= b | ||
| } |
There was a problem hiding this comment.
minor
| if is_nan(b) { | |
| is_nan(a) | |
| } else { | |
| a >= b | |
| } | |
| (is_nan(b) && is_nan(a)) || (!is_nan(b) && (a >= b)) |
| if is_nan(b) { | ||
| !is_nan(a) | ||
| } else if is_nan(a) { | ||
| false | ||
| } else { | ||
| a < b | ||
| } |
There was a problem hiding this comment.
minor
| if is_nan(b) { | |
| !is_nan(a) | |
| } else if is_nan(a) { | |
| false | |
| } else { | |
| a < b | |
| } | |
| (is_nan(b) && !is_nan(a) ) || | |
| (!is_nan(b) && !is_nan(a) && (a < b)) |
There was a problem hiding this comment.
Actually I'm not a fan of these shorten expressions. It is more hard to read and easier to mistake.
Note: I just checked compiled code. It appears to be the same.
There was a problem hiding this comment.
Agree. It's more readable this way.
|
@tustvold As I'm going to use total_cmp, how about we merge this first in, and then I propose a PR to change to total_cmp and clean up the feature flag? |
| if is_nan(b) { | ||
| !is_nan(a) | ||
| } else if is_nan(a) { | ||
| false | ||
| } else { | ||
| a < b | ||
| } |
There was a problem hiding this comment.
Agree. It's more readable this way.
|
I'm going to merge this in and make a PR to use total_cmp and remove the flag. |
|
Benchmark runs are scheduled for baseline = 248fa30 and contender = 24036e9. 24036e9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2599.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?