MassivePizza opened a new issue, #24659: URL: https://github.com/apache/datafusion/issues/24659
### Describe the bug There is already a note about this [in the PartialOrd impl](https://github.com/apache/arrow-datafusion/blob/97d8c0d905b82ac3228453cbff185ab6ad646ef0/datafusion/physical-plan/src/topk/mod.rs#L1161). 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. ### Expected behavior Ord impl on TopKRow accounts for `batch_id` and `index` to stabilize ordering of equal elements. ### Additional context I ran into this when looking into TopK performance. Turns out that `BinaryHeap::into_sorted_vec` is slow (see https://github.com/rust-lang/rust/issues/115357) and doesn't have any particular ordering guarantees when draining. In our case, `let vec = heap.into_vec(); vec.sort_unstable()` is around 55% faster than `heap.into_sorted_vec()` for 50000 elements. The problem is that tests didn't pass anymore, which makes this optimization a breaking change. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
