TopKRow ordering is inconsistent, preventing a possible sort optimization

[MassivePizza]

Describe the bug

There is already a note about this in the PartialOrd impl. This is not consistent with PartialEq, which compares row, batch_id and index (that feels weird on its own; you'd think row is enough for eq in this context).

To Reproduce

Attempting to optimize heap drains can change ordering, which makes some sqllogictest(s) fail since duplicate rows get drained in a new order. I ran into this when looking into TopK performance. Turns out that BinaryHeap::into_sorted_vec is slow (see rust-lang/rust#115357) and doesn't have any particular ordering guarantees when draining.

See the test failures on this CI run.

Expected behavior

Ord impl on TopKRow accounts for batch_id and index to stabilize ordering of equal elements.
Current tests won't pass with this change though, making it a breaking change.

Here are test failures with stable ordering: fail2.txt

Additional context

In our case let vec = heap.into_vec(); vec.sort_unstable(); vec is around 55% faster than heap.into_sorted_vec() for 50000 elements (courtesy of a new bench). Stabilized ordering would unlock that optimization; BinaryHeap::into_sorted_vec and Vec::sort_unstable produce the same order with stable ordering. Structures other than BinaryHeap could also be used in the future. I've been looking into min-max heaps but could only get a 3-5% improvement over the BinaryHeap in std.

Another solution could be making sort_unstable opt-in with a config, which would avoid the breaking change. Unless we can rely the fact that TopK is already opt-in?
I think accounting for batch_id and index could be good future proofing though (see expected behavior above).

[MassivePizza]

I suppose this issue could also be labeled as a feature request.