From 91a4817f1957e3959c35616e1aa390b02050f5c2 Mon Sep 17 00:00:00 2001 From: Yannick Welsch Date: Fri, 27 Sep 2024 08:59:29 +0200 Subject: [PATCH 01/11] Avoid schema changes with IF NOT EXISTS --- .../catalog_entry/duck_schema_entry.cpp | 11 +++++++++ test/catalog/test_catalog_version.cpp | 23 +++++++++++++++++++ test/sql/catalog/test_if_not_exists.test | 10 ++++++++ 3 files changed, 44 insertions(+) diff --git a/src/catalog/catalog_entry/duck_schema_entry.cpp b/src/catalog/catalog_entry/duck_schema_entry.cpp index 42dea06f0231..b56201ea2471 100644 --- a/src/catalog/catalog_entry/duck_schema_entry.cpp +++ b/src/catalog/catalog_entry/duck_schema_entry.cpp @@ -119,6 +119,17 @@ optional_ptr DuckSchemaEntry::AddEntryInternal(CatalogTransaction // first find the set for this entry auto &set = GetCatalogSet(entry_type); dependencies.AddDependency(*this); + if (on_conflict == OnCreateConflict::IGNORE_ON_CONFLICT) { + auto old_entry = set.GetEntry(transaction, entry_name); + if (old_entry) { + if (old_entry->type != entry_type) { + throw CatalogException("Existing object %s is of type %s, trying to replace with type %s", entry_name, + CatalogTypeToString(old_entry->type), CatalogTypeToString(entry_type)); + } + return nullptr; + } + } + if (on_conflict == OnCreateConflict::REPLACE_ON_CONFLICT) { // CREATE OR REPLACE: first try to drop the entry auto old_entry = set.GetEntry(transaction, entry_name); diff --git a/test/catalog/test_catalog_version.cpp b/test/catalog/test_catalog_version.cpp index 21d28b21b822..e22c90054778 100644 --- a/test/catalog/test_catalog_version.cpp +++ b/test/catalog/test_catalog_version.cpp @@ -44,6 +44,29 @@ TEST_CASE("Test catalog versioning", "[catalog]") { REQUIRE(catalog.GetCatalogVersion(*con1.context) == 2); }); + // The following should not increment the version + REQUIRE_NO_FAIL(con1.Query("CREATE TABLE IF NOT EXISTS foo3 as SELECT 42")); + + con1.context->RunFunctionInTransaction([&]() { + auto &catalog = Catalog::GetCatalog(*con1.context, ""); + REQUIRE(catalog.GetCatalogVersion(*con1.context) == 2); + }); + + REQUIRE_NO_FAIL(con1.Query("CREATE SCHEMA IF NOT EXISTS my_schema")); + + con1.context->RunFunctionInTransaction([&]() { + auto &catalog = Catalog::GetCatalog(*con1.context, ""); + REQUIRE(catalog.GetCatalogVersion(*con1.context) == 3); + }); + + // The following should not increment the version + REQUIRE_NO_FAIL(con1.Query("CREATE SCHEMA IF NOT EXISTS my_schema")); + + con1.context->RunFunctionInTransaction([&]() { + auto &catalog = Catalog::GetCatalog(*con1.context, ""); + REQUIRE(catalog.GetCatalogVersion(*con1.context) == 3); + }); + // check catalog version of system catalog con1.context->RunFunctionInTransaction([&]() { auto &catalog = Catalog::GetCatalog(*con1.context, "system"); diff --git a/test/sql/catalog/test_if_not_exists.test b/test/sql/catalog/test_if_not_exists.test index b546d63454fc..a280a271ea18 100644 --- a/test/sql/catalog/test_if_not_exists.test +++ b/test/sql/catalog/test_if_not_exists.test @@ -14,6 +14,16 @@ CREATE TABLE IF NOT EXISTS integers2 AS SELECT 42 statement ok CREATE TABLE IF NOT EXISTS integers2 AS SELECT 42 +statement error +CREATE VIEW IF NOT EXISTS integers2 AS SELECT 42 +---- +Existing object integers2 is of type Table, trying to replace with type View + +statement error +DROP VIEW IF EXISTS integers +---- +Existing object integers is of type Table, trying to replace with type View + statement ok DROP TABLE IF EXISTS integers From 8e8a01946f58c730747854bac55f9355887ebbe4 Mon Sep 17 00:00:00 2001 From: Yannick Welsch Date: Fri, 27 Sep 2024 13:57:12 +0200 Subject: [PATCH 02/11] feedback --- src/catalog/catalog_entry/duck_schema_entry.cpp | 6 +----- test/sql/catalog/test_if_not_exists.test | 7 +++---- test/sql/create/create_or_replace.test | 8 ++++++++ 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/catalog/catalog_entry/duck_schema_entry.cpp b/src/catalog/catalog_entry/duck_schema_entry.cpp index b56201ea2471..f3c8684fb397 100644 --- a/src/catalog/catalog_entry/duck_schema_entry.cpp +++ b/src/catalog/catalog_entry/duck_schema_entry.cpp @@ -122,10 +122,6 @@ optional_ptr DuckSchemaEntry::AddEntryInternal(CatalogTransaction if (on_conflict == OnCreateConflict::IGNORE_ON_CONFLICT) { auto old_entry = set.GetEntry(transaction, entry_name); if (old_entry) { - if (old_entry->type != entry_type) { - throw CatalogException("Existing object %s is of type %s, trying to replace with type %s", entry_name, - CatalogTypeToString(old_entry->type), CatalogTypeToString(entry_type)); - } return nullptr; } } @@ -326,7 +322,7 @@ void DuckSchemaEntry::DropEntry(ClientContext &context, DropInfo &info) { throw InternalException("Failed to drop entry \"%s\" - entry could not be found", info.name); } if (existing_entry->type != info.type) { - throw CatalogException("Existing object %s is of type %s, trying to replace with type %s", info.name, + throw CatalogException("Existing object %s is of type %s, trying to drop type %s", info.name, CatalogTypeToString(existing_entry->type), CatalogTypeToString(info.type)); } diff --git a/test/sql/catalog/test_if_not_exists.test b/test/sql/catalog/test_if_not_exists.test index a280a271ea18..a840f57b386f 100644 --- a/test/sql/catalog/test_if_not_exists.test +++ b/test/sql/catalog/test_if_not_exists.test @@ -14,15 +14,14 @@ CREATE TABLE IF NOT EXISTS integers2 AS SELECT 42 statement ok CREATE TABLE IF NOT EXISTS integers2 AS SELECT 42 -statement error +# similar to Postgres, treat as NOOP if the entry exists but the type is different +statement ok CREATE VIEW IF NOT EXISTS integers2 AS SELECT 42 ----- -Existing object integers2 is of type Table, trying to replace with type View statement error DROP VIEW IF EXISTS integers ---- -Existing object integers is of type Table, trying to replace with type View +Catalog Error: Existing object integers is of type Table, trying to drop type View statement ok DROP TABLE IF EXISTS integers diff --git a/test/sql/create/create_or_replace.test b/test/sql/create/create_or_replace.test index 529e12add4c2..35fc2300480c 100644 --- a/test/sql/create/create_or_replace.test +++ b/test/sql/create/create_or_replace.test @@ -11,6 +11,14 @@ CREATE TABLE integers(i INTEGER) statement ok CREATE OR REPLACE TABLE integers(i INTEGER, j INTEGER) +statement ok +CREATE VIEW integers2 AS SELECT 42 + +statement error +CREATE OR REPLACE TABLE integers2(i INTEGER) +---- +Catalog Error: integers2 is not an table + statement ok CREATE TABLE IF NOT EXISTS integers(i INTEGER) From 49f3ecac733cce877328c763faa8d018e915763e Mon Sep 17 00:00:00 2001 From: zmajeed Date: Fri, 27 Sep 2024 14:57:17 -0500 Subject: [PATCH 03/11] Remove header inclusion guard in windows_undefs.hpp to insure certain Win32 macros are always undefined. Copy deletion of ICU undefine of __STRICT_ANSI__ from ICU upstream that conflicts with GCC-14 header files --- extension/icu/third_party/icu/common/putil.cpp | 5 ----- src/include/duckdb/common/windows_undefs.hpp | 3 ++- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/extension/icu/third_party/icu/common/putil.cpp b/extension/icu/third_party/icu/common/putil.cpp index 56d25c3bdee0..c79811499441 100644 --- a/extension/icu/third_party/icu/common/putil.cpp +++ b/extension/icu/third_party/icu/common/putil.cpp @@ -46,11 +46,6 @@ // First, the platform type. Need this for U_PLATFORM. #include "unicode/platform.h" -#if U_PLATFORM == U_PF_MINGW && defined __STRICT_ANSI__ -/* tzset isn't defined in strict ANSI on MinGW. */ -#undef __STRICT_ANSI__ -#endif - /* * Cygwin with GCC requires inclusion of time.h after the above disabling strict asci mode statement. */ diff --git a/src/include/duckdb/common/windows_undefs.hpp b/src/include/duckdb/common/windows_undefs.hpp index 680c2a43c695..c991f32bf552 100644 --- a/src/include/duckdb/common/windows_undefs.hpp +++ b/src/include/duckdb/common/windows_undefs.hpp @@ -6,7 +6,8 @@ // //===----------------------------------------------------------------------===// -#pragma once +// Do not add a header inclusion guard to this file. Otherwise these Win32 macros +// may get defined and stomp on DuckDB symbols #ifdef WIN32 From d160a197ba7e06297866ff9faf9d12238870a4f6 Mon Sep 17 00:00:00 2001 From: zmajeed Date: Tue, 1 Oct 2024 15:00:02 -0500 Subject: [PATCH 04/11] Fix build when threads are disabled --- src/parallel/task_scheduler.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/parallel/task_scheduler.cpp b/src/parallel/task_scheduler.cpp index 743a52e5ef89..89417008d004 100644 --- a/src/parallel/task_scheduler.cpp +++ b/src/parallel/task_scheduler.cpp @@ -335,8 +335,13 @@ idx_t TaskScheduler::GetEstimatedCPUId() { #elif defined(_GNU_SOURCE) auto cpu = sched_getcpu(); if (cpu < 0) { +#ifndef DUCKDB_NO_THREADS // fallback to thread id return (idx_t)std::hash()(std::this_thread::get_id()); +#else + + return (idx_t)getpid(); +#endif } return (idx_t)cpu; #elif defined(__aarch64__) && defined(__APPLE__) @@ -345,8 +350,12 @@ idx_t TaskScheduler::GetEstimatedCPUId() { asm volatile("mrs %x0, tpidrro_el0" : "=r"(c)::"memory"); return (idx_t)(c & (1 << 3) - 1); #else +#ifndef DUCKDB_NO_THREADS // fallback to thread id return (idx_t)std::hash()(std::this_thread::get_id()); +#else + return (idx_t)getpid(); +#endif #endif #endif } From 61074eccec56aba267797487644eabe5d6be91d3 Mon Sep 17 00:00:00 2001 From: piever Date: Wed, 2 Oct 2024 11:48:35 +0200 Subject: [PATCH 05/11] add method to check whether julia connection is open --- tools/juliapkg/src/database.jl | 2 ++ tools/juliapkg/test/test_connection.jl | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/tools/juliapkg/src/database.jl b/tools/juliapkg/src/database.jl index 4b5d2926a72f..c30c358c4741 100644 --- a/tools/juliapkg/src/database.jl +++ b/tools/juliapkg/src/database.jl @@ -108,7 +108,9 @@ DBInterface.connect(db::DB) = Connection(db.handle) DBInterface.close!(db::DB) = close_database(db) DBInterface.close!(con::Connection) = _close_connection(con) Base.close(db::DB) = close_database(db) +Base.close(con::Connection) = _close_connection(con) Base.isopen(db::DB) = db.handle.handle != C_NULL +Base.isopen(con::Connection) = con.handle != C_NULL Base.show(io::IO, db::DuckDB.DB) = print(io, string("DuckDB.DB(", "\"$(db.handle.file)\"", ")")) Base.show(io::IO, con::DuckDB.Connection) = print(io, string("DuckDB.Connection(", "\"$(con.db.file)\"", ")")) diff --git a/tools/juliapkg/test/test_connection.jl b/tools/juliapkg/test/test_connection.jl index 7086ae538bcd..c09848b27450 100644 --- a/tools/juliapkg/test/test_connection.jl +++ b/tools/juliapkg/test/test_connection.jl @@ -7,6 +7,11 @@ DBInterface.close!(con) DBInterface.close!(con) @test 1 == 1 + + con = DBInterface.connect(DuckDB.DB, ":memory:") + @test isopen(con) + close(con) + @test !isopen(con) end @testset "Test opening a bogus directory" begin From e30387047945eadd19101974e796ef2c513a0d21 Mon Sep 17 00:00:00 2001 From: zmajeed Date: Wed, 2 Oct 2024 06:10:54 -0500 Subject: [PATCH 06/11] Use 0 for CPU ID when it cannot be determined --- src/parallel/task_scheduler.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/parallel/task_scheduler.cpp b/src/parallel/task_scheduler.cpp index 89417008d004..fd6671a3e161 100644 --- a/src/parallel/task_scheduler.cpp +++ b/src/parallel/task_scheduler.cpp @@ -340,7 +340,7 @@ idx_t TaskScheduler::GetEstimatedCPUId() { return (idx_t)std::hash()(std::this_thread::get_id()); #else - return (idx_t)getpid(); + return 0; #endif } return (idx_t)cpu; @@ -354,7 +354,7 @@ idx_t TaskScheduler::GetEstimatedCPUId() { // fallback to thread id return (idx_t)std::hash()(std::this_thread::get_id()); #else - return (idx_t)getpid(); + return 0; #endif #endif #endif From d73736194fc087f5c0f8270f4a7e6c0df6a1458d Mon Sep 17 00:00:00 2001 From: Tishj Date: Wed, 2 Oct 2024 16:31:02 +0200 Subject: [PATCH 07/11] detect the subquery after binding and throw --- .../binder/statement/bind_create_table.cpp | 5 ++++- .../generated_columns/virtual/create_table.test | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/planner/binder/statement/bind_create_table.cpp b/src/planner/binder/statement/bind_create_table.cpp index 018815088001..7ffbaf23603e 100644 --- a/src/planner/binder/statement/bind_create_table.cpp +++ b/src/planner/binder/statement/bind_create_table.cpp @@ -228,7 +228,10 @@ void Binder::BindGeneratedColumns(BoundCreateTableInfo &info) { auto bound_expression = expr_binder.Bind(expression); D_ASSERT(bound_expression); - D_ASSERT(!bound_expression->HasSubquery()); + if (bound_expression->HasSubquery()) { + throw BinderException("Failed to bind generated column '%s' because the expression contains a subquery", + col.Name()); + } if (col.Type().id() == LogicalTypeId::ANY) { // Do this before changing the type, so we know it's the first time the type is set col.ChangeGeneratedExpressionType(bound_expression->return_type); diff --git a/test/sql/generated_columns/virtual/create_table.test b/test/sql/generated_columns/virtual/create_table.test index f49cd8ff9266..2b754319980c 100644 --- a/test/sql/generated_columns/virtual/create_table.test +++ b/test/sql/generated_columns/virtual/create_table.test @@ -54,6 +54,23 @@ CREATE TABLE unit ( amount_sold INTEGER, ); ---- +Expression of generated column "total_profit" contains a subquery, which isn't allowed + +statement ok +CREATE MACRO my_macro() AS ( + (select 42) +); + +# Expression contains a subquery - through a macro +statement error +CREATE TABLE unit ( + total_profit INTEGER GENERATED ALWAYS AS(my_macro()) VIRTUAL, + word VARCHAR, + price INTEGER, + amount_sold INTEGER, +); +---- +Failed to bind generated column 'total_profit' because the expression contains a subquery # Duplicate column definition statement error From e5148aece1367219afa954dfddd8861214d0b09c Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Thu, 3 Oct 2024 10:29:15 +0200 Subject: [PATCH 08/11] Add missing word in TableFunction comment --- src/include/duckdb/function/table_function.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/include/duckdb/function/table_function.hpp b/src/include/duckdb/function/table_function.hpp index 68883293bb6d..d15990d078c1 100644 --- a/src/include/duckdb/function/table_function.hpp +++ b/src/include/duckdb/function/table_function.hpp @@ -242,7 +242,7 @@ class TableFunction : public SimpleNamedParameterFunction { // NOLINT: work-arou //! The returned FunctionData object should be constant and should not be changed during execution. table_function_bind_t bind; //! (Optional) Bind replace function - //! This function is called before the regular bind function. It allows returning a TableRef will be used to + //! This function is called before the regular bind function. It allows returning a TableRef that will be used to //! to generate a logical plan that replaces the LogicalGet of a regularly bound TableFunction. The BindReplace can //! also return a nullptr to indicate a regular bind needs to be performed instead. table_function_bind_replace_t bind_replace; From 3e4c9f16e318dc4416573e424420777ea4818814 Mon Sep 17 00:00:00 2001 From: Tishj Date: Fri, 4 Oct 2024 11:50:10 +0200 Subject: [PATCH 09/11] throw if the dict type is of size 0, STRUCTs must have at least one member --- tools/pythonpkg/src/typing/pytype.cpp | 3 +++ tools/pythonpkg/tests/fast/test_type.py | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/tools/pythonpkg/src/typing/pytype.cpp b/tools/pythonpkg/src/typing/pytype.cpp index 7b03f167121c..fc19c17ba123 100644 --- a/tools/pythonpkg/src/typing/pytype.cpp +++ b/tools/pythonpkg/src/typing/pytype.cpp @@ -270,6 +270,9 @@ static LogicalType FromGenericAlias(const py::object &obj) { static LogicalType FromDictionary(const py::object &obj) { auto dict = py::reinterpret_steal(obj); child_list_t children; + if (dict.size() == 0) { + throw InvalidInputException("Could not convert empty dictionary to a duckdb STRUCT type"); + } children.reserve(dict.size()); for (auto &item : dict) { auto &name_p = item.first; diff --git a/tools/pythonpkg/tests/fast/test_type.py b/tools/pythonpkg/tests/fast/test_type.py index 718b9defe310..d87a1cae38db 100644 --- a/tools/pythonpkg/tests/fast/test_type.py +++ b/tools/pythonpkg/tests/fast/test_type.py @@ -88,6 +88,12 @@ def test_struct_type(self): type = duckdb.struct_type([BIGINT, BOOLEAN]) assert str(type) == 'STRUCT(v1 BIGINT, v2 BOOLEAN)' + def test_incomplete_struct_type(self): + with pytest.raises( + duckdb.InvalidInputException, match='Could not convert empty dictionary to a duckdb STRUCT type' + ): + type = duckdb.typing.DuckDBPyType(dict()) + def test_map_type(self): type = duckdb.map_type(duckdb.sqltype("BIGINT"), duckdb.sqltype("DECIMAL(10, 2)")) assert str(type) == 'MAP(BIGINT, DECIMAL(10,2))' From 37e4d0365c8c30b4e863f64ee0f7dc05bde4aaf6 Mon Sep 17 00:00:00 2001 From: Tishj Date: Fri, 4 Oct 2024 17:15:06 +0200 Subject: [PATCH 10/11] add the ExecutorException class, making use of the EXECUTOR ExceptionType --- src/common/exception.cpp | 3 +++ src/include/duckdb/common/exception.hpp | 10 ++++++++++ 2 files changed, 13 insertions(+) diff --git a/src/common/exception.cpp b/src/common/exception.cpp index b8aac720765c..4527f3ff5414 100644 --- a/src/common/exception.cpp +++ b/src/common/exception.cpp @@ -292,6 +292,9 @@ PermissionException::PermissionException(const string &msg) : Exception(Exceptio SyntaxException::SyntaxException(const string &msg) : Exception(ExceptionType::SYNTAX, msg) { } +ExecutorException::ExecutorException(const string &msg) : Exception(ExceptionType::EXECUTOR, msg) { +} + ConstraintException::ConstraintException(const string &msg) : Exception(ExceptionType::CONSTRAINT, msg) { } diff --git a/src/include/duckdb/common/exception.hpp b/src/include/duckdb/common/exception.hpp index dd6431c5999b..3bb6059267e4 100644 --- a/src/include/duckdb/common/exception.hpp +++ b/src/include/duckdb/common/exception.hpp @@ -329,6 +329,16 @@ class InvalidInputException : public Exception { } }; +class ExecutorException : public Exception { +public: + DUCKDB_API explicit ExecutorException(const string &msg); + + template + explicit ExecutorException(const string &msg, ARGS... params) + : ExecutorException(ConstructMessage(msg, params...)) { + } +}; + class InvalidConfigurationException : public Exception { public: DUCKDB_API explicit InvalidConfigurationException(const string &msg); From 82b81fda9ac13ab736326cabc5521fc5cfaed438 Mon Sep 17 00:00:00 2001 From: c8ef Date: Mon, 7 Oct 2024 10:09:15 +0800 Subject: [PATCH 11/11] Update union_cast.test --- test/sql/types/union/union_cast.test | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sql/types/union/union_cast.test b/test/sql/types/union/union_cast.test index aa50b9110cef..e04cf46ed17e 100644 --- a/test/sql/types/union/union_cast.test +++ b/test/sql/types/union/union_cast.test @@ -129,7 +129,7 @@ i32 1 NULL NULL str NULL two NULL str NULL three NULL -# Allow explict casts +# Allow explicit casts statement ok CREATE TABLE tbl3 (u UNION(i INT));