Repository: narwhals-dev/narwhals
Pull request #2314 (2025-03-29 20:25:50)

feat: disallow `.over(order_by=...)` for non-order-dependent operations (as it signals a user error)
We currently allow `nw.col('a').mean().over(order_by='i')`, but probably shouldn't because the `order_by` has no effect there and is a sign of a user-error

<!--
# Thanks for contributing a pull request! 
## Please make sure you see our contribution guidelines: https://github.com/narwhals-dev/narwhals/blob/main/CONTRIBUTING.md
-->

## What type of PR is this? (check all applicable)

- [ ] 💾 Refactor
- [ ] ✨ Feature
- [ ] 🐛 Bug Fix
- [ ] 🔧 Optimization
- [ ] 📝 Documentation
- [ ] ✅ Test
- [ ] 🐳 Other

## Related issues

- Related issue #\<issue number\>
- Closes #\<issue number\>

## Checklist

- [ ] Code follows style guide (ruff)
- [ ] Tests added
- [ ] Documented the changes

## If you have comments or can explain your changes, please do so below


dangotbanned: > We currently allow `nw.col('a').mean().over(order_by='i')`, but probably shouldn't because the `order_by` has no effect there and is a sign of a user-error

That does sound reasonable, but what does `polars` do in that scenario?

AFAIK, `polars` *usually* allows no-ops. I'm guessing because they'd be optimized out anyway
MarcoGorelli: thanks!

yeah but tbh i think it's potentially misleading that Polars no-ops, else a user might expect `pl.col('a').sum().over('b', order_by='c')` to behave like sql `sum(a) over (partition by b order by c)`, whereas it's actually like ``sum(a) over (partition by b)``

Patch:
diff --git a/narwhals/expr.py b/narwhals/expr.py
index 1c6e6772..d1854957 100644
--- a/narwhals/expr.py
+++ b/narwhals/expr.py
@@ -15,6 +15,7 @@ from narwhals._expression_parsing import combine_metadata
 from narwhals._expression_parsing import combine_metadata_binary_op
 from narwhals._expression_parsing import extract_compliant
 from narwhals.dtypes import _validate_dtype
+from narwhals.exceptions import InvalidOperationError
 from narwhals.exceptions import LengthChangingExprError
 from narwhals.expr_cat import ExprCatNamespace
 from narwhals.expr_dt import ExprDateTimeNamespace
@@ -1593,6 +1594,9 @@ class Expr:
         n_open_windows = self._metadata.n_open_windows
         if flat_order_by is not None and self._metadata.kind.is_window():
             n_open_windows -= 1
+        elif flat_order_by is not None and not n_open_windows:
+            msg = "Cannot use `order_by` in `over` on expression which isn't order-dependent."
+            raise InvalidOperationError(msg)
         current_meta = self._metadata
         next_meta = ExprMetadata(
             kind,


Test patch:
diff --git a/.github/workflows/downstream_tests.yml b/.github/workflows/downstream_tests.yml
index eec4e4dc..37603601 100644
--- a/.github/workflows/downstream_tests.yml
+++ b/.github/workflows/downstream_tests.yml
@@ -186,7 +186,9 @@ jobs:
           UV_SYSTEM_PYTHON: 1
         run: |
           cd py-shiny
+          # temporary pin to get ci green
           make narwhals-install-shiny
+          uv pip install "pyflakes<3.3.0" "flake8<7.2.0" --system
       - name: install-narwhals-dev
         run: |
           uv pip uninstall narwhals --system
diff --git a/tests/expression_parsing_test.py b/tests/expression_parsing_test.py
index 21a468e6..89d825ed 100644
--- a/tests/expression_parsing_test.py
+++ b/tests/expression_parsing_test.py
@@ -3,6 +3,7 @@ from __future__ import annotations
 import pytest
 
 import narwhals as nw
+from narwhals.exceptions import InvalidOperationError
 
 
 @pytest.mark.parametrize(
@@ -21,3 +22,8 @@ import narwhals as nw
 )
 def test_has_open_windows(expr: nw.Expr, expected: int) -> None:
     assert expr._metadata.n_open_windows == expected
+
+
+def test_misleading_order_by() -> None:
+    with pytest.raises(InvalidOperationError):
+        nw.col("a").mean().over(order_by="b")