GH-45216: [C++][Compute] Refactor Rank implementation

[pitrou]

Rationale for this change

The Rank implementation currently mixes ties/duplicates detection and rank computation in a single function CreateRankings. This makes it poorly reusable for other Rank-like functions such as the Percentile Rank function proposed in GH-45190.

What changes are included in this PR?

Split duplicates detection into a dedicated function that sets a marker bit in the sort-indices array (it is private to the Rank implementation, so it is safe to mutate it).

The rank computation itself (CreateRankings) becomes simpler and, moreover, it does not need to read the input values: it becomes therefore type-agnostic.

This yields a code size reduction (around 45kB saved on the author's machine):

• before:

$ size /build/build-release/relwithdebinfo/libarrow.so
   text	   data	    bss	    dec	    hex	filename
26072218	 353832	2567985	28994035	1ba69f3	/build/build-release/relwithdebinfo/libarrow.so

• after:

$ size /build/build-release/relwithdebinfo/libarrow.so
   text	   data	    bss	    dec	    hex	filename
26028198	 353832	2567985	28950015	1b9bdff	/build/build-release/relwithdebinfo/libarrow.so

Rank benchmark results are mostly neutral, though there are slight improvements on some benchmarks, and slight regressions especially on all-nulls input.

Are these changes tested?

Yes, by existing tests.

Are there any user-facing changes?

No.

• GitHub Issue: [C++][Compute] Refactor Rank implementation #45216

[pitrou]

ASAN/UBSAN failure in arrow-dataset-dataset-writer-test is unrelated.

[WillAyd]

Looks good and thanks for the ping. Just some small questions but I think this is moving in the right direction

[WillAyd]

Is this leading bit never used by it in the current implementation, or are we just assuming that to be highly unlikely?

[pitrou]

It's not, unless someone manages to fit more than 2**63 elements in a Arrow array :)

[WillAyd]

So the float types are explicitly looking at nan as a value instead of the mask right? That's something you are looking to align in a follow up? Or its still an open question as to how to handle?

[pitrou]

Hmm, this change is actually a small unrelated refactor. It does not change any functionality, it's just moving some of the helper code to more modern C++17 idioms such as if constexpr :)