Skip to content

Commit 2937301

Browse files
authored
GH-45193: [C++][Compute] Treat NaNs and nulls as distinct values in rank tie-breaking (#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. * GitHub Issue: #45193 Authored-by: Abhishek Bansal <abhibansal593@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
1 parent adecb17 commit 2937301

2 files changed

Lines changed: 54 additions & 6 deletions

File tree

cpp/src/arrow/compute/kernels/vector_rank.cc

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,9 @@ namespace {
3636
// is the same as the value at the previous sort index.
3737
constexpr uint64_t kDuplicateMask = 1ULL << 63;
3838

39-
template <typename ValueSelector>
40-
void MarkDuplicates(const NullPartitionResult& sorted, ValueSelector&& value_selector) {
39+
template <typename ValueSelector, typename IsNullSelector>
40+
void MarkDuplicates(const NullPartitionResult& sorted, ValueSelector&& value_selector,
41+
IsNullSelector&& is_null_selector) {
4142
using T = decltype(value_selector(int64_t{}));
4243

4344
// Process non-nulls
@@ -55,10 +56,14 @@ void MarkDuplicates(const NullPartitionResult& sorted, ValueSelector&& value_sel
5556

5657
// Process nulls
5758
if (sorted.nulls_end != sorted.nulls_begin) {
58-
// TODO this should be able to distinguish between NaNs and real nulls (GH-45193)
5959
auto it = sorted.nulls_begin;
60+
bool prev_is_null = is_null_selector(*it);
6061
while (++it < sorted.nulls_end) {
61-
*it |= kDuplicateMask;
62+
bool curr_is_null = is_null_selector(*it);
63+
if (curr_is_null == prev_is_null) {
64+
*it |= kDuplicateMask;
65+
}
66+
prev_is_null = curr_is_null;
6267
}
6368
}
6469
}
@@ -82,7 +87,12 @@ Result<NullPartitionResult> DoSortAndMarkDuplicate(
8287
auto value_selector = [&array](int64_t index) {
8388
return GetView::LogicalValue(array.GetView(index));
8489
};
85-
MarkDuplicates(sorted, value_selector);
90+
if constexpr (has_null_like_values<ArrowType>()) {
91+
auto is_null_selector = [&array](int64_t index) { return array.IsNull(index); };
92+
MarkDuplicates(sorted, value_selector, is_null_selector);
93+
} else {
94+
MarkDuplicates(sorted, value_selector, [](int64_t) { return true; });
95+
}
8696
}
8797
return sorted;
8898
}
@@ -105,7 +115,15 @@ Result<NullPartitionResult> DoSortAndMarkDuplicate(
105115
ChunkedArrayResolver(std::span(arrays))](int64_t index) {
106116
return resolver.Resolve(index).Value<ArrowType>();
107117
};
108-
MarkDuplicates(sorted, value_selector);
118+
if constexpr (has_null_like_values<ArrowType>()) {
119+
auto is_null_selector =
120+
[resolver = ChunkedArrayResolver(std::span(arrays))](int64_t index) {
121+
return resolver.Resolve(index).IsNull();
122+
};
123+
MarkDuplicates(sorted, value_selector, is_null_selector);
124+
} else {
125+
MarkDuplicates(sorted, value_selector, [](int64_t) { return true; });
126+
}
109127
}
110128
return sorted;
111129
}

cpp/src/arrow/compute/kernels/vector_sort_test.cc

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2362,6 +2362,36 @@ TEST_F(TestRank, Real) {
23622362
}
23632363
}
23642364

2365+
TEST_F(TestRank, NaNsAndNulls) {
2366+
auto type = float64();
2367+
auto array = ArrayFromJSON(type, "[1.0, null, NaN, 2.0, NaN, null]");
2368+
SetInput(array);
2369+
2370+
// Sorted order (at_end): [1.0, 2.0, NaN, NaN, null, null]
2371+
// Ranks (min): [1, 5, 3, 2, 3, 5]
2372+
auto expected_at_end = ArrayFromJSON(uint64(), "[1, 5, 3, 2, 3, 5]");
2373+
AssertRank(SortOrder::Ascending, NullPlacement::AtEnd, RankOptions::Min,
2374+
expected_at_end);
2375+
2376+
// Sorted order (at_start): [null, null, NaN, NaN, 1.0, 2.0]
2377+
// Ranks (min): [5, 1, 3, 6, 3, 1]
2378+
auto expected_at_start = ArrayFromJSON(uint64(), "[5, 1, 3, 6, 3, 1]");
2379+
AssertRank(SortOrder::Ascending, NullPlacement::AtStart, RankOptions::Min,
2380+
expected_at_start);
2381+
2382+
// Sorted order (descending, at_end): [2.0, 1.0, NaN, NaN, null, null]
2383+
// Ranks (min): [2, 5, 3, 1, 3, 5]
2384+
auto expected_desc_at_end = ArrayFromJSON(uint64(), "[2, 5, 3, 1, 3, 5]");
2385+
AssertRank(SortOrder::Descending, NullPlacement::AtEnd, RankOptions::Min,
2386+
expected_desc_at_end);
2387+
2388+
// Sorted order (descending, at_start): [null, null, NaN, NaN, 2.0, 1.0]
2389+
// Ranks (min): [6, 1, 3, 5, 3, 1]
2390+
auto expected_desc_at_start = ArrayFromJSON(uint64(), "[6, 1, 3, 5, 3, 1]");
2391+
AssertRank(SortOrder::Descending, NullPlacement::AtStart, RankOptions::Min,
2392+
expected_desc_at_start);
2393+
}
2394+
23652395
TEST_F(TestRank, Integral) {
23662396
for (auto integer_type : ::arrow::IntTypes()) {
23672397
SetInput(ArrayFromJSON(integer_type, "[2, 3, 1, 0, 5]"));

0 commit comments

Comments
 (0)