feat: unify exception for nested ``over`` statements, nested aggregations, filtrations on aggregations, document expression metadata better by MarcoGorelli · Pull Request #2351 · narwhals-dev/narwhals

MarcoGorelli · 2025-04-05T17:39:18Z

What type of PR is this? (check all applicable)

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

narwhals/_expression_parsing.py

dangotbanned · 2025-04-06T12:58:52Z

utils/check_api_reference.py

 expr_methods = [
    i
-    for i in nw.Expr(lambda: 0, ExprMetadata.simple_selector()).__dir__()
+    for i in nw.Expr(lambda: 0, ExprMetadata.selector_single()).__dir__()


I'm not sure what the lambda: 0, part is for here - but pyright is shouting at all 4 of these

Argument of type "() -> Literal[0]" cannot be assigned to parameter "to_compliant_expr" of type "_ToCompliant" in function "__init__" Type "() -> Literal[0]" is not assignable to type "_ToCompliant" Function accepts too many positional parameters; expected 0 but received 1 Function return type "Literal[0]" is incompatible with type "CompliantExpr[Any, Any]" "Literal[0]" is incompatible with protocol "CompliantExpr[Any, Any]" "_implementation" is not present "_backend_version" is not present "_version" is not present "_evaluate_output_names" is not present ...

this is just to get the list of methods which exist of Expr, it's an internal script

narwhals/expr.py

narwhals/_expression_parsing.py

MarcoGorelli · 2025-04-07T07:45:58Z

~~This can be improved, marking as draft for now~~ done

narwhals/_expression_parsing.py

dangotbanned · 2025-04-08T10:46:24Z

@MarcoGorelli @FBruzzesi

Is there overlap with the rules defined here and what we plan to allow in #2325?

I was thinking about (#2225 (comment)) again and just wanted to double-check things are aligned (if they need to be)

MarcoGorelli · 2025-04-08T16:13:43Z

Is there overlap with the rules defined here and what we plan to allow in #2325?

somewhat - in #2325, we should only allow TRANSFORM expressions where window_kind is NONE as arguments to group_by

dangotbanned

Really great stuff @MarcoGorelli 🎉

Left a few notes/suggestions - also maybe consider renaming the PR to something shorter 😉

dangotbanned · 2025-04-08T20:51:35Z

docs/how_it_works.md

+print(nw.col("a").mean().over("b"))
+```
+
+Note how they tell us something but their metadata. This section is all about


Note how they tell us something but their metadata

Is this a typo?

dangotbanned · 2025-04-08T20:54:06Z

docs/how_it_works.md

+   aggregate (e.g. `nw.col('a').drop_nulls()`).
+- `FILTRATION`: expressions which change length but don't


Suggested change

aggregate (e.g. `nw.col('a').drop_nulls()`).

- `FILTRATION`: expressions which change length but don't

- `FILTRATION`: expressions which change length but don't

aggregate (e.g. `nw.col('a').drop_nulls()`).

dangotbanned · 2025-04-08T21:01:43Z

docs/how_it_works.md

+  - `nw.col('a')` is not order-dependent, so it's `TRANSFORM`.
+  - `nw.col('a').abs()` is not order-dependent, so it's a `TRANSFORM`.
+  - `nw.col('a').cum_sum()`'s last operation is `cum_sum`, so it's `WINDOW`.
+  - `nw.col('a').cum_sum()+1`'s last operation is `__add__`, and it preserves


Suggested change

- `nw.col('a').cum_sum()+1`'s last operation is `__add__`, and it preserves

- `nw.col('a').cum_sum() + 1`'s last operation is `__add__`, and it preserves

dangotbanned · 2025-04-08T21:07:28Z

docs/how_it_works.md

+How these change depends on the operation.
+
+#### Chaining
+
+Say we have `expr.expr_method()`. How does `expr`'s `ExprMetadata` change?
+This depends on `expr_method`.


I'm not quite sure what to suggest, but some notes

Maybe the sentence before Chaining is redundant?

After Chaining I think there's a few too many expr/Expr 😵

dangotbanned · 2025-04-08T21:11:46Z

docs/how_it_works.md

+- `drop_nulls` and `filter` result in `FILTRATION`, and can only be applied
+  to `TRANSFORM` and `WINDOW`.
+- `over` always results in `TRANSFORM`. This is a bit more complicated,
+  so we elaborate on it in the "You open a window" section below.


Suggested change

so we elaborate on it in the "You open a window" section below.

so we elaborate on it in ["You open a window ..."](#you-open-a-window-to-another-window-to-another-window-to-another-window).

dangotbanned · 2025-04-08T21:17:32Z

docs/how_it_works.md

+#### Binary operations (e.g. `nw.col('a') + nw.col('b')`)
+
+How do expression kinds change under binary operations? For example,
+if we do `expr1 + expr2`, then what can we say about the output kind?
+The rules are:
+
+- If both are `LITERAL`, then the output is `LITERAL`.
+- If one is a `FILTRATION`, then:
+
+    - if the other is `LITERAL` or `AGGREGATION`, then the output is `FILTRATION`.
+    - else, we raise an error.
+
+- If one is `TRANSFORM` or `WINDOW` and the other is not `FILTRATION`,
+  then the output is `TRANSFORM`.
+- If one is `AGGREGATION` and the other is `LITERAL` or `AGGREGATION`,
+  the output is `AGGREGATION`.
+
+For n-ary operations such as `nw.sum_horizontal`, the above logic is
+extended across inputs. For example, `nw.sum_horizontal(expr1, expr2, expr3)`
+is `LITERAL` if all of `expr1`, `expr2`, and `expr3` are.


Don't worry about it for this PR, but I feel like this could really benefit from some kind of visual aid.

Not sure what format would make the most sense - just wanted to note it as tough to keep track of in my head 😔

dangotbanned · 2025-04-08T21:18:07Z

docs/how_it_works.md

+### "You open a window to another window to another window to another window"
+
+When we print out an expression, in addition to the expression kind,
+we also see `window_kind`. These are four window kinds:


Suggested change

we also see `window_kind`. These are four window kinds:

we also see `window_kind`. There are four window kinds:

dangotbanned · 2025-04-08T21:20:09Z

narwhals/_expression_parsing.py

    - FILTRATION vs (LITERAL | AGGREGATION) -> FILTRATION
    - FILTRATION vs (FILTRATION | TRANSFORM | WINDOW) -> raise
-    - (TRANSFORM | WINDOW) vs (LITERAL | AGGREGATION) -> TRANSFORM
+    - (TRANSFORM | WINDOW) vs (anything) -> TRANSFORM


Suggested change

- (TRANSFORM | WINDOW) vs (anything) -> TRANSFORM

- (TRANSFORM | WINDOW) vs (...) -> TRANSFORM

MarcoGorelli · 2025-04-09T15:18:36Z

thanks for your review! really appreciate it

These have been popping up and bugging me for 2 weeks 🙈 #2351 (comment)

* chore(typing): fix `check_docstrings` `Path` warning `pyright` did a good job spotting that one * chore: Use `dir(...)` instead of `.__dir__()` Fixes 2/6 errors and shrinks the script * chore(typing): Fix 2 more errors These have been popping up and bugging me for 2 weeks 🙈 #2351 (comment) * chore(typing): Fix the last 2 as well * chore: Simplify the rest * chore(typing): Ignore remaining in `utils` * chore(typing): Ignore everything in `tpch` These are all the same issue, can be fixed with a constrained `TypeVar` * fix: pre-`3.13` compat? Following where `__name__` used to be sourced from - python/cpython#101876 * chore(typing): Fix some of the new `tpch` ignores The rest seem pretty unavoidable * fix: don't clobber `typing` 🤦‍♂️ https://github.com/narwhals-dev/narwhals/actions/runs/14624570842/job/41032838053?pr=2426

feat: unify exception for nested over statements

8186f0f

MarcoGorelli force-pushed the no-double-over branch from eb6ec10 to 8186f0f Compare April 5, 2025 17:49

dangotbanned reviewed Apr 5, 2025

View reviewed changes

narwhals/_expression_parsing.py Show resolved Hide resolved

MarcoGorelli marked this pull request as ready for review April 5, 2025 21:39

rename exprmetadata selector staticmethods

ac74e15

dangotbanned reviewed Apr 6, 2025

View reviewed changes

narwhals/_expression_parsing.py Outdated Show resolved Hide resolved

MarcoGorelli added 2 commits April 6, 2025 11:20

docs fixups

f1bcaa4

simple -> single

5b05a9a

MarcoGorelli added documentation Improvements or additions to documentation enhancement New feature or request error reporting labels Apr 6, 2025

dangotbanned reviewed Apr 6, 2025

View reviewed changes

narwhals/expr.py Outdated Show resolved Hide resolved

dangotbanned reviewed Apr 6, 2025

View reviewed changes

narwhals/expr.py Outdated Show resolved Hide resolved

dangotbanned reviewed Apr 6, 2025

View reviewed changes

narwhals/_expression_parsing.py Outdated Show resolved Hide resolved

dangotbanned reviewed Apr 6, 2025

View reviewed changes

narwhals/_expression_parsing.py Outdated Show resolved Hide resolved

dangotbanned reviewed Apr 6, 2025

View reviewed changes

narwhals/_expression_parsing.py Outdated Show resolved Hide resolved

MarcoGorelli added 2 commits April 6, 2025 20:12

n_closed_windows => has_windows

902a590

docs

d44e7c5

MarcoGorelli force-pushed the no-double-over branch from 11a6bbc to d44e7c5 Compare April 6, 2025 19:27

MarcoGorelli added 2 commits April 6, 2025 20:32

fixup

fb11483

simplify

a45284e

MarcoGorelli marked this pull request as draft April 7, 2025 07:46

MarcoGorelli added 2 commits April 7, 2025 11:03

it can always be done simpler!

74b0de1

coverage

6a138c6

MarcoGorelli marked this pull request as ready for review April 7, 2025 10:55

dangotbanned reviewed Apr 7, 2025

View reviewed changes

narwhals/_expression_parsing.py Show resolved Hide resolved

dangotbanned reviewed Apr 7, 2025

View reviewed changes

narwhals/_expression_parsing.py Outdated Show resolved Hide resolved

dangotbanned reviewed Apr 7, 2025

View reviewed changes

narwhals/_expression_parsing.py Outdated Show resolved Hide resolved

dangotbanned reviewed Apr 7, 2025

View reviewed changes

narwhals/_expression_parsing.py Show resolved Hide resolved

MarcoGorelli added 2 commits April 7, 2025 20:14

update outdated "open window" refs

bc87e6b

update outdated "open window" refs

770490a

MarcoGorelli marked this pull request as draft April 7, 2025 21:59

MarcoGorelli added 2 commits April 8, 2025 17:04

Merge remote-tracking branch 'upstream/main' into no-double-over

c8db7a1

document effect of UNCLOSEABLE window

f3bf2b9

MarcoGorelli marked this pull request as ready for review April 8, 2025 16:41

dangotbanned self-requested a review April 8, 2025 17:10

dangotbanned approved these changes Apr 8, 2025

View reviewed changes

MarcoGorelli added 2 commits April 9, 2025 16:05

Merge remote-tracking branch 'upstream/main' into no-double-over

234f874

typos (thanks Dan!)

5481f9a

MarcoGorelli merged commit 16b4527 into narwhals-dev:main Apr 9, 2025
29 checks passed

dangotbanned added a commit that referenced this pull request Apr 23, 2025

chore(typing): Fix 2 more errors

ace3213

These have been popping up and bugging me for 2 weeks 🙈 #2351 (comment)

dangotbanned mentioned this pull request Apr 23, 2025

chore(typing): fix/ignore utils, tpch warnings #2426

Merged

10 tasks

		aggregate (e.g. `nw.col('a').drop_nulls()`).
		- `FILTRATION`: expressions which change length but don't

	- `nw.col('a').cum_sum()+1`'s last operation is `__add__`, and it preserves
	- `nw.col('a').cum_sum() + 1`'s last operation is `__add__`, and it preserves

	so we elaborate on it in the "You open a window" section below.
	so we elaborate on it in ["You open a window ..."](#you-open-a-window-to-another-window-to-another-window-to-another-window).

	we also see `window_kind`. These are four window kinds:
	we also see `window_kind`. There are four window kinds:

	- (TRANSFORM \| WINDOW) vs (anything) -> TRANSFORM
	- (TRANSFORM \| WINDOW) vs (...) -> TRANSFORM

Conversation

MarcoGorelli commented Apr 5, 2025 • edited by dangotbanned Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MarcoGorelli commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dangotbanned commented Apr 8, 2025

Uh oh!

MarcoGorelli commented Apr 8, 2025

Uh oh!

dangotbanned left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoGorelli commented Apr 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MarcoGorelli commented Apr 5, 2025 •

edited by dangotbanned

Loading

MarcoGorelli commented Apr 7, 2025 •

edited

Loading