duckdb to_string: don't copy (#7582)

1. Don't allocate new string maps in to_string, copy to existing.
2. Add EXPLAIN test for to_string
3. Fix a bug in duckdb sql logic tests runner which didn't compare JSON
correctly.

Signed-off-by: Mikhail Kot <to@myrrc.dev>

Author: myrrc

vortex-duckdb/cpp/include/duckdb_vx/table_function.h

@@ -38,18 +38,10 @@ void duckdb_vx_tfunc_bind_result_add_column(duckdb_vx_tfunc_bind_result ffi_resu
                                             size_t name_len,
                                             duckdb_logical_type ffi_type);
 
-// String map for to_string result
 typedef struct duckdb_vx_string_map_ *duckdb_vx_string_map;
-
-// Create a new string map
-duckdb_vx_string_map duckdb_vx_string_map_create();
-
 // Add a key-value pair to the string map
 void duckdb_vx_string_map_insert(duckdb_vx_string_map map, const char *key, const char *value);
 
-// Free the string map
-void duckdb_vx_string_map_free(duckdb_vx_string_map map);
-
 // Input data passed into the init_global and init_local callbacks.
 typedef struct {
     const void *bind_data;
@@ -153,8 +145,8 @@ typedef struct {
     bool (*pushdown_complex_filter)(void *bind_data, duckdb_vx_expr expr, duckdb_vx_error *error_out);
 
     void *pushdown_expression;
-    duckdb_vx_string_map (*to_string)(void *bind_data);
-    // void *dynamic_to_string;
+
+    void (*to_string)(void *bind_data, duckdb_vx_string_map map);
 
     double (*table_scan_progress)(duckdb_client_context ctx, void *bind_data, void *global_state);
 

vortex-duckdb/cpp/table_function.cpp

@@ -397,24 +397,18 @@ OperatorPartitionData c_get_partition_data(ClientContext & /*context*/,
     return OperatorPartitionData(index);
 }
 
+extern "C" void duckdb_vx_string_map_insert(duckdb_vx_string_map map, const char *key, const char *value) {
+    D_ASSERT(map);
+    D_ASSERT(key);
+    D_ASSERT(value);
+    reinterpret_cast<InsertionOrderPreservingMap<string> *>(map)->insert(key, value);
+}
+
 InsertionOrderPreservingMap<string> c_to_string(TableFunctionToStringInput &input) {
     InsertionOrderPreservingMap<string> result;
-    auto &bind = input.bind_data->Cast<CTableBindData>();
-
-    // Call the Rust side to get custom string representation if available
-    if (bind.info->vtab.to_string) {
-        auto map = bind.info->vtab.to_string(bind.ffi_data->DataPtr());
-        if (map) {
-            // Copy the map contents to the result
-            auto *cpp_map = reinterpret_cast<InsertionOrderPreservingMap<string> *>(map);
-            for (const auto &[key, value] : *cpp_map) {
-                result[key] = value;
-            }
-            // Free the map allocated by Rust
-            duckdb_vx_string_map_free(map);
-        }
-    }
-
+    duckdb_vx_string_map ffi_map = reinterpret_cast<duckdb_vx_string_map>(&result);
+    void *const ffi_bind = input.bind_data->Cast<CTableBindData>().ffi_data->DataPtr();
+    static_cast<CTableFunctionInfo &>(*input.table_function.function_info).vtab.to_string(ffi_bind, ffi_map);
     return result;
 }
 
@@ -471,25 +465,4 @@ extern "C" duckdb_state duckdb_vx_tfunc_register(duckdb_database ffi_db, const d
     }
     return DuckDBSuccess;
 }
-
-extern "C" duckdb_vx_string_map duckdb_vx_string_map_create() {
-    auto map = new InsertionOrderPreservingMap<string>();
-    return reinterpret_cast<duckdb_vx_string_map>(map);
-}
-
-extern "C" void duckdb_vx_string_map_insert(duckdb_vx_string_map map, const char *key, const char *value) {
-    if (!map || !key || !value) {
-        return;
-    }
-    auto *cpp_map = reinterpret_cast<InsertionOrderPreservingMap<string> *>(map);
-    (*cpp_map)[string(key)] = string(value);
-}
-
-extern "C" void duckdb_vx_string_map_free(duckdb_vx_string_map map) {
-    if (!map) {
-        return;
-    }
-    auto *cpp_map = reinterpret_cast<InsertionOrderPreservingMap<string> *>(map);
-    delete cpp_map;
-}
 } // namespace vortex

vortex-duckdb/src/datasource.rs

@@ -64,6 +64,7 @@ use crate::duckdb::Cardinality;
 use crate::duckdb::ClientContextRef;
 use crate::duckdb::ColumnStatistics;
 use crate::duckdb::DataChunkRef;
+use crate::duckdb::DuckdbStringMapRef;
 use crate::duckdb::ExpressionRef;
 use crate::duckdb::LogicalType;
 use crate::duckdb::TableFilterSetRef;
@@ -540,17 +541,12 @@ impl<T: DataSourceTableFunction> TableFunction for T {
             .ok_or_else(|| vortex_err!("batch id missing, no batches exported"))
     }
 
-    fn to_string(bind_data: &Self::BindData) -> Option<Vec<(String, String)>> {
-        let mut result = Vec::new();
-
-        result.push(("Function".to_string(), "Vortex Scan".to_string()));
-
+    fn to_string(bind_data: &Self::BindData, map: &mut DuckdbStringMapRef) {
+        map.push("Function", "Vortex Scan");
         if !bind_data.filter_exprs.is_empty() {
             let mut filters = bind_data.filter_exprs.iter().map(|f| format!("{}", f));
-            result.push(("Filters".to_string(), filters.join(" /\\\n")));
+            map.push("Filters", &filters.join(" /\\\n"));
         }
-
-        Some(result)
     }
 }
 

