From b7900c9297501aaf89bb5974a91de15931767f23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Furga=C5=82a?= <83299832+00200200@users.noreply.github.com> Date: Tue, 29 Sep 2026 14:48:16 +0200 Subject: [PATCH] Honor overwrite=False when writing an existing parquet or csv file. COPY replaces a single file via a tmp rename, so overwrite=False had no effect. Fail with the same IOException as partitioned writes instead of silently replacing the file. --- src/pyrelation.cpp | 41 ++++++++++++++++++++++--------- tests/fast/api/test_to_csv.py | 13 ++++++++++ tests/fast/api/test_to_parquet.py | 13 ++++++++++ 3 files changed, 55 insertions(+), 12 deletions(-) diff --git a/src/pyrelation.cpp b/src/pyrelation.cpp index 38ff3984..46053a2f 100644 --- a/src/pyrelation.cpp +++ b/src/pyrelation.cpp @@ -19,6 +19,7 @@ #include "duckdb_python/map.hpp" #include "duckdb_python/expression/pyexpression.hpp" #include "duckdb/common/arrow/physical_arrow_collector.hpp" +#include "duckdb/common/file_system.hpp" #include "duckdb_python/arrow/arrow_export_utils.hpp" namespace duckdb { @@ -1252,6 +1253,32 @@ static Value NestedDictToStruct(const nb::object &dictionary) { return Value::STRUCT(std::move(children)); } +static void ApplyOverwriteOption(identifier_map_t> &options, const string &function_name, + const string &filename, Relation &relation, const nb::object &overwrite) { + if (nb::none().is(overwrite)) { + return; + } + if (!nb::isinstance(overwrite)) { + throw InvalidInputException(function_name + " only accepts 'overwrite' as a boolean"); + } + const bool overwrite_val = static_cast(nb::bool_(overwrite)); + options["overwrite_or_ignore"] = {Value::BOOLEAN(overwrite_val)}; + if (overwrite_val) { + return; + } + // Single-file COPY replaces via a tmp rename, so overwrite_or_ignore=false is a no-op. + auto context = relation.context->TryGetContext(); + if (!context) { + throw InvalidInputException(function_name + " cannot run after the connection has been closed"); + } + auto &fs = FileSystem::GetFileSystem(*context); + if (fs.FileExists(filename)) { + throw IOException("Cannot write to \"%s\" - it exists and is a file, not a directory! Enable OVERWRITE option " + "to overwrite the file", + filename); + } +} + void DuckDBPyRelation::ToParquet(const string &filename, const nb::object &compression, const nb::object &field_ids, const nb::object &row_group_size_bytes, const nb::object &row_group_size, const nb::object &overwrite, const nb::object &per_thread_output, @@ -1327,12 +1354,7 @@ void DuckDBPyRelation::ToParquet(const string &filename, const nb::object &compr options["append"] = {Value::BOOLEAN((bool)nb::bool_(append))}; } - if (!nb::none().is(overwrite)) { - if (!nb::isinstance(overwrite)) { - throw InvalidInputException("to_parquet only accepts 'overwrite' as a boolean"); - } - options["overwrite_or_ignore"] = {Value::BOOLEAN((bool)nb::bool_(overwrite))}; - } + ApplyOverwriteOption(options, "to_parquet", filename, *rel, overwrite); if (!nb::none().is(per_thread_output)) { if (!nb::isinstance(per_thread_output)) { @@ -1467,12 +1489,7 @@ void DuckDBPyRelation::ToCSV(const string &filename, const nb::object &sep, cons options["compression"] = {Value(nb::cast(compression))}; } - if (!nb::none().is(overwrite)) { - if (!nb::isinstance(overwrite)) { - throw InvalidInputException("to_csv only accepts 'overwrite' as a boolean"); - } - options["overwrite_or_ignore"] = {Value::BOOLEAN((bool)nb::bool_(overwrite))}; - } + ApplyOverwriteOption(options, "to_csv", filename, *rel, overwrite); if (!nb::none().is(per_thread_output)) { if (!nb::isinstance(per_thread_output)) { diff --git a/tests/fast/api/test_to_csv.py b/tests/fast/api/test_to_csv.py index 9e51e316..fa6a9e5b 100644 --- a/tests/fast/api/test_to_csv.py +++ b/tests/fast/api/test_to_csv.py @@ -264,6 +264,19 @@ def test_to_csv_overwrite_not_enabled(self): with pytest.raises(duckdb.IOException, match="OVERWRITE"): rel.to_csv(temp_file_name, header=True, partition_by=["c_category_1"]) + def test_to_csv_overwrite_false_errors_on_existing_file(self): + temp_file_name = os.path.join(tempfile.mkdtemp(), next(tempfile._get_candidate_names())) # noqa: PTH118 + duckdb.sql("SELECT 1 AS test").to_csv(temp_file_name, overwrite=False) + with pytest.raises(duckdb.IOException, match="OVERWRITE"): + duckdb.sql("SELECT 2 AS test").to_csv(temp_file_name, overwrite=False) + assert duckdb.read_csv(temp_file_name).fetchall() == [(1,)] + + def test_to_csv_overwrite_true_replaces_existing_file(self): + temp_file_name = os.path.join(tempfile.mkdtemp(), next(tempfile._get_candidate_names())) # noqa: PTH118 + duckdb.sql("SELECT 1 AS test").to_csv(temp_file_name) + duckdb.sql("SELECT 2 AS test").to_csv(temp_file_name, overwrite=True) + assert duckdb.read_csv(temp_file_name).fetchall() == [(2,)] + def test_to_csv_per_thread_output(self): temp_file_name = os.path.join(tempfile.mkdtemp(), next(tempfile._get_candidate_names())) # noqa: PTH118 num_threads = duckdb.sql("select current_setting('threads')").fetchone()[0] diff --git a/tests/fast/api/test_to_parquet.py b/tests/fast/api/test_to_parquet.py index 71d5e00e..9b57b1c7 100644 --- a/tests/fast/api/test_to_parquet.py +++ b/tests/fast/api/test_to_parquet.py @@ -102,6 +102,19 @@ def test_overwrite(self, write_columns): assert result.execute().fetchall() == expected + def test_overwrite_false_errors_on_existing_file(self): + temp_file_name = os.path.join(tempfile.mkdtemp(), next(tempfile._get_candidate_names())) # noqa: PTH118 + duckdb.sql("SELECT 1 AS test").to_parquet(temp_file_name, overwrite=False) + with pytest.raises(duckdb.IOException, match="OVERWRITE"): + duckdb.sql("SELECT 2 AS test").to_parquet(temp_file_name, overwrite=False) + assert duckdb.read_parquet(temp_file_name).fetchall() == [(1,)] + + def test_overwrite_true_replaces_existing_file(self): + temp_file_name = os.path.join(tempfile.mkdtemp(), next(tempfile._get_candidate_names())) # noqa: PTH118 + duckdb.sql("SELECT 1 AS test").to_parquet(temp_file_name) + duckdb.sql("SELECT 2 AS test").to_parquet(temp_file_name, overwrite=True) + assert duckdb.read_parquet(temp_file_name).fetchall() == [(2,)] + def test_use_tmp_file(self): temp_file_name = os.path.join(tempfile.mkdtemp(), next(tempfile._get_candidate_names())) # noqa: PTH118 df = pd.DataFrame(