fix: raise for rank followed by over with order_by for sql-like backends (#3178)

Author: MarcoGorelli

narwhals/_ibis/expr.py

@@ -336,7 +336,18 @@ def _rank(expr: ir.Column) -> ir.Value:
 
             return ibis.cases((expr.notnull(), rank_))
 
-        return self._with_callable(_rank)
+        def window_f(df: IbisLazyFrame, inputs: WindowInputs[ir.Value]) -> list[ir.Value]:
+            if inputs.order_by:
+                msg = "`rank` followed by `over` with `order_by` specified is not supported for Ibis backend."
+                raise NotImplementedError(msg)
+            return [
+                _rank(cast("ir.Column", expr)).over(
+                    ibis.window(group_by=inputs.partition_by)
+                )
+                for expr in self(df)
+            ]
+
+        return self._with_callable(_rank, window_f)
 
     @property
     def str(self) -> IbisExprStringNamespace:

narwhals/_sql/expr.py

@@ -755,17 +755,15 @@ def rank(self, method: RankMethod, *, descending: bool) -> Self:
         def _rank(
             expr: NativeExprT,
             partition_by: Sequence[str | NativeExprT] = (),
-            order_by: Sequence[str | NativeExprT] = (),
             *,
-            descending: Sequence[bool],
-            nulls_last: Sequence[bool],
+            descending: bool,
         ) -> NativeExprT:
             count_expr = self._count_star()
             window_kwargs: dict[str, Any] = {
                 "partition_by": partition_by,
-                "order_by": (expr, *order_by),
-                "descending": descending,
-                "nulls_last": nulls_last,
+                "order_by": (expr,),
+                "descending": [descending],
+                "nulls_last": [True],
             }
             count_window_kwargs: dict[str, Any] = {"partition_by": (*partition_by, expr)}
             window = self._window_expression
@@ -791,21 +789,16 @@ def _rank(
             return self._when(~F("isnull", expr), rank_expr)  # type: ignore[operator]
 
         def _unpartitioned_rank(expr: NativeExprT) -> NativeExprT:
-            return _rank(expr, descending=[descending], nulls_last=[True])
+            return _rank(expr, descending=descending)
 
         def _partitioned_rank(
             df: SQLLazyFrameT, inputs: WindowInputs[NativeExprT]
         ) -> Sequence[NativeExprT]:
-            # node: when `descending` / `nulls_last` are supported in `.over`, they should be respected here
-            # https://github.com/narwhals-dev/narwhals/issues/2790
+            if inputs.order_by:
+                msg = "`rank` followed by `over` with `order_by` specified is not supported for SQL-like backends."
+                raise NotImplementedError(msg)
             return [
-                _rank(
-                    expr,
-                    inputs.partition_by,
-                    inputs.order_by,
-                    descending=[descending] + [False] * len(inputs.order_by),
-                    nulls_last=[True] + [False] * len(inputs.order_by),
-                )
+                _rank(expr, inputs.partition_by, descending=descending)
                 for expr in self(df)
             ]
 

tests/expr_and_series/rank_test.py

@@ -340,19 +340,39 @@ def test_rank_with_order_by(
     if "duckdb" in str(constructor) and DUCKDB_VERSION < (1, 3):
         pytest.skip(reason="too old version")
 
+    context = (
+        pytest.raises(NotImplementedError)
+        if any(x in str(constructor) for x in ("pyspark", "duckdb", "ibis"))
+        else does_not_raise()
+    )
+
     df = nw.from_native(
         constructor(
             {"a": [1, 1, 2, 2, 3, 3], "b": [3, None, 4, 3, 5, 6], "i": list(range(6))}
         )
     )
-    result = df.with_columns(c=nw.col("a").rank("ordinal").over(order_by="b")).sort("i")
-    expected = {
-        "a": [1, 1, 2, 2, 3, 3],
-        "b": [3, None, 4, 3, 5, 6],
-        "i": [0, 1, 2, 3, 4, 5],
-        "c": [2, 1, 4, 3, 5, 6],
-    }
-    assert_equal_data(result, expected)
+    with context:
+        result = df.with_columns(c=nw.col("a").rank("ordinal").over(order_by="b")).sort(
+            "i"
+        )
+        expected = {
+            "a": [1, 1, 2, 2, 3, 3],
+            "b": [3, None, 4, 3, 5, 6],
+            "i": [0, 1, 2, 3, 4, 5],
+            "c": [2, 1, 4, 3, 5, 6],
+        }
+        assert_equal_data(result, expected)
+
+    with context:
+        # gh 3177
+        df = nw.from_native(constructor({"i": [0, 1, 2], "j": [1, 2, 1]}))
+        result = (
+            df.with_columns(z=nw.col("j").rank("min").over(order_by="i"))
+            .sort("i")
+            .select("z")
+        )
+        expected = {"z": [1.0, 3.0, 1.0]}
+        assert_equal_data(result, expected)
 
 
 def test_rank_with_order_by_and_partition_by(
@@ -386,14 +406,20 @@ def test_rank_with_order_by_and_partition_by(
             }
         )
     )
-    result = df.with_columns(c=nw.col("a").rank("ordinal").over("g", order_by="b")).sort(
-        "i"
+    context = (
+        pytest.raises(NotImplementedError)
+        if any(x in str(constructor) for x in ("pyspark", "duckdb", "ibis"))
+        else does_not_raise()
     )
-    expected = {
-        "a": [1, 1, 2, 2, 3, 3],
-        "b": [3, None, 4, 3, 5, 6],
-        "i": [0, 1, 2, 3, 4, 5],
-        "g": ["x", "x", "x", "y", "y", "y"],
-        "c": [2, 1, 3, 1, 2, 3],
-    }
-    assert_equal_data(result, expected)
+    with context:
+        result = df.with_columns(
+            c=nw.col("a").rank("ordinal").over("g", order_by="b")
+        ).sort("i")
+        expected = {
+            "a": [1, 1, 2, 2, 3, 3],
+            "b": [3, None, 4, 3, 5, 6],
+            "i": [0, 1, 2, 3, 4, 5],
+            "g": ["x", "x", "x", "y", "y", "y"],
+            "c": [2, 1, 3, 1, 2, 3],
+        }
+        assert_equal_data(result, expected)