fix: Multi-output column expressions in frame sort method

[alexander-beedie]

Several fixes relating to multi-output expressions (including selectors) in frame sort method:

• Better handle multi-output column expressions in by parameter; could raise an incorrect "the amount of ordering booleans … does not match the number of series …" error depending on the number of such expressions passed and how many columns they actually selected.
• Don't panic if multi-output column expression evaluates to no columns - bail with a suitable error instead.
• Correctly align/expand "nulls_last" & "descending" booleans when we expand multi-output expressions.

Also: standardised on "{:?}" format string for Rust-side ColumnNotFound error so we always get the quotes around the column name (which helps to see things like the empty string being used as a column name, which otherwise just looks like an incomplete error message). We had mixed usage here; sometimes it would show the quotes, sometimes it wouldn't (update: now added polars_err!(col_not_found=name) to centralise this).

Examples

from datetime import date
import polars.selectors as cs
import polars as pl

df = pl.DataFrame({
    "dts": [date(2077,10,3), date(2077,10,2), date(2077,10,2)],
    "strs": ["abc", "def", "ghi"],
    "vals": [10.5, 20.3, 15.7],
})

Before

Panic if no matching cols:

df.sort(pl.col("^x.*$"))
# PanicException: index out of bounds: the len is 0 but the index is 0

Failed to use selectors in by:

df.sort(cs.temporal() | cs.binary(), cs.string(), cs.integer())
# ComputeError: the amount of ordering booleans: 3 does not match the number of series: 2

After

Descriptive error if no matching cols:

df.sort(pl.col("^x.*$"))
# ComputeError: No columns selected for sorting

Successful use of selectors in by:

df.sort(cs.temporal() | cs.binary(), cs.string(), cs.integer())
# shape: (3, 3)
# ┌────────────┬──────┬──────┐
# │ dts        ┆ strs ┆ vals │
# │ ---        ┆ ---  ┆ ---  │
# │ date       ┆ str  ┆ f64  │
# ╞════════════╪══════╪══════╡
# │ 2077-10-02 ┆ def  ┆ 20.3 │
# │ 2077-10-02 ┆ ghi  ┆ 15.7 │
# │ 2077-10-03 ┆ abc  ┆ 10.5 │
# └────────────┴──────┴──────┘

Note that use of multi-output expressions in by may or may not fail in the current build - it depends on how many such expressions there are in relation to how many columns actually get selected/expanded.

[codecov]

Codecov Report

Attention: Patch coverage is 73.68421% with 5 lines in your changes missing coverage. Please review.

Project coverage is 80.40%. Comparing base (dea0679) to head (888803f).
Report is 4 commits behind head on main.

Files	Patch %	Lines
crates/polars-core/src/frame/mod.rs	83.33%	2 Missing ⚠️
...re/src/chunked_array/ops/sort/arg_sort_multiple.rs	0.00%	1 Missing ⚠️
crates/polars-io/src/partition.rs	0.00%	1 Missing ⚠️
...ates/polars-plan/src/plans/conversion/dsl_to_ir.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17947      +/-   ##
==========================================
+ Coverage   80.33%   80.40%   +0.06%     
==========================================
  Files        1494     1494              
  Lines      196451   196480      +29     
  Branches     2817     2817              
==========================================
+ Hits       157828   157977     +149     
+ Misses      38103    37983     -120     
  Partials      520      520              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ritchie46]

Note that use of multi-output expressions in by may or may not fail in the current build - it depends on how many such expressions there are in relation to how many columns actually get selected.

That's wrong. The columns selected by the sorted should also be selected by the optimizer. Do you mean we don't do that atm?

[alexander-beedie]

That's wrong. The columns selected by the sorted should also be selected by the optimizer. Do you mean we don't do that atm?

It's because we currently check that the number of column expressions matches the vec lengths of "nulls_last" / "descending" params before those expressions are expanded (there's an incorrect assumption baked-in that one expression == one column).

If the expansion happens to have the same number of cols as those vec lengths then no error is raised (but can still be a bug as we may not have the intended sort behaviour if mixed true/false in "nulls_last" / "descending"). If we expand to more/less cols than those vec lengths we get an error from args_validate.

Meant to park this in Draft last night while I track down the best place to handle this properly, as it's definitely not the Python check I added 😜

Update: have now fixed this properly such that we assign the indicated bool to the expanded exprs (and the fix is down in DSL/IR, not Python ;)

[ritchie46]

Great!