Merge pull request #9424 from Maxxen/serialize-operators

Fix field ids in LogicalCopyToFile:Deserialize
duckdb · Oct 24, 2023 · f880e23 · f880e23
2 parents 7895966 + 4625aab
commit f880e23
Show file tree

Hide file tree

Showing 8 changed files with 33 additions and 23 deletions.
diff --git a/.github/config/out_of_tree_extensions.cmake b/.github/config/out_of_tree_extensions.cmake
@@ -58,6 +58,7 @@ duckdb_extension_load(spatial
         GIT_TAG 36e5a126976ac3b66716893360ef7e6295707082
         INCLUDE_DIR spatial/include
         TEST_DIR test/sql
+        APPLY_PATCHES
         )
 
 ################# SQLITE_SCANNER

diff --git a/.github/patches/extensions/spatial/const_copy_param.patch b/.github/patches/extensions/spatial/const_copy_param.patch
@@ -0,0 +1,15 @@
+diff --git a/spatial/src/spatial/gdal/functions/st_write.cpp b/spatial/src/spatial/gdal/functions/st_write.cpp
+index 36a71da..15ebcf4 100644
+--- a/spatial/src/spatial/gdal/functions/st_write.cpp
++++ b/spatial/src/spatial/gdal/functions/st_write.cpp
+@@ -55,8 +55,8 @@ struct GlobalState : public GlobalFunctionData {
+ //===--------------------------------------------------------------------===//
+ // Bind
+ //===--------------------------------------------------------------------===//
+-static unique_ptr<FunctionData> Bind(ClientContext &context, CopyInfo &info, vector<string> &names,
+-                                     vector<LogicalType> &sql_types) {
++static unique_ptr<FunctionData> Bind(ClientContext &context, const CopyInfo &info, const vector<string> &names,
++                                     const vector<LogicalType> &sql_types) {
+
+ 	GdalFileHandler::SetLocalClientContext(context);
+
diff --git a/extension/parquet/parquet_extension.cpp b/extension/parquet/parquet_extension.cpp
@@ -740,8 +740,8 @@ static void GetFieldIDs(const Value &field_ids_value, ChildFieldIDs &field_ids,
 	}
 }
 
-unique_ptr<FunctionData> ParquetWriteBind(ClientContext &context, CopyInfo &info, vector<string> &names,
-                                          vector<LogicalType> &sql_types) {
+unique_ptr<FunctionData> ParquetWriteBind(ClientContext &context, const CopyInfo &info, const vector<string> &names,
+                                          const vector<LogicalType> &sql_types) {
 	D_ASSERT(names.size() == sql_types.size());
 	bool row_group_size_bytes_set = false;
 	auto bind_data = make_uniq<ParquetWriteBindData>();

diff --git a/src/function/table/copy_csv.cpp b/src/function/table/copy_csv.cpp
@@ -91,15 +91,15 @@ void BaseCSVData::Finalize() {
 	}
 }
 
-static unique_ptr<FunctionData> WriteCSVBind(ClientContext &context, CopyInfo &info, vector<string> &names,
-                                             vector<LogicalType> &sql_types) {
+static unique_ptr<FunctionData> WriteCSVBind(ClientContext &context, const CopyInfo &info, const vector<string> &names,
+                                             const vector<LogicalType> &sql_types) {
 	auto bind_data = make_uniq<WriteCSVData>(info.file_path, sql_types, names);
 
 	// check all the options in the copy info
 	for (auto &option : info.options) {
 		auto loption = StringUtil::Lower(option.first);
 		auto &set = option.second;
-		bind_data->options.SetWriteOption(loption, ConvertVectorToValue(std::move(set)));
+		bind_data->options.SetWriteOption(loption, ConvertVectorToValue(set));
 	}
 	// verify the parsed options
 	if (bind_data->options.force_quote.empty()) {

diff --git a/src/include/duckdb/function/copy_function.hpp b/src/include/duckdb/function/copy_function.hpp
@@ -71,8 +71,8 @@ struct PreparedBatchData {
 enum class CopyFunctionExecutionMode { REGULAR_COPY_TO_FILE, PARALLEL_COPY_TO_FILE, BATCH_COPY_TO_FILE };
 
 typedef BoundStatement (*copy_to_plan_t)(Binder &binder, CopyStatement &stmt);
-typedef unique_ptr<FunctionData> (*copy_to_bind_t)(ClientContext &context, CopyInfo &info, vector<string> &names,
-                                                   vector<LogicalType> &sql_types);
+typedef unique_ptr<FunctionData> (*copy_to_bind_t)(ClientContext &context, const CopyInfo &info,
+                                                   const vector<string> &names, const vector<LogicalType> &sql_types);
 typedef unique_ptr<LocalFunctionData> (*copy_to_initialize_local_t)(ExecutionContext &context, FunctionData &bind_data);
 typedef unique_ptr<GlobalFunctionData> (*copy_to_initialize_global_t)(ClientContext &context, FunctionData &bind_data,
                                                                       const string &file_path);

diff --git a/src/include/duckdb/planner/operator/logical_copy_to_file.hpp b/src/include/duckdb/planner/operator/logical_copy_to_file.hpp
@@ -21,7 +21,7 @@ class LogicalCopyToFile : public LogicalOperator {
 
 public:
 	LogicalCopyToFile(CopyFunction function, unique_ptr<FunctionData> bind_data, unique_ptr<CopyInfo> copy_info)
-	    : LogicalOperator(LogicalOperatorType::LOGICAL_COPY_TO_FILE), function(function),
+	    : LogicalOperator(LogicalOperatorType::LOGICAL_COPY_TO_FILE), function(std::move(function)),
 	      bind_data(std::move(bind_data)), copy_info(std::move(copy_info)) {
 	}
 	CopyFunction function;

diff --git a/src/planner/binder/statement/bind_copy.cpp b/src/planner/binder/statement/bind_copy.cpp
@@ -141,12 +141,13 @@ BoundStatement Binder::BindCopyTo(CopyStatement &stmt) {
 	}
 
 	auto unique_column_names = GetUniqueNames(select_node.names);
+	auto file_path = stmt.info->file_path;
 
 	auto function_data =
 	    copy_function.function.copy_to_bind(context, *stmt.info, unique_column_names, select_node.types);
 	// now create the copy information
-	auto copy = make_uniq<LogicalCopyToFile>(copy_function.function, std::move(function_data), stmt.info->Copy());
-	copy->file_path = stmt.info->file_path;
+	auto copy = make_uniq<LogicalCopyToFile>(copy_function.function, std::move(function_data), std::move(stmt.info));
+	copy->file_path = file_path;
 	copy->use_tmp_file = use_tmp_file;
 	copy->overwrite_or_ignore = overwrite_or_ignore;
 	copy->filename_pattern = filename_pattern;

diff --git a/src/planner/operator/logical_copy_to_file.cpp b/src/planner/operator/logical_copy_to_file.cpp
@@ -49,7 +49,7 @@ unique_ptr<LogicalOperator> LogicalCopyToFile::Deserialize(Deserializer &deseria
 
 	// Deserialize function
 	auto &context = deserializer.Get<ClientContext &>();
-	auto name = deserializer.ReadProperty<string>(209, "function_name");
+	auto name = deserializer.ReadProperty<string>(210, "function_name");
 
 	auto &func_catalog_entry =
 	    Catalog::GetEntry(context, CatalogType::COPY_FUNCTION_ENTRY, SYSTEM_CATALOG, DEFAULT_SCHEMA, name);
@@ -58,26 +58,19 @@ unique_ptr<LogicalOperator> LogicalCopyToFile::Deserialize(Deserializer &deseria
 	}
 	auto &function_entry = func_catalog_entry.Cast<CopyFunctionCatalogEntry>();
 	auto function = function_entry.function;
-
 	// Deserialize function data
 	unique_ptr<FunctionData> bind_data;
-	auto has_serialize = deserializer.ReadProperty<bool>(210, "function_has_serialize");
+	auto has_serialize = deserializer.ReadProperty<bool>(211, "function_has_serialize");
 	if (has_serialize) {
-		deserializer.ReadObject(211, "function_data",
+		// Just deserialize the bind data
+		deserializer.ReadObject(212, "function_data",
 		                        [&](Deserializer &obj) { bind_data = function.deserialize(obj, function); });
 	} else {
+		// Otherwise, re-bind with the copy info
 		if (!function.copy_to_bind) {
 			throw InternalException("Copy function \"%s\" has neither bind nor (de)serialize", function.name);
 		}
-		vector<LogicalType> bind_types;
-		vector<string> bind_names;
-		bind_data = function.copy_to_bind(context, *copy_info, bind_names, bind_types);
-		if (bind_names != names) {
-			throw InternalException("Copy function \"%s\" has different names in bind and deserialize", function.name);
-		}
-		if (bind_types != expected_types) {
-			throw InternalException("Copy function \"%s\" has different types in bind and deserialize", function.name);
-		}
+		bind_data = function.copy_to_bind(context, *copy_info, names, expected_types);
 	}
 
 	auto result = make_uniq<LogicalCopyToFile>(function, std::move(bind_data), std::move(copy_info));