Skip to content

Commit 75a6489

Browse files
committed
review fixes
1 parent a875747 commit 75a6489

21 files changed

Lines changed: 631 additions & 74 deletions

CLAUDE.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ This is the **production** duckdb-python client — the `duckdb` package on PyPI
1010
- **Package name**: `duckdb`
1111
- **Bindings**: pybind11
1212
- **Build backend**: `duckdb_packaging.build_backend` (custom wrapper around scikit-build-core)
13-
- **Supported Python**: 3.10, 3.11, 3.12, 3.13, 3.14
13+
- **Supported Python**: 3.11, 3.12, 3.13, 3.14
1414
- **Free-threaded Python**: not supported in this client. A separate prototype client based on DuckDB's C API targets free-threading, Stable ABI, and multi-interpreter support.
1515

1616
## IMPORTANT: build before running anything
@@ -115,7 +115,7 @@ uv sync --no-build-isolation -v --reinstall -p 3.11
115115
uv sync --no-build-isolation -v --reinstall -p 3.14
116116
```
117117

118-
Supported: `3.10`, `3.11`, `3.12`, `3.13`, `3.14`. Do **not** use free-threaded variants (`3.13t`, `3.14t`) — the production client does not support them.
118+
Supported: `3.11`, `3.12`, `3.13`, `3.14`. Do **not** use free-threaded variants (`3.13t`, `3.14t`) — the production client does not support them.
119119

120120
### Build configuration reference
121121

pyproject.toml

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ dynamic = ["version"]
1010
description = "DuckDB in-process database"
1111
readme = "README.md"
1212
keywords = ["DuckDB", "Database", "SQL", "OLAP"]
13-
requires-python = ">=3.10.0"
13+
requires-python = ">=3.11.0"
1414
classifiers = [
1515
"Development Status :: 5 - Production/Stable",
1616
"License :: OSI Approved :: MIT License",
@@ -25,7 +25,6 @@ classifiers = [
2525
"Programming Language :: Python",
2626
"Programming Language :: Python :: 3",
2727
"Programming Language :: Python :: 3 :: Only",
28-
"Programming Language :: Python :: 3.10",
2928
"Programming Language :: Python :: 3.11",
3029
"Programming Language :: Python :: 3.12",
3130
"Programming Language :: Python :: 3.13",
@@ -480,7 +479,7 @@ before-build = ["yum install -y ccache"]
480479
[tool.cibuildwheel.macos]
481480
before-build = ["brew install ccache"]
482481
# nanobind uses C++17 aligned new/delete (std::align_val_t), which the runtime only provides on macOS 10.13+.
483-
# cp310/cp311's framework defaults to a 10.9 deployment target (used for the x86_64 slice of x86_64/universal2
482+
# cp311's framework defaults to a 10.9 deployment target (used for the x86_64 slice of x86_64/universal2
484483
# wheels), so nanobind fails to compile there; cp312+ frameworks already target 10.13+. Pin 10.14 so every CPython
485484
# version builds (arm64 slices are 11.0 regardless).
486485
environment = { MACOSX_DEPLOYMENT_TARGET = "10.14" }

src/arrow/filter_pushdown_visitor.cpp

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,24 @@ nb::object EmitCompare(FilterBackend &backend, ExpressionType op, nb::object col
6161
return backend.NaNCompare(op, std::move(col));
6262
}
6363
auto scalar = backend.MakeScalar(constant, arrow_type, timezone_config);
64+
// DuckDB orders NaN as the greatest float value, so `nan > finite` and `nan >= finite` are TRUE, while
65+
// IEEE (pyarrow) makes them FALSE. For a finite FLOAT/DOUBLE constant with `>` / `>=`, the plain
66+
// comparison would silently drop NaN column rows the engine keeps (the arrow scan never re-applies
67+
// pushed filters). OR the NaN rows back in so the pushed filter matches DuckDB semantics. `<`, `<=`,
68+
// `=`, `<>` already agree with IEEE, so they are left unchanged. (Idempotent for the polars backend,
69+
// which already treats NaN as greatest, and only reached for float constants so is_nan is always valid.)
70+
// N3: keying is_nan on the CONSTANT's float-ness is safe -- a float constant here implies a float column
71+
// (int/float comparisons are constant-folded to int bounds or wrapped in a non-pushed CAST upstream), so
72+
// col.is_nan() always resolves to a valid pyarrow kernel.
73+
const auto constant_type_id = constant.type().id();
74+
const bool constant_is_float =
75+
constant_type_id == LogicalTypeId::FLOAT || constant_type_id == LogicalTypeId::DOUBLE;
76+
if (constant_is_float &&
77+
(op == ExpressionType::COMPARE_GREATERTHAN || op == ExpressionType::COMPARE_GREATERTHANOREQUALTO)) {
78+
auto compare = backend.Compare(op, col, std::move(scalar));
79+
auto is_nan = backend.IsNaN(std::move(col));
80+
return backend.Or(std::move(compare), std::move(is_nan));
81+
}
6482
return backend.Compare(op, std::move(col), std::move(scalar));
6583
}
6684

src/arrow/polars_filter_pushdown.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,10 @@ struct PolarsBackend : public FilterBackend {
7171
}
7272
}
7373

74+
nb::object IsNaN(nb::object col) override {
75+
return col.attr("is_nan")();
76+
}
77+
7478
nb::object IsNull(nb::object col) override {
7579
return col.attr("is_null")();
7680
}

src/arrow/pyarrow_filter_pushdown.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,10 @@ struct PyArrowBackend : public FilterBackend {
243243
}
244244
}
245245

246+
nb::object IsNaN(nb::object col) override {
247+
return col.attr("is_nan")();
248+
}
249+
246250
nb::object IsNull(nb::object col) override {
247251
return col.attr("is_null")();
248252
}

src/duckdb_python.cpp

Lines changed: 46 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -708,33 +708,36 @@ static void InitializeConnectionMethods(nb::module_ &m) {
708708
"run the query as-is.",
709709
nb::arg("query"), nb::kw_only(), nb::arg("alias") = "", nb::arg("params") = nb::none(),
710710
nb::arg("connection").none() = nb::none());
711-
m.def(
712-
"read_csv",
713-
// nb::arg + nb::kwargs can't coexist under nanobind's annotation rules; drop the annotations.
714-
[](const nb::object &name, nb::kwargs &kwargs) {
715-
std::shared_ptr<DuckDBPyConnection> conn;
716-
if (kwargs.contains("conn") && !kwargs["conn"].is_none()) {
717-
conn = nb::cast<std::shared_ptr<DuckDBPyConnection>>(kwargs["conn"]);
718-
}
719-
if (!conn) {
720-
conn = DuckDBPyConnection::DefaultConnection();
721-
}
722-
return conn->ReadCSV(name, kwargs);
723-
},
724-
"Create a relation object from the CSV file in 'name'");
725-
m.def(
726-
"from_csv_auto",
727-
[](const nb::object &name, nb::kwargs &kwargs) {
728-
std::shared_ptr<DuckDBPyConnection> conn;
729-
if (kwargs.contains("conn") && !kwargs["conn"].is_none()) {
730-
conn = nb::cast<std::shared_ptr<DuckDBPyConnection>>(kwargs["conn"]);
731-
}
732-
if (!conn) {
733-
conn = DuckDBPyConnection::DefaultConnection();
734-
}
735-
return conn->ReadCSV(name, kwargs);
736-
},
737-
"Create a relation object from the CSV file in 'name'");
711+
// nanobind's all-or-nothing nb::arg rule forbids naming just the source parameter alongside **kwargs, so the
712+
// module-level read_csv / from_csv_auto take (*args, **kwargs) and recover the advertised keywords by hand:
713+
// the source may be positional or passed as `path_or_buffer=`, and the connection as `connection=` / `conn=`.
714+
// Each recovered keyword is popped from kwargs so ReadCSV's unknown-parameter check only sees CSV options.
715+
// N2: extra positional args (e.g. read_csv("a", "b")) are silently dropped rather than raising; negligible.
716+
auto module_read_csv = [](nb::args args, nb::kwargs kwargs) {
717+
nb::object name = nb::none();
718+
if (args.size() >= 1) {
719+
name = nb::object(args[0]);
720+
} else if (kwargs.contains("path_or_buffer")) {
721+
name = kwargs["path_or_buffer"];
722+
PyDict_DelItemString(kwargs.ptr(), "path_or_buffer");
723+
}
724+
std::shared_ptr<DuckDBPyConnection> conn;
725+
for (const char *conn_key : {"connection", "conn"}) {
726+
if (kwargs.contains(conn_key)) {
727+
nb::object conn_arg = kwargs[conn_key];
728+
PyDict_DelItemString(kwargs.ptr(), conn_key);
729+
if (!conn && !conn_arg.is_none()) {
730+
conn = nb::cast<std::shared_ptr<DuckDBPyConnection>>(conn_arg);
731+
}
732+
}
733+
}
734+
if (!conn) {
735+
conn = DuckDBPyConnection::DefaultConnection();
736+
}
737+
return conn->ReadCSV(name, kwargs);
738+
};
739+
m.def("read_csv", module_read_csv, "Create a relation object from the CSV file in 'name'");
740+
m.def("from_csv_auto", module_read_csv, "Create a relation object from the CSV file in 'name'");
738741
m.def(
739742
"from_df",
740743
[](const PandasDataFrame &value, std::shared_ptr<DuckDBPyConnection> conn = nullptr) {
@@ -822,9 +825,21 @@ static void InitializeConnectionMethods(nb::module_ &m) {
822825
"Load an installed extension", nb::arg("extension"), nb::kw_only(), nb::arg("connection").none() = nb::none());
823826
m.def(
824827
"project",
825-
// nanobind forbids named typed parameters after nb::args; the keyword-only `groups` and `connection`
826-
// are therefore taken from **kwargs (preserving the previous defaults/None-handling).
827-
[](const PandasDataFrame &df, const nb::args &args, const nb::kwargs &kwargs) {
828+
// nanobind forbids named typed parameters after nb::args, so this takes (*args, **kwargs) and recovers the
829+
// advertised signature by hand: `df` may be positional (args[0]) or the `df=` keyword (the stubs advertise
830+
// it as positional-or-keyword); the remaining positionals are projection expressions; `groups` /
831+
// `connection` are keyword-only (pulled from kwargs, preserving the previous defaults/None-handling).
832+
[](const nb::args &args, const nb::kwargs &kwargs) {
833+
nb::object df_obj = nb::none();
834+
nb::args proj_args = nb::steal<nb::args>(PyTuple_New(0));
835+
if (args.size() >= 1) {
836+
df_obj = nb::object(args[0]);
837+
proj_args = nb::steal<nb::args>(PyTuple_GetSlice(args.ptr(), 1, static_cast<Py_ssize_t>(args.size())));
838+
} else if (kwargs.contains("df")) {
839+
df_obj = kwargs["df"];
840+
PyDict_DelItemString(kwargs.ptr(), "df");
841+
}
842+
auto df = nb::cast<PandasDataFrame>(df_obj);
828843
string groups = "";
829844
if (kwargs.contains("groups") && !kwargs["groups"].is_none()) {
830845
groups = nb::cast<std::string>(kwargs["groups"]);
@@ -836,7 +851,7 @@ static void InitializeConnectionMethods(nb::module_ &m) {
836851
if (!conn) {
837852
conn = DuckDBPyConnection::DefaultConnection();
838853
}
839-
return conn->FromDF(df)->Project(args, groups);
854+
return conn->FromDF(df)->Project(proj_args, groups);
840855
},
841856
"Project the relation object by the projection in project_expr");
842857
m.def(

src/include/duckdb_python/arrow/filter_pushdown_visitor.hpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,11 @@ struct FilterBackend {
5555
// each operator decomposes into is_nan / ~is_nan / lit(true|false).
5656
virtual nb::object NaNCompare(ExpressionType op, nb::object col) = 0;
5757

58+
// Column-side NaN predicate: `col.is_nan()`. Used to re-include NaN rows for `>` / `>=` against a
59+
// finite float constant, since DuckDB orders NaN as the greatest value (so `nan > finite` is TRUE)
60+
// while IEEE comparisons make them FALSE.
61+
virtual nb::object IsNaN(nb::object col) = 0;
62+
5863
virtual nb::object IsNull(nb::object col) = 0;
5964
virtual nb::object IsNotNull(nb::object col) = 0;
6065

src/include/duckdb_python/filesystem_object.hpp

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,14 @@ class FileSystemObject : public RegisteredObject {
2222
// Assert that the 'obj' is a filesystem
2323
D_ASSERT(duckdb::PyUtil::IsInstance(
2424
obj, DuckDBPyConnection::ImportCache()->duckdb.filesystem.ModifiedMemoryFileSystem()));
25-
for (auto &file : filenames) {
26-
obj.attr("delete")(file);
25+
// Destructors are implicitly noexcept: a Python exception escaping here (fsspec `_rm` raises
26+
// KeyError for a missing entry) would std::terminate the process. Swallow it, mirroring
27+
// ~PythonFileHandle / ~PythonFilesystem.
28+
try {
29+
for (auto &file : filenames) {
30+
obj.attr("delete")(file);
31+
}
32+
} catch (...) { // NOLINT: intentional catch-all in a destructor
2733
}
2834
}
2935

0 commit comments

Comments
 (0)