feat: support window operations for DuckDB by MarcoGorelli · Pull Request #2263 · narwhals-dev/narwhals

MarcoGorelli · 2025-03-21T18:46:14Z

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

FBruzzesi

@MarcoGorelli this was an amazing streaming! I left two nitpick comments and a suggestion for whoever wants to have some fun working on #2174 🥦

FBruzzesi · 2025-03-21T21:38:23Z

narwhals/_duckdb/expr.py

 from typing import Sequence
 from typing import cast

+import duckdb


Should we follow the same pattern as below and directly import from duckdb import SQLExpression?

FBruzzesi · 2025-03-21T21:42:05Z

narwhals/_duckdb/expr.py

+            if reverse:
+                order_by_sql = "order by " + ", ".join(
+                    f'"{x}" desc nulls last' for x in order_by
+                )
+            else:
+                order_by_sql = "order by " + ", ".join(
+                    f'"{x}" asc nulls first' for x in order_by
+                )
+            if partition_by:
+                partition_by_sql = "partition by " + ",".join(
+                    f'"{x}"' for x in partition_by
+                )
+            else:
+                partition_by_sql = ""
+            sql = f"sum ({_input}) over ({partition_by_sql} {order_by_sql} rows between unbounded preceding and current row)"
+            return duckdb.SQLExpression(sql)


I can see a lot of re-usability if everything here for #2174 - only "word" to input is "sum" and other operations 🙌🏼

yup - let's generalise when we add more of them

FBruzzesi · 2025-03-21T21:43:12Z

.github/workflows/pytest.yml

+      - name: install duckdb nightly
+        run: uv pip install -U --pre duckdb --system


Should we wait for the duckdb release before merging this PR? You are ahead of them 😂

nah let's be one step ahead so when they release we're ready 🔥

dangotbanned · 2025-03-22T10:59:15Z

narwhals/_duckdb/expr.py

