Warn and preserve order for Polars left joins

[sabasiddique1]

Ref #1869
Goal
Ensure Polars left joins preserve row order and warn users when their Polars version cannot enforce it.

What changed
In skrub/_join_utils.py, set maintain_order="left" when supported by Polars
If unsupported, emit a warning about possible row reordering

Tests
pytest skrub/tests/test_join_utils.py -v

[rcap107]

Hi @sabasiddique1, the issue was referring to the behavior of the polars join in the context of the skrub data ops, rather than the joiners. That is why the issue was labeled as "internal" (now called "core dev") and "data_ops".

The data ops are a part of the codebase that should be touched mostly by maintainers, so that part of the issue should not be handled here.

However, the fix you made is very relevant to the joiner behavior, so it might still be useful to implement.

I am wondering if we should raise an exception directly if polars 0.20 doesn't support a way to maintain the row order, or if a user warning is enough.

In any case, I'm not convinced we should move on with this PR because we are planning to refactor most of the joiners, so it's unclear at this point how useful this PR would be.

[sabasiddique1]

Hi @rcap107, thanks for the clarification -- that makes sense.

Since this is mainly about DataOps and you’re planning a joiner refactor, I’m happy to pause/close this PR to avoid churn. My change is in the joiner path, not DataOps.

If you still want this as a small joiner‑only improvement (and have a preference for warning vs hard error on older Polars), I can update it. Otherwise I’ll close it and wait for guidance after the refactor.

Thanks again!

[rcap107]

Hi @rcap107, thanks for the clarification -- that makes sense.

Since this is mainly about DataOps and you’re planning a joiner refactor, I’m happy to pause/close this PR to avoid churn. My change is in the joiner path, not DataOps.

If you still want this as a small joiner‑only improvement (and have a preference for warning vs hard error on older Polars), I can update it. Otherwise I’ll close it and wait for guidance after the refactor.

Thanks again!

Thanks for the understanding. For the moment, we can pause the PR. I'll come back in a few days to let you know how we're going to proceed. This is also because we might be bumping the polars requirements, and that would avoid the need to check whether maintain_order is available as a parameter.

[sabasiddique1]

Thanks for the update. I’ll pause this PR and wait for your guidance after the refactor / polars bump. Let me know how you’d like to proceed.

[rcap107]

Hi @sabasiddique1, in the end we're going to bump the polars requirements so that the minimum version is 1.0, but since we'll do that in a minor release, that won't happen for a few more weeks.

In the meantime, you can simplify the code to assume that the min version is 1.0 and that maintain_order is there. We will come back to the PR once we're ready to move on with the next release. Thanks!

[rcap107]

Hi @sabasiddique1, I opened #1897 to bump the requirements. Once that PR gets merged, we can move on with this.

[rcap107]

Hi @sabasiddique1, it took longer than I thought, but we have bumped the Polars requirements to 1.5, so now it should be possible to simplify the check for the parameter.

[sabasiddique1]

Hi @rcap107, thanks for the updates, this looks much cleaner now.

Before I jump in though, are you planning to take this forward, or would you like me to continue working on it?

[rcap107]

Hi @sabasiddique1, please continue with it! I was updating the PR as it had been a while and just ended up removing that part on the old polars reqs, I shouldn't have touched it actually.

The main thing that is missing before we can merge this is a test that checks for the new lines, which is what coverage is complaining about.

[sabasiddique1]

Hi @rcap107, picking this back up — sorry for the delay.

Two things from looking at the current branch:

1. The maintain_order block is nested inside the else of the coalesce check, so it only runs when coalesce is absent. Every CI env has coalesce, so those lines never execute — that's likely the 0% diff coverage.
2. We can't drop the check yet: maintain_order was added to DataFrame.join in polars 1.17 (feat: Add maintain_order parameter to joins pola-rs/polars#20026), not 1.5. coalesce is the one that's safe to assume. With min-deps pinned to 1.5.0, passing it unconditionally would TypeError there.

So either keep a runtime check + warning, or bump min polars to 1.17 in a minor release and drop it. On warning vs error — I'd lean warning: polars does preserve left order in practice today, so what's missing is the guarantee, not the behaviour. Raising would break working code on 1.5–1.16.

I'll push the fix with tests covering both branches unless you'd rather go the version-bump route.

[sabasiddique1]

Pushed the fix and tests, coverage should be fine now. But one job fails: ci-py310-min-optional-deps (polars 1.5.0), on test_fuzzy_join.py::test_perfect_matches[polars]. That test runs under warnings.simplefilter("error"), so the new warning becomes an exception. The rest are fail-fast cancellations.

Which makes me think warning on every join isn't great — any test treating warnings as errors will break, and silencing it in that one test just moves the problem.

Would you rather bump min polars to 1.17 (where maintain_order landed) and drop the check entirely? The polars path becomes a single join call, no warning, no version-dependent test. I didn't want to push a dependency bump myself, but happy to do it here or in a separate PR.

Otherwise I'll just ignore the warning in test_perfect_matches. Let me know.

[rcap107]

Hi @sabasiddique1, as a rule of thumb we typically don't use warnings almost anywhere in the library.

So, the better alternative looks to be bumping the min version of polars to 1.17, which was released in December 2024. Bumping the release would simplify a lot the code, and since we're going to bump dependencies eventually anyway it means that a lot of what has been done here would become redundant when that happens.

If we go with that strategy, however, this PR would have to wait until we actually do bump the requirements, which will have to wait until 2 years since 1.17 has passed, meaning December. Would that be ok with you?