Merge pull request #67 from hotdata-dev/fix/column-id-overflow

fix: reject column_id values exceeding i32 range

Author: zfarrell

src/types.rs

@@ -293,7 +293,12 @@ pub fn build_read_schema_with_field_id_mapping(
         .iter()
         .map(|col| {
             let data_type = ducklake_to_arrow_type(&col.column_type)?;
-            let field_id = col.column_id as i32;
+            let field_id = i32::try_from(col.column_id).map_err(|_| {
+                DuckLakeError::Internal(format!(
+                    "column_id {} for column '{}' exceeds i32 range for Parquet field_id",
+                    col.column_id, col.column_name
+                ))
+            })?;
 
             let (read_name, needs_rename) =
                 if let Some(parquet_name) = parquet_field_ids.get(&field_id) {
@@ -746,4 +751,69 @@ mod tests {
             _ => panic!("Expected UnsupportedType error when building schema with complex type"),
         }
     }
+
+    #[test]
+    fn test_column_id_i32_max_succeeds() {
+        let columns = vec![DuckLakeTableColumn {
+            column_id: i32::MAX as i64,
+            column_name: "id".to_string(),
+            column_type: "int32".to_string(),
+            is_nullable: true,
+        }];
+
+        let mut parquet_field_ids = HashMap::new();
+        parquet_field_ids.insert(i32::MAX, "id".to_string());
+
+        let result = build_read_schema_with_field_id_mapping(&columns, &parquet_field_ids);
+        assert!(result.is_ok(), "column_id = i32::MAX should succeed");
+    }
+
+    #[test]
+    fn test_column_id_overflow_returns_error() {
+        let columns = vec![DuckLakeTableColumn {
+            column_id: i32::MAX as i64 + 1, // 2147483648, exceeds i32 range
+            column_name: "id".to_string(),
+            column_type: "int32".to_string(),
+            is_nullable: true,
+        }];
+
+        let parquet_field_ids = HashMap::new();
+
+        let result = build_read_schema_with_field_id_mapping(&columns, &parquet_field_ids);
+        assert!(result.is_err(), "column_id > i32::MAX should fail");
+        match result {
+            Err(DuckLakeError::Internal(msg)) => {
+                assert!(
+                    msg.contains("2147483648"),
+                    "Error should contain the overflowing value: {}",
+                    msg
+                );
+                assert!(
+                    msg.contains("exceeds i32 range"),
+                    "Error should explain the issue: {}",
+                    msg
+                );
+            },
+            _ => panic!("Expected Internal error for column_id overflow"),
+        }
+    }
+
+    #[test]
+    fn test_column_id_negative_within_i32_range_succeeds() {
+        let columns = vec![DuckLakeTableColumn {
+            column_id: -1,
+            column_name: "id".to_string(),
+            column_type: "int32".to_string(),
+            is_nullable: true,
+        }];
+
+        let mut parquet_field_ids = HashMap::new();
+        parquet_field_ids.insert(-1_i32, "id".to_string());
+
+        let result = build_read_schema_with_field_id_mapping(&columns, &parquet_field_ids);
+        assert!(
+            result.is_ok(),
+            "Negative column_id within i32 range should succeed"
+        );
+    }
 }

tests/common/mod.rs

@@ -10,6 +10,18 @@
 
 use anyhow::Result;
 use std::path::Path;
+use std::sync::Once;
+
+static INSTALL_DUCKLAKE: Once = Once::new();
+
+/// Install the ducklake extension exactly once (thread-safe across parallel tests).
+fn ensure_ducklake_installed() {
+    INSTALL_DUCKLAKE.call_once(|| {
+        let conn = duckdb::Connection::open_in_memory().expect("open in-memory duckdb");
+        conn.execute("INSTALL ducklake;", [])
+            .expect("install ducklake extension");
+    });
+}
 
 /// Creates a catalog with a simple table (no deletes)
 ///
@@ -29,7 +41,7 @@ pub fn create_catalog_no_deletes(catalog_path: &Path) -> Result<()> {
     // Use in-memory database to avoid file locking issues
     let conn = duckdb::Connection::open_in_memory()?;
 
-    conn.execute("INSTALL ducklake;", [])?;
+    ensure_ducklake_installed();
     conn.execute("LOAD ducklake;", [])?;
 
     let ducklake_path = format!("ducklake:{}", catalog_path.display());
@@ -69,7 +81,7 @@ pub fn create_catalog_with_deletes(catalog_path: &Path) -> Result<()> {
     // Use in-memory database to avoid file locking issues
     let conn = duckdb::Connection::open_in_memory()?;
 
-    conn.execute("INSTALL ducklake;", [])?;
+    ensure_ducklake_installed();
     conn.execute("LOAD ducklake;", [])?;
 
     let ducklake_path = format!("ducklake:{}", catalog_path.display());
@@ -115,7 +127,7 @@ pub fn create_catalog_with_updates(catalog_path: &Path) -> Result<()> {
     // Use in-memory database to avoid file locking issues
     let conn = duckdb::Connection::open_in_memory()?;
 
-    conn.execute("INSTALL ducklake;", [])?;
+    ensure_ducklake_installed();
     conn.execute("LOAD ducklake;", [])?;
 
     let ducklake_path = format!("ducklake:{}", catalog_path.display());
@@ -170,7 +182,7 @@ pub fn create_catalog_filter_pushdown(catalog_path: &Path) -> Result<()> {
     // Use in-memory database to avoid file locking issues
     let conn = duckdb::Connection::open_in_memory()?;
 
-    conn.execute("INSTALL ducklake;", [])?;
+    ensure_ducklake_installed();
     conn.execute("LOAD ducklake;", [])?;
 
     let ducklake_path = format!("ducklake:{}", catalog_path.display());
@@ -208,7 +220,7 @@ pub fn create_catalog_filter_pushdown(catalog_path: &Path) -> Result<()> {
 pub fn create_catalog_empty_table(catalog_path: &Path) -> Result<()> {
     let conn = duckdb::Connection::open_in_memory()?;
 
-    conn.execute("INSTALL ducklake;", [])?;
+    ensure_ducklake_installed();
     conn.execute("LOAD ducklake;", [])?;
 
     let ducklake_path = format!("ducklake:{}", catalog_path.display());
@@ -238,7 +250,7 @@ pub fn create_catalog_empty_table(catalog_path: &Path) -> Result<()> {
 pub fn create_catalog_basic_test(catalog_path: &Path) -> Result<()> {
     let conn = duckdb::Connection::open_in_memory()?;
 
-    conn.execute("INSTALL ducklake;", [])?;
+    ensure_ducklake_installed();
     conn.execute("LOAD ducklake;", [])?;
 
     let ducklake_path = format!("ducklake:{}", catalog_path.display());
@@ -290,7 +302,7 @@ pub fn to_datafusion_error(e: anyhow::Error) -> datafusion::error::DataFusionErr
 pub fn create_catalog_complex_deletions(catalog_path: &Path) -> Result<()> {
     let conn = duckdb::Connection::open_in_memory()?;
 
-    conn.execute("INSTALL ducklake;", [])?;
+    ensure_ducklake_installed();
     conn.execute("LOAD ducklake;", [])?;
 
     let ducklake_path = format!("ducklake:{}", catalog_path.display());
@@ -336,7 +348,7 @@ pub fn create_catalog_complex_deletions(catalog_path: &Path) -> Result<()> {
 pub fn create_catalog_multiple_snapshots(catalog_path: &Path) -> Result<()> {
     let conn = duckdb::Connection::open_in_memory()?;
 
-    conn.execute("INSTALL ducklake;", [])?;
+    ensure_ducklake_installed();
     conn.execute("LOAD ducklake;", [])?;
 
     let ducklake_path = format!("ducklake:{}", catalog_path.display());