+        ) -> duckdb.Expression:
+            if reverse:
+                order_by_sql = "order by " + ", ".join(
+                    f'"{x}" desc nulls last' for x in order_by


@MarcoGorelli I commented this on the stream, not sure it was ever tried out, anyways now in context 🙂

Suggested change

f'"{x}" desc nulls last' for x in order_by

f'{x!r} desc nulls last' for x in order_by

https://docs.python.org/3/reference/lexical_analysis.html#formatted-string-literals

i don't think that's the same, we need double quotes around the column names as that's what sql expects (single quotes are string literals)

i don't think that's the same, we need double quotes around the column names as that's what sql expects (single quotes are string literals)

Does it not respect the outer quotes?

I would've thought these two used different internal ones:

f'{x!r} desc nulls last' for x in order_by f"{x!r} desc nulls last" for x in order_by

dangotbanned · 2025-03-22T11:05:35Z

narwhals/_duckdb/expr.py

+        if self._backend_version < (1, 3):
+            msg = "At least version 1.3 of DuckDB is required for `over` operation."
+            raise NotImplementedError(msg)
+        if (window_function := self._window_function) is not None:


AFAIK, you only need the is not None when the type overrides __bool__.

Going to guess this is from experience battling pandas, polars, and (maybe) numpy who all tell you off for using __bool__

Suggested change

if (window_function := self._window_function) is not None:

if (window_function := self._window_function):

dangotbanned · 2025-03-22T11:14:17Z

narwhals/_duckdb/typing.py

+    class WindowFunction(Protocol):
+        def __call__(
+            self,
+            _input: duckdb.Expression,
+            partition_by: Sequence[str],
+            order_by: Sequence[str],
+        ) -> duckdb.Expression: ...


I've been meaning to ask about this for a while now.

Is that naming convention intended to signal positional-only for _input?

It seems similar to a convention that was common before PEP 570 – Python Positional-Only Parameters.

Current

class WindowFunction(Protocol): def __call__( self, _input: duckdb.Expression, partition_by: Sequence[str], order_by: Sequence[str], ) -> duckdb.Expression: ...

Before PEP 570

class WindowFunction(Protocol): def __call__( self, __input: duckdb.Expression, partition_by: Sequence[str], order_by: Sequence[str], ) -> duckdb.Expression: ...

After PEP 570

class WindowFunction(Protocol): def __call__( self, input: duckdb.Expression, /, partition_by: Sequence[str], order_by: Sequence[str], ) -> duckdb.Expression: ...

yeah _input wasn't a very good name, i don't think i'd put much thought into it at the time and now it's all over

we could use native_expr in its place?

we could use native_expr in its place?

Whatever you feel works 👍

Side note

One of the cool things about positional-only args is that you can use different names - and it still works the same at runtime and to a type checker.
Was a bit of a 🤯 when I learned that.

Here, that might mean you could use expr when it is unambiguous; but native_expr when either compliant_expr or native_expr could be confused

MarcoGorelli · 2025-03-22T13:23:13Z

thanks all for your reviews!

dangotbanned · 2025-03-23T12:24:41Z

narwhals/_duckdb/expr.py

+with contextlib.suppress(ImportError):  # requires duckdb>=1.3.0
+    from duckdb import SQLExpression
+


@MarcoGorelli this is still causing typing issues outside of CI on a fresh install.

Fix 1

I've been using this locally, but is more of a workaround:

diff --git a/narwhals/_duckdb/expr.py b/narwhals/_duckdb/expr.py index fd371b73..0ec4681d 100644 --- a/narwhals/_duckdb/expr.py +++ b/narwhals/_duckdb/expr.py @@ -41,8 +41,11 @@ if TYPE_CHECKING: from narwhals.utils import Version from narwhals.utils import _FullContext -with contextlib.suppress(ImportError): # requires duckdb>=1.3.0 - from duckdb import SQLExpression +if not TYPE_CHECKING: + with contextlib.suppress(ImportError): # requires duckdb>=1.3.0 + from duckdb import SQLExpression +else: + from duckdb import Expression as SQLExpression class DuckDBExpr(LazyExpr["DuckDBLazyFrame", "duckdb.Expression"]):

Fix 2

Specifying this requirement in pyproject.toml somewhere.
Not sure on exactly how though

narwhals/Makefile

Lines 22 to 24 in de9f375

typing: ## Run typing checks

# install duckdb nightly so mypy recognises duckdb.SQLExpression

$(VENV_BIN)/uv pip install -U --pre duckdb

narwhals/.github/workflows/extremes.yml

Line 185 in de9f375

uv pip install -U --pre duckdb --system

dangotbanned · 2025-03-23T12:29:50Z

tests/expr_and_series/str/to_datetime_test.py

-    if "sqlframe" in str(constructor):
-        # https://github.com/eakmanrq/sqlframe/issues/325
-        request.applymarker(pytest.mark.xfail)


@MarcoGorelli I'm also needing this since the PR merged 🤔

The comment is the error that started showing up for me, but not in CI

diff --git a/tests/expr_and_series/str/to_datetime_test.py b/tests/expr_and_series/str/to_datetime_test.py index 412485d0..4ed208f6 100644 --- a/tests/expr_and_series/str/to_datetime_test.py +++ b/tests/expr_and_series/str/to_datetime_test.py @@ -14,6 +14,7 @@ from tests.utils import PANDAS_VERSION from tests.utils import PYARROW_VERSION from tests.utils import assert_equal_data from tests.utils import is_pyarrow_windows_no_tzdata +from tests.utils import is_windows if TYPE_CHECKING: from tests.utils import Constructor @@ -219,6 +220,15 @@ def test_to_datetime_tz_aware( if "cudf" in str(constructor): # cuDF does not yet support timezone-aware datetimes request.applymarker(pytest.mark.xfail) + if "sqlframe" in str(constructor) and format is not None and is_windows(): + # + # E duckdb.duckdb.InvalidInputException: Invalid Input Error: + # Could not parse string "2020-01-01 01:02:03+0100" according to format specifier "%Y-%-m-%-d %-H:%-M:%-SZ" + # E 2020-01-01 01:02:03+0100 + # E ^ + # E Error: Literal does not match, expected Z + # + request.applymarker(pytest.mark.xfail) context = ( pytest.raises(NotImplementedError) if any(x in str(constructor) for x in ("duckdb",)) and format is None

- narwhals-dev#2263 (comment) - narwhals-dev#2263 (comment)

Was broken for me locally following narwhals-dev#2263 (comment)

MarcoGorelli added 4 commits March 21, 2025 15:41

wip

d6fd44a

support cum_sum too!

1655fbc

raise on lower versions

8c36db7

feat: support window operations for DuckDB

ba5b646

MarcoGorelli force-pushed the duckdb-over branch from 56a7eb5 to ba5b646 Compare March 21, 2025 18:46

use duckdb nightly in full coverage job

8235348

MarcoGorelli force-pushed the duckdb-over branch from bc4da37 to 8235348 Compare March 21, 2025 20:10

ci

f2cc84d

MarcoGorelli force-pushed the duckdb-over branch from f16970d to f2cc84d Compare March 21, 2025 20:21

MarcoGorelli added 2 commits March 21, 2025 20:26

ci fixup

06de928

lint

35e603c

MarcoGorelli force-pushed the duckdb-over branch from 314fa79 to 35e603c Compare March 21, 2025 20:28

MarcoGorelli added the enhancement New feature or request label Mar 21, 2025

missing file

38a4710

FBruzzesi reviewed Mar 21, 2025

View reviewed changes

MarcoGorelli and others added 7 commits March 22, 2025 09:16

Merge branch 'main' into duckdb-over

07872ca

import sqlexpression

1c08990

post-merge fixup

eaf0172

extra quotes for safety

6485303

extra quotes for safety

5e50682

old python compat

65ae7cb

old python compat

bc515af

dangotbanned reviewed Mar 22, 2025

View reviewed changes

make test have empty space

eecff03

MarcoGorelli marked this pull request as ready for review March 22, 2025 12:41

MarcoGorelli added 3 commits March 22, 2025 12:52

factor out generate_order_by_sql

f1a50e8

remove outdated sqlframe xfail

231f1f3

again

6fb1b76

coverage

de9f375

MarcoGorelli merged commit 295ee89 into main Mar 22, 2025
27 of 28 checks passed

MarcoGorelli deleted the duckdb-over branch March 22, 2025 13:25

dangotbanned reviewed Mar 23, 2025

View reviewed changes

dangotbanned added a commit to MarcoGorelli/narwhals that referenced this pull request Mar 23, 2025

chore(typing): Ignore issues from narwhals-dev#2263

2d4dd8a

- narwhals-dev#2263 (comment) - narwhals-dev#2263 (comment)

dangotbanned added a commit to MarcoGorelli/narwhals that referenced this pull request Mar 23, 2025

test: Kinda unbreak sqlframe test

b5b1c92

Was broken for me locally following narwhals-dev#2263 (comment)

		- name: install duckdb nightly
		run: uv pip install -U --pre duckdb --system

	f'"{x}" desc nulls last' for x in order_by
	f'{x!r} desc nulls last' for x in order_by

	if (window_function := self._window_function) is not None:
	if (window_function := self._window_function):

		with contextlib.suppress(ImportError): # requires duckdb>=1.3.0
		from duckdb import SQLExpression

	typing: ## Run typing checks
	# install duckdb nightly so mypy recognises duckdb.SQLExpression
	$(VENV_BIN)/uv pip install -U --pre duckdb

Conversation

MarcoGorelli commented Mar 21, 2025

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!

FBruzzesi 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!

dangotbanned Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Current

Before PEP 570

After PEP 570

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dangotbanned Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Side note

Uh oh!

MarcoGorelli commented Mar 22, 2025

Uh oh!

Uh oh!

dangotbanned Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Fix 1

Fix 2

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dangotbanned Mar 22, 2025 •

edited

Loading

dangotbanned Mar 22, 2025 •

edited

Loading

dangotbanned Mar 23, 2025 •

edited

Loading