vortex-duckdb/src/duckdb/table_function/mod.rs

@@ -45,6 +45,18 @@ pub struct ColumnStatistics {
     pub has_null: bool,
 }
 
+// String map lifetime is managed by C++ code
+crate::lifetime_wrapper!(DuckdbStringMap, cpp::duckdb_vx_string_map, |_| {});
+impl DuckdbStringMapRef {
+    pub fn push(&mut self, key: &str, value: &str) {
+        let key = CString::new(key).unwrap_or_else(|_| CString::default());
+        let value = CString::new(value).unwrap_or_else(|_| CString::default());
+        unsafe {
+            cpp::duckdb_vx_string_map_insert(self.as_ptr(), key.as_ptr(), value.as_ptr());
+        }
+    }
+}
+
 /// A trait that defines the supported operations for a table function in DuckDB.
 ///
 /// This trait does not yet cover the full C++ API, see table_function.hpp.
@@ -154,9 +166,7 @@ pub trait TableFunction: Sized + Debug {
     ) -> VortexResult<u64>;
 
     /// Returns a vector of key-value pairs for EXPLAIN output
-    fn to_string(_bind_data: &Self::BindData) -> Option<Vec<(String, String)>> {
-        None
-    }
+    fn to_string(bind_data: &Self::BindData, map: &mut DuckdbStringMapRef);
 
     // TODO(ngates): there are many more callbacks that can be configured.
 }
@@ -223,35 +233,13 @@ impl DatabaseRef {
     }
 }
 
-/// The to_string callback for a table function.
 unsafe extern "C-unwind" fn to_string_callback<T: TableFunction>(
     bind_data: *mut c_void,
-) -> cpp::duckdb_vx_string_map {
+    map: cpp::duckdb_vx_string_map,
+) {
     let bind_data = unsafe { &*(bind_data as *const T::BindData) };
-
-    match T::to_string(bind_data) {
-        Some(map) => {
-            // Create a new C++ map
-            let cpp_map = unsafe { cpp::duckdb_vx_string_map_create() };
-
-            // Fill the map with key-value pairs
-            for (key, value) in map {
-                let key_cstr = CString::new(key).unwrap_or_else(|_| CString::default());
-                let value_cstr = CString::new(value).unwrap_or_else(|_| CString::default());
-
-                unsafe {
-                    cpp::duckdb_vx_string_map_insert(
-                        cpp_map,
-                        key_cstr.as_ptr(),
-                        value_cstr.as_ptr(),
-                    );
-                }
-            }
-
-            cpp_map
-        }
-        None => ptr::null_mut(),
-    }
+    let map = unsafe { DuckdbStringMap::borrow_mut(map) };
+    T::to_string(bind_data, map);
 }
 
 /// The native function callback for a table function.

vortex-duckdb/src/e2e_test/object_cache_test.rs

@@ -121,6 +121,8 @@ impl TableFunction for TestTableFunction {
     ) -> Option<ColumnStatistics> {
         None
     }
+
+    fn to_string(_bind_data: &Self::BindData, _map: &mut crate::duckdb::DuckdbStringMapRef) {}
 }
 
 use crate::duckdb::Database;

vortex-sqllogictest/bin/sqllogictests-runner.rs

@@ -17,6 +17,7 @@ use indicatif::MultiProgress;
 use indicatif::ProgressBar;
 use indicatif::ProgressDrawTarget;
 use indicatif::ProgressStyle;
+use sqllogictest::Normalizer;
 use sqllogictest::Record;
 use sqllogictest::Runner;
 use sqllogictest::parse_file;
@@ -28,6 +29,18 @@ use vortex_sqllogictest::duckdb::DuckDB;
 use vortex_sqllogictest::duckdb::DuckDBTestError;
 use vortex_sqllogictest::utils::list_files;
 
+fn duckdb_validator(normalizer: Normalizer, actual: &[Vec<String>], expected: &[String]) -> bool {
+    let actual = actual.iter().flat_map(|strings| {
+        strings
+            .join(" ")
+            .trim_end()
+            .split('\n')
+            .map(|line| line.trim_end().to_string())
+            .collect::<Vec<_>>()
+    });
+    Iterator::eq(actual, expected.iter().map(normalizer))
+}
+
 #[tokio::main]
 async fn main() -> anyhow::Result<()> {
     let args = Args::parse();
@@ -127,6 +140,7 @@ async fn main() -> anyhow::Result<()> {
                 duckdb_runner.add_label("duckdb");
                 duckdb_runner.with_column_validator(strict_column_validator);
                 duckdb_runner.with_normalizer(value_normalizer);
+                duckdb_runner.with_validator(duckdb_validator);
 
                 for record in records.iter() {
                     if let Record::Halt { .. } = record {

vortex-sqllogictest/slt/explain.slt

@@ -0,0 +1,26 @@
+# SPDX-License-Identifier: Apache-2.0
+# SPDX-FileCopyrightText: Copyright the Vortex contributors
+
+include ./setup.slt
+
+onlyif duckdb
+query I
+COPY (SELECT * FROM (VALUES ('Hello'), ('Hi'), ('Hey')) AS t(str)) TO '$__TEST_DIR__/explain.vortex';
+----
+3
+
+onlyif duckdb
+query TT
+EXPLAIN (FORMAT json) SELECT * FROM '$__TEST_DIR__/explain.vortex';
+----
+physical_plan [
+    {
+        "name": "READ_VORTEX",
+        "children": [],
+        "extra_info": {
+            "Function": "Vortex Scan",
+            "Projections": "str",
+            "Estimated Cardinality": "3"
+        }
+    }
+]