Fix nulls ordering for Range frames #9271
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
aditi-pandit
force-pushed
the
window_range_null
branch
2 times, most recently
from
56ee8b8
to
2f101af
Compare
aditi-pandit
force-pushed
the
window_range_null
branch
from
2f101af
to
47329cc
Compare
velox/exec/WindowPartition.cpp
Outdated
| if constexpr (isAscending) { | ||
| flags.nullsFirst = sortKeyInfo_[0].second.isNullsFirst(); | ||
| } else { | ||
| flags.nullsFirst = !sortKeyInfo_[0].second.isNullsFirst(); | ||
| } |
There was a problem hiding this comment.
Curious why we don't simply set flags = {sortKeyInfo_[0].second.isNullsFirst(), isAscending, false} but need this custom logic instead?
There was a problem hiding this comment.
@kagamiori : The actual data could be ascending or descending ordered (depending on ORDER BY). The range frame is trying to find a particular value in the ORDER BY column. But depending on preceding/following asc/desc we look for smallest value greater than or greatest value less than. The comparisons for this were simpler to write if the comparison test was with asc semantics only. So the comparison flags are always ascending (like you have put together), but the nulls flag needs to flip depending on asc/desc and how it compares with values in ascending semantics.
Hope this sounds okay. We can discuss if you need further clarification on slack/workspace.
There was a problem hiding this comment.
I see. This makes sense to me. Thank you for explaining! Could we add a comment here with this explanation too?
There was a problem hiding this comment.
@kagamiori : As I played with the comparisons more, this code could be simplified keeping CompareFlags consistent with the ordering. I think the current code reads well with the current comments. But please let me know your suggestions.
aditi-pandit
force-pushed
the
window_range_null
branch
from
47329cc
to
ff0e548
Compare
|
The connbench regressions are unrelated to the code in this PR. |
aditi-pandit
force-pushed
the
window_range_null
branch
from
ff0e548
to
ef61cf3
Compare
There was a problem hiding this comment.
LGTM. Shall we add the test coverage in WindowTestBase as well?
(cc'ing @mbasmanova in case she has any concern with this usage of CompareFlags.)
velox/exec/WindowPartition.cpp
Outdated
| if constexpr (isAscending) { | ||
| flags.nullsFirst = sortKeyInfo_[0].second.isNullsFirst(); | ||
| } else { | ||
| flags.nullsFirst = !sortKeyInfo_[0].second.isNullsFirst(); | ||
| } |
There was a problem hiding this comment.
I see. This makes sense to me. Thank you for explaining! Could we add a comment here with this explanation too?
|
@kagamiori, @mbasmanova : On second thoughts, there is some more simplification possible. I'm moving this PR to draft mode and will open for review again. |
aditi-pandit
force-pushed
the
window_range_null
branch
2 times, most recently
from
4538cef
to
52094b8
Compare
@kagamiori : This PR is ready for review now. I added array_agg function in the rest of the range frame testing as well. |
|
@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
aditi-pandit
force-pushed
the
window_range_null
branch
from
52094b8
to
defb7cc
Compare
|
@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@kagamiori merged this pull request in da6a3d3. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
The nullsFirst flag weren't correctly passed in column comparisons for range frames. This caused range values for nulls to match those of adjacent rows for desc nulls last and asc nulls first order clauses.
Setting the CompareFlags also made me realize an improvement to simplify the code.
Fixes prestodb/presto#21889