feat: modify rank so that it works with over() for lazy backends
#2533
Conversation
|
really nice, thanks! i'm pleasantly surprised that for ibis it seems to just work without requiring adjustments on our end, nice |
narwhals/_spark_like/expr.py
Outdated
| def _rank(_input: Column) -> Column: | ||
| order_by = self._sort(_input, descending=descending, nulls_last=True) | ||
| window = self.partition_by().orderBy(*order_by) | ||
| count_window = self.partition_by(_input) | ||
| def _rank(_input: Column, window: WindowSpec, count_window: WindowSpec) -> Column: |
There was a problem hiding this comment.
I think the way you've done it in _duckdb is nice, where you share order_by between the two functions
Could we do that here too, so that the signatures are aligned?
There was a problem hiding this comment.
Thank you @MarcoGorelli ! Yes makes sense, I aligned them now
…to feat/rank-over-lazy-backends
narwhals/_duckdb/expr.py
Outdated
| *, | ||
| descending: bool, | ||
| partition_by_sql: str | None = None, | ||
| ) -> duckdb.Expression: | ||
| if descending: | ||
| by_sql = f"{_input} desc nulls last" | ||
| else: | ||
| by_sql = f"{_input} asc nulls last" | ||
| order_by_sql = f"order by {by_sql}" |
There was a problem hiding this comment.
it might be clearer for this order_by_sql to go in the else block (line 784)?
narwhals/_duckdb/expr.py
Outdated
| @@ -63,6 +65,7 @@ class DuckDBExpr(LazyExpr["DuckDBLazyFrame", "duckdb.Expression"]): | |||
| def __init__( | |||
| self, | |||
| call: EvalSeries[DuckDBLazyFrame, duckdb.Expression], | |||
| previous_call: EvalSeries[DuckDBLazyFrame, duckdb.Expression] | None = None, | |||
There was a problem hiding this comment.
does this need to be an argument to DuckDBExpr? I think it can be removed, as it's set in _with_unorderable_window_function
narwhals/_spark_like/expr.py
Outdated
| @@ -78,6 +80,7 @@ class SparkLikeExpr(LazyExpr["SparkLikeLazyFrame", "Column"]): | |||
| def __init__( | |||
| self, | |||
| call: EvalSeries[SparkLikeLazyFrame, Column], | |||
| previous_call: EvalSeries[SparkLikeLazyFrame, Column] | None = None, | |||
There was a problem hiding this comment.
thanks @raisadz - is_unique followed by over still needs doing, just flagging it in case it interests you to take a look at that too?
What type of PR is this? (check all applicable)
Related issues
Checklist
If you have comments or can explain your changes, please do so below