feat(rust, python): add `maintain_order` argument to `sort`/`top_k`/`bottom_k` #9672

CloseChoice · 2023-07-02T02:26:02Z

ritchie46

Thanks for going through the code churn. This looks good. Can you add the mentioned comment in all docstrings?

You must also ensure we don't convert to streaming if this flag is set as the streaming API will not have a stable sort.

ritchie46 · 2023-07-02T12:40:39Z

py-polars/polars/lazyframe/frame.py

@@ -1236,6 +1249,8 @@ def bottom_k(
            per column by passing a sequence of booleans.
        nulls_last
            Place null values last.
+        maintain_order
+            Whether the order should be maintained if elements are equal.


Can you add a note that this is more expensive and blocks streaming.

CloseChoice · 2023-07-02T19:51:28Z

Thanks for going through the code churn. This looks good. Can you add the mentioned comment in all docstrings?

You must also ensure we don't convert to streaming if this flag is set as the streaming API will not have a stable sort.

Done, not quite sure why the python test fails, is this known to be a fluky test?

Edit: Ah, I added streaming to the test_all features, that's probably the reason for the failed test.

ritchie46 · 2023-07-03T11:48:27Z

polars/polars-core/src/frame/mod.rs

    ) -> PolarsResult<&mut Self> {
+        println!("In sort in place");


A rogue print

ritchie46 · 2023-07-03T11:49:45Z

polars/polars-lazy/src/physical_plan/streaming/checks.rs

@@ -3,6 +3,10 @@ use polars_plan::prelude::*;

 pub(super) fn is_streamable_sort(args: &SortArguments) -> bool {
    // check if slice is positive
+    if args.maintain_order {


We shouldn't print here. And can we combine the check with the slice check.

ritchie46 · 2023-07-03T11:50:58Z

polars/polars-algo/src/algo.rs

@@ -97,7 +97,7 @@ pub fn hist(s: &Series, bins: Option<&Series>, bin_count: Option<usize>) -> Resu

    cuts.left_join(&out, [category_str], [category_str])?
        .fill_null(FillNullStrategy::Zero)?
-        .sort(["category"], false)
+        .sort(["category"], false, false)


Now that I think of this, I think we should make a struct called something like SortOptions that implements Default. That should make calling this from the rust side much more ergonomic.

Are you sure that this is something for this PR?

We already have SortOptions (s. here) and also the possibility to sort_with_options. The difference between sort and sort_with_options is that we cannot specify multiple descending columns in sort_with_options (e.g. this is not possible with sort_with_options). I guess a generalization of SortOptions might be needed for this, but I don't see this in this PR.

The reason I propose this is because the number of arguments are becoming unwieldy and that can be resolved with an argument struct. Would you be willing to make a follow up PR on that?

Yes, already thought about that. I think I might need some guidance on how to best do this design-wise but I can do the implementation.

I just created an issue for this: #9820

ritchie46 · 2023-07-12T06:27:27Z

Alright, can you do a rebase @CloseChoice? Then we can do the argument struct in a later PR.

ritchie46 · 2023-07-12T07:11:02Z

Thanks @CloseChoice

…bottom_k` (pola-rs#9672)

stinodego changed the title ~~Feature/sort maintain order~~ fix(rust, python): add maintain_order argument to sort/top_k/bottom_k Jul 2, 2023

stinodego changed the title ~~fix(rust, python): add maintain_order argument to sort/top_k/bottom_k~~ feat(rust, python): add maintain_order argument to sort/top_k/bottom_k Jul 2, 2023

github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars enhancement New feature or an improvement of an existing feature labels Jul 2, 2023

CloseChoice marked this pull request as ready for review July 2, 2023 09:35

CloseChoice requested review from ritchie46, stinodego, alexander-beedie and universalmind303 as code owners July 2, 2023 09:35

ritchie46 reviewed Jul 2, 2023

View reviewed changes

CloseChoice requested a review from ritchie46 July 3, 2023 10:59

ritchie46 reviewed Jul 3, 2023

View reviewed changes

CloseChoice requested a review from MarcoGorelli as a code owner July 9, 2023 19:44

CloseChoice force-pushed the feature/sort-maintain-order branch from 7bd6fdb to 1b0a875 Compare July 9, 2023 19:51

CloseChoice requested a review from ritchie46 July 11, 2023 21:50

CloseChoice added 11 commits July 12, 2023 08:34

WIP: add maintain order

3a6de11

fix linting and remove prints

cd2bf24

rebasing

d6ad422

remove prints in rust

c2bf5a2

fix rust tests

ba1de2e

fix errors

4362755

fix type errors python

3b3ab70

update formatting

535839e

fix linting

b97eaed

fix doctest

715025f

block streaming mode if maintain_order=True

94052d9

CloseChoice added 8 commits July 12, 2023 08:36

fix black

082a5f5

fix rust test

c5af5bc

fix python docstrings

6fcc452

reformat frame.py

425536b

fix docstring

64654cf

remove streaming from polars-lazy test features

0377abf

update is_streamable_sort

be483f4

remove print

e1fdfb1

CloseChoice force-pushed the feature/sort-maintain-order branch from a19277e to e1fdfb1 Compare July 12, 2023 06:36

remove qcut

4ab24ce

ritchie46 merged commit a1d5a22 into pola-rs:main Jul 12, 2023

CloseChoice deleted the feature/sort-maintain-order branch July 12, 2023 07:16

c-peters pushed a commit to c-peters/polars that referenced this pull request Jul 14, 2023

feat(rust, python): add maintain_order argument to sort/top_k/`…

141ffdb

…bottom_k` (pola-rs#9672)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rust, python): add `maintain_order` argument to `sort`/`top_k`/`bottom_k` #9672

feat(rust, python): add `maintain_order` argument to `sort`/`top_k`/`bottom_k` #9672

CloseChoice commented Jul 2, 2023

ritchie46 left a comment

ritchie46 Jul 2, 2023

CloseChoice Jul 2, 2023

CloseChoice commented Jul 2, 2023 •

edited

Loading

ritchie46 Jul 3, 2023

CloseChoice Jul 10, 2023

ritchie46 Jul 3, 2023

CloseChoice Jul 10, 2023

ritchie46 Jul 3, 2023

CloseChoice Jul 9, 2023

ritchie46 Jul 10, 2023

CloseChoice Jul 10, 2023

CloseChoice Jul 11, 2023

ritchie46 commented Jul 12, 2023

ritchie46 commented Jul 12, 2023

feat(rust, python): add maintain_order argument to sort/top_k/bottom_k #9672

feat(rust, python): add maintain_order argument to sort/top_k/bottom_k #9672

Conversation

CloseChoice commented Jul 2, 2023

ritchie46 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CloseChoice commented Jul 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritchie46 commented Jul 12, 2023

ritchie46 commented Jul 12, 2023

feat(rust, python): add `maintain_order` argument to `sort`/`top_k`/`bottom_k` #9672

feat(rust, python): add `maintain_order` argument to `sort`/`top_k`/`bottom_k` #9672

CloseChoice commented Jul 2, 2023 •

edited

Loading