apply suggestions from code review

Author: teunbrand

Cargo.toml

@@ -59,7 +59,6 @@ palette = { version = "0.7", default-features = false, features = ["std", "appro
 
 # Spatial
 geozero = { version = "0.14", default-features = false }
-hex = "0.4"
 
 # Utilities
 regex = "1.10"

src/execute/mod.rs

@@ -369,18 +369,6 @@ fn detect_geometry_column(schema: &Schema) -> Option<String> {
         return Some(candidates[0].name.clone());
     }
 
-    // Fall back to name-only match (e.g. extension types we don't recognise)
-    if candidates.is_empty() {
-        let by_name: Vec<_> = schema
-            .iter()
-            .filter(|c| looks_like_geometry(&c.name))
-            .collect();
-
-        if by_name.len() == 1 {
-            return Some(by_name[0].name.clone());
-        }
-    }
-
     None
 }
 

src/reader/data.rs

@@ -60,66 +60,6 @@ pub fn builtin_parquet_bytes(name: &str) -> Option<&'static [u8]> {
     }
 }
 
-// =============================================================================
-// DuckDB builtin data registration (requires duckdb + builtin-data)
-// =============================================================================
-
-/// Register any builtin datasets referenced in the SQL with a DuckDB connection.
-///
-/// Finds `ggsql:X` patterns in the SQL, writes the embedded parquet data to
-/// a temp file, and creates a table named `__ggsql_data_X__` in DuckDB.
-#[cfg(all(feature = "duckdb", feature = "builtin-data"))]
-pub fn register_builtin_datasets_duckdb(
-    sql: &str,
-    conn: &duckdb::Connection,
-) -> Result<(), GgsqlError> {
-    use std::{env, fs};
-
-    let dataset_names = extract_builtin_dataset_names(sql)?;
-
-    // Load spatial extension before registering datasets that contain
-    // geometry columns, so DuckDB reads them as GEOMETRY rather than BLOB.
-    if dataset_names.iter().any(|n| n == "world") {
-        let _ = conn.execute("LOAD spatial", duckdb::params![]);
-    }
-
-    for name in dataset_names {
-        let Some(parquet_bytes) = builtin_parquet_bytes(&name) else {
-            continue;
-        };
-
-        let table_name = naming::builtin_data_table(&name);
-
-        // Write parquet to temp file for DuckDB's read_parquet
-        let mut tmp_path = env::temp_dir();
-        tmp_path.push(format!("{}.parquet", name));
-        if !tmp_path.exists() {
-            fs::write(&tmp_path, parquet_bytes).map_err(|e| {
-                GgsqlError::ReaderError(format!(
-                    "Failed to write builtin dataset '{}' to {}: {}",
-                    name,
-                    tmp_path.display(),
-                    e
-                ))
-            })?;
-        }
-
-        let create_sql = format!(
-            "CREATE TABLE IF NOT EXISTS {} AS SELECT * FROM read_parquet('{}')",
-            naming::quote_ident(&table_name),
-            tmp_path.display()
-        );
-
-        conn.execute(&create_sql, duckdb::params![]).map_err(|e| {
-            GgsqlError::ReaderError(format!(
-                "Failed to register builtin dataset '{}': {}",
-                name, e
-            ))
-        })?;
-    }
-    Ok(())
-}
-
 // =============================================================================
 // Arrow-based builtin data loading
 // =============================================================================

src/reader/duckdb.rs

@@ -13,6 +13,66 @@ use std::cell::RefCell;
 use std::collections::HashSet;
 use std::sync::Arc;
 
+// =============================================================================
+// DuckDB builtin data registration
+// =============================================================================
+
+/// Register any builtin datasets referenced in the SQL with a DuckDB connection.
+///
+/// Finds `ggsql:X` patterns in the SQL, writes the embedded parquet data to
+/// a temp file, and creates a table named `__ggsql_data_X__` in DuckDB.
+#[cfg(feature = "builtin-data")]
+fn register_builtin_datasets_duckdb(
+    sql: &str,
+    conn: &Connection,
+) -> Result<()> {
+    use std::{env, fs};
+
+    let dataset_names = super::data::extract_builtin_dataset_names(sql)?;
+
+    // Load spatial extension before registering datasets that contain
+    // geometry columns, so DuckDB reads them as GEOMETRY rather than BLOB.
+    if dataset_names.iter().any(|n| n == "world") {
+        let _ = conn.execute("LOAD spatial", params![]);
+    }
+
+    for name in dataset_names {
+        let Some(parquet_bytes) = super::data::builtin_parquet_bytes(&name) else {
+            continue;
+        };
+
+        let table_name = naming::builtin_data_table(&name);
+
+        // Write parquet to temp file for DuckDB's read_parquet
+        let mut tmp_path = env::temp_dir();
+        tmp_path.push(format!("{}.parquet", name));
+        if !tmp_path.exists() {
+            fs::write(&tmp_path, parquet_bytes).map_err(|e| {
+                GgsqlError::ReaderError(format!(
+                    "Failed to write builtin dataset '{}' to {}: {}",
+                    name,
+                    tmp_path.display(),
+                    e
+                ))
+            })?;
+        }
+
+        let create_sql = format!(
+            "CREATE TABLE IF NOT EXISTS {} AS SELECT * FROM read_parquet('{}')",
+            naming::quote_ident(&table_name),
+            tmp_path.display()
+        );
+
+        conn.execute(&create_sql, params![]).map_err(|e| {
+            GgsqlError::ReaderError(format!(
+                "Failed to register builtin dataset '{}': {}",
+                name, e
+            ))
+        })?;
+    }
+    Ok(())
+}
+
 /// DuckDB SQL dialect with native function support.
 ///
 /// Overrides SQL generation methods to use DuckDB-native functions
@@ -214,7 +274,7 @@ impl Reader for DuckDBReader {
     fn execute_sql(&self, sql: &str) -> Result<DataFrame> {
         // Register builtin datasets if referenced
         #[cfg(feature = "builtin-data")]
-        super::data::register_builtin_datasets_duckdb(sql, &self.conn)?;
+        register_builtin_datasets_duckdb(sql, &self.conn)?;
 
         // Rewrite ggsql:name → __ggsql_data_name__ in SQL
         let sql = super::data::rewrite_namespaced_sql(sql)?;

src/writer/vegalite/layer.rs

@@ -2220,8 +2220,6 @@ impl GeomRenderer for SpatialRenderer {
                 let mut feature = serde_json::Map::new();
                 feature.insert("type".to_string(), json!("Feature"));
 
-                let mut properties = serde_json::Map::new();
-
                 for col_name in &col_names {
                     let col = df.column(col_name).map_err(|e| {
                         GgsqlError::WriterError(format!(
@@ -2233,14 +2231,18 @@ impl GeomRenderer for SpatialRenderer {
                     if *col_name == geometry_col {
                         let geom = Self::parse_geometry_from_array(col, row_idx)?;
                         feature.insert("geometry".to_string(), geom);
-                    } else {
+                    } else if !matches!(
+                        // These are reserved names for the geojson format, so
+                        // we shouldn't be inserting columns with that name.
+                        // Our naming module should prevent such collisions,
+                        // so this is purely defensive.
+                        col_name.as_str(),
+                        "type" | "geometry" | "properties" | "bbox" | "id"
+                    ) {
                         let value = super::data::series_value_at(col, row_idx)?;
-                        properties.insert(col_name.clone(), value.clone());
                         feature.insert(col_name.clone(), value);
                     }
                 }
-
-                feature.insert("properties".to_string(), Value::Object(properties));
                 features.push(Value::Object(feature));
             }