You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "abhishek593 (via GitHub)" <gi...@apache.org> on 2026/02/17 07:42:41 UTC
[PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
abhishek593 opened a new pull request, #49304:
URL: https://github.com/apache/arrow/pull/49304
### Rationale for this change
The rank kernel incorrectly treated NaNs and Nulls as ties. This fix ensures they are treated as distinct values according to Arrow's sorting conventions.
### What changes are included in this PR?
Updated the internal MarkDuplicates helper in vector_rank.cc to distinguish between NaNs and Nulls.
### Are these changes tested?
Yes. Added a regression test TestRank.NaNsAndNulls in vector_sort_test.cc and verified all compute tests pass.
### Are there any user-facing changes?
The output of the rank function will now correctly differentiate between NaNs and Nulls instead of ranking them as ties. Fixes incorrect/invalid ranking results for datasets containing both NaNs and Nulls.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #49304:
URL: https://github.com/apache/arrow/pull/49304#discussion_r3111062471
##########
cpp/src/arrow/compute/kernels/vector_rank.cc:
##########
@@ -55,10 +57,14 @@ void MarkDuplicates(const NullPartitionResult& sorted, ValueSelector&& value_sel
// Process nulls
if (sorted.nulls_end != sorted.nulls_begin) {
- // TODO this should be able to distinguish between NaNs and real nulls (GH-45193)
auto it = sorted.nulls_begin;
+ bool prev_is_null = is_null_selector(*it);
while (++it < sorted.nulls_end) {
- *it |= kDuplicateMask;
+ bool curr_is_null = is_null_selector(*it);
Review Comment:
By the way, we know that, after sorting, nulls are clustered either before or either NaNs, so we could take advantage of that to avoid looking the null bitmap for each element. But I think the current solution is still good enough.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #49304:
URL: https://github.com/apache/arrow/pull/49304#issuecomment-4271451064
Let's wait for @pitrou who might want to take another look.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #49304:
URL: https://github.com/apache/arrow/pull/49304#discussion_r3103415012
##########
cpp/src/arrow/compute/kernels/vector_sort_test.cc:
##########
@@ -2362,6 +2341,24 @@ TEST_F(TestRank, Real) {
}
}
+TEST_F(TestRank, NaNsAndNulls) {
+ auto type = float64();
+ auto array = ArrayFromJSON(type, "[1.0, null, NaN, 2.0, NaN, null]");
+ SetInput(array);
+
+ // Sorted order (at_end): [1.0, 2.0, NaN, NaN, null, null]
+ // Ranks (min): [1, 5, 3, 2, 3, 5]
+ auto expected_at_end = ArrayFromJSON(uint64(), "[1, 5, 3, 2, 3, 5]");
+ AssertRank(SortOrder::Ascending, NullPlacement::AtEnd, RankOptions::Min,
+ expected_at_end);
+
+ // Sorted order (at_start): [null, null, NaN, NaN, 1.0, 2.0]
+ // Ranks (min): [5, 1, 3, 6, 3, 1]
+ auto expected_at_start = ArrayFromJSON(uint64(), "[5, 1, 3, 6, 3, 1]");
+ AssertRank(SortOrder::Ascending, NullPlacement::AtStart, RankOptions::Min,
+ expected_at_start);
Review Comment:
Thanks for the update!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
Posted by "abhishek593 (via GitHub)" <gi...@apache.org>.
abhishek593 commented on code in PR #49304:
URL: https://github.com/apache/arrow/pull/49304#discussion_r2964321189
##########
cpp/src/arrow/compute/kernels/vector_rank.cc:
##########
@@ -38,8 +38,9 @@ namespace {
// is the same as the value at the previous sort index.
constexpr uint64_t kDuplicateMask = 1ULL << 63;
-template <typename ValueSelector>
-void MarkDuplicates(const NullPartitionResult& sorted, ValueSelector&& value_selector) {
+template <typename ArrowType, typename ValueSelector, typename IsNullSelector>
+void MarkDuplicates(const NullPartitionResult& sorted, ValueSelector&& value_selector,
+ IsNullSelector&& is_null_selector) {
using T = decltype(value_selector(int64_t{}));
Review Comment:
@pitrou I have updated the PR with suggested changes.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #49304:
URL: https://github.com/apache/arrow/pull/49304#issuecomment-4289835915
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 293730128c3dbe7b59fa4ca0de8acb8bc3bbf018.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/72347068877) has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49304:
URL: https://github.com/apache/arrow/pull/49304#issuecomment-4281240832
This LGTM, but there is a C++ lint failure, can you please reformat @abhishek593 ?
(this should be as easy as `pre-commit run -a cpp`)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #49304:
URL: https://github.com/apache/arrow/pull/49304
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #49304:
URL: https://github.com/apache/arrow/pull/49304#discussion_r2910682511
##########
cpp/src/arrow/compute/kernels/vector_rank.cc:
##########
@@ -38,8 +38,9 @@ namespace {
// is the same as the value at the previous sort index.
constexpr uint64_t kDuplicateMask = 1ULL << 63;
-template <typename ValueSelector>
-void MarkDuplicates(const NullPartitionResult& sorted, ValueSelector&& value_selector) {
+template <typename ArrowType, typename ValueSelector, typename IsNullSelector>
+void MarkDuplicates(const NullPartitionResult& sorted, ValueSelector&& value_selector,
+ IsNullSelector&& is_null_selector) {
using T = decltype(value_selector(int64_t{}));
Review Comment:
Idea: instead of specializing on `ArrowType`, we can just pass a `is_null_selector` that always returns true for types without NaNs.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
Posted by "abhishek593 (via GitHub)" <gi...@apache.org>.
abhishek593 commented on code in PR #49304:
URL: https://github.com/apache/arrow/pull/49304#discussion_r3103114564
##########
cpp/src/arrow/compute/kernels/vector_sort_test.cc:
##########
@@ -2362,6 +2341,24 @@ TEST_F(TestRank, Real) {
}
}
+TEST_F(TestRank, NaNsAndNulls) {
+ auto type = float64();
+ auto array = ArrayFromJSON(type, "[1.0, null, NaN, 2.0, NaN, null]");
+ SetInput(array);
+
+ // Sorted order (at_end): [1.0, 2.0, NaN, NaN, null, null]
+ // Ranks (min): [1, 5, 3, 2, 3, 5]
+ auto expected_at_end = ArrayFromJSON(uint64(), "[1, 5, 3, 2, 3, 5]");
+ AssertRank(SortOrder::Ascending, NullPlacement::AtEnd, RankOptions::Min,
+ expected_at_end);
+
+ // Sorted order (at_start): [null, null, NaN, NaN, 1.0, 2.0]
+ // Ranks (min): [5, 1, 3, 6, 3, 1]
+ auto expected_at_start = ArrayFromJSON(uint64(), "[5, 1, 3, 6, 3, 1]");
+ AssertRank(SortOrder::Ascending, NullPlacement::AtStart, RankOptions::Min,
+ expected_at_start);
Review Comment:
@zanmato1984 I have added tests for descending order as well. Sorry for the delay.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
Posted by "abhishek593 (via GitHub)" <gi...@apache.org>.
abhishek593 commented on PR #49304:
URL: https://github.com/apache/arrow/pull/49304#issuecomment-4026368505
@pitrou Please review! Thanks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49304:
URL: https://github.com/apache/arrow/pull/49304#issuecomment-4278995277
Well, there are a number of CI failures that need addressing, already :)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49304:
URL: https://github.com/apache/arrow/pull/49304#issuecomment-4286799534
Thanks for the update @abhishek593 ! I'm going to merge now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49304:
URL: https://github.com/apache/arrow/pull/49304#issuecomment-4279008067
You probably want to rebase from git main, actually, and fix the `span` references to use `std::span`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #49304:
URL: https://github.com/apache/arrow/pull/49304#discussion_r2976822914
##########
cpp/src/arrow/compute/kernels/vector_sort_test.cc:
##########
@@ -2362,6 +2341,24 @@ TEST_F(TestRank, Real) {
}
}
+TEST_F(TestRank, NaNsAndNulls) {
+ auto type = float64();
+ auto array = ArrayFromJSON(type, "[1.0, null, NaN, 2.0, NaN, null]");
+ SetInput(array);
+
+ // Sorted order (at_end): [1.0, 2.0, NaN, NaN, null, null]
+ // Ranks (min): [1, 5, 3, 2, 3, 5]
+ auto expected_at_end = ArrayFromJSON(uint64(), "[1, 5, 3, 2, 3, 5]");
+ AssertRank(SortOrder::Ascending, NullPlacement::AtEnd, RankOptions::Min,
+ expected_at_end);
+
+ // Sorted order (at_start): [null, null, NaN, NaN, 1.0, 2.0]
+ // Ranks (min): [5, 1, 3, 6, 3, 1]
+ auto expected_at_start = ArrayFromJSON(uint64(), "[5, 1, 3, 6, 3, 1]");
+ AssertRank(SortOrder::Ascending, NullPlacement::AtStart, RankOptions::Min,
+ expected_at_start);
Review Comment:
Maybe we can also add cases for `SortOrder::Descending`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org