chore: Clean up

Author: Weijun-H

datafusion/functions-nested/src/map.rs

@@ -486,3 +486,129 @@ fn make_map_array_internal<O: OffsetSizeTrait>(
     let map_data = map_data_builder.build()?;
     Ok(ColumnarValue::Array(Arc::new(MapArray::from(map_data))))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use arrow::array::{ListArray, Int32Array, StringArray};
+    use arrow::datatypes::Field;
+
+    #[test]
+    fn test_make_map_with_null_maps() {
+        // Test that NULL map values (entire map is NULL) are correctly handled
+        // This test directly calls make_map_batch with a List containing NULL elements
+        // On main branch: Would fail at line 66 with "map key cannot be null"
+        // With fix: Correctly handles NULL maps by routing to make_map_array_internal
+
+        // Create a List<List<String>> where one element is NULL (representing a NULL map)
+        // keys = [['a'], NULL, ['b']]
+        let key_values: Vec<Option<Vec<Option<&str>>>> = vec![
+            Some(vec![Some("a")]),
+            None,  // This is a NULL map, not a null key
+            Some(vec![Some("b")]),
+        ];
+
+        let mut key_builder = arrow::array::ListBuilder::new(
+            arrow::array::StringBuilder::new()
+        );
+
+        for key_list in key_values {
+            match key_list {
+                Some(keys) => {
+                    for key in keys {
+                        key_builder.values().append_option(key);
+                    }
+                    key_builder.append(true);
+                }
+                None => {
+                    key_builder.append(false);  // NULL map
+                }
+            }
+        }
+        let keys_array = Arc::new(key_builder.finish());
+
+        // Create corresponding values = [[1], [2], [3]]
+        let value_values: Vec<Option<Vec<Option<i32>>>> = vec![
+            Some(vec![Some(1)]),
+            Some(vec![Some(2)]),
+            Some(vec![Some(3)]),
+        ];
+
+        let mut value_builder = arrow::array::ListBuilder::new(
+            arrow::array::Int32Builder::new()
+        );
+
+        for value_list in value_values {
+            match value_list {
+                Some(values) => {
+                    for value in values {
+                        value_builder.values().append_option(value);
+                    }
+                    value_builder.append(true);
+                }
+                None => {
+                    value_builder.append(false);
+                }
+            }
+        }
+        let values_array = Arc::new(value_builder.finish());
+
+        // Call make_map_batch - this should succeed with the fix
+        let result = make_map_batch(&[
+            ColumnarValue::Array(keys_array),
+            ColumnarValue::Array(values_array),
+        ]);
+
+        assert!(result.is_ok(), "Should handle NULL maps correctly");
+
+        if let Ok(ColumnarValue::Array(map_array)) = result {
+            assert_eq!(map_array.len(), 3);
+            assert!(map_array.is_null(1), "Second map should be NULL");
+            assert!(!map_array.is_null(0), "First map should not be NULL");
+            assert!(!map_array.is_null(2), "Third map should not be NULL");
+        } else {
+            panic!("Expected Array result");
+        }
+    }
+
+    #[test]
+    fn test_make_map_with_null_key_within_map_should_fail() {
+        // Test that null keys WITHIN a map are properly rejected
+        // keys = [['a', NULL, 'b']]  -- NULL here is a null key, should fail
+        let mut key_builder = arrow::array::ListBuilder::new(
+            arrow::array::StringBuilder::new()
+        );
+
+        key_builder.values().append_value("a");
+        key_builder.values().append_null();  // NULL key within the map
+        key_builder.values().append_value("b");
+        key_builder.append(true);
+
+        let keys_array = Arc::new(key_builder.finish());
+
+        // values = [[1, 2, 3]]
+        let mut value_builder = arrow::array::ListBuilder::new(
+            arrow::array::Int32Builder::new()
+        );
+        value_builder.values().append_value(1);
+        value_builder.values().append_value(2);
+        value_builder.values().append_value(3);
+        value_builder.append(true);
+
+        let values_array = Arc::new(value_builder.finish());
+
+        // Call make_map_batch - this should fail
+        let result = make_map_batch(&[
+            ColumnarValue::Array(keys_array),
+            ColumnarValue::Array(values_array),
+        ]);
+
+        assert!(result.is_err(), "Should reject null keys within maps");
+        let err = result.unwrap_err();
+        assert!(
+            err.to_string().contains("cannot be null"),
+            "Error should mention null keys, got: {}",
+            err
+        );
+    }
+}

datafusion/sqllogictest/test_files/map.slt

@@ -854,88 +854,3 @@ NULL
 
 statement ok
 drop table tt;
-
-# Test for issue with NULL map values
-# Memory table tests
-statement ok
-CREATE TABLE test_null_map AS VALUES
-  ('1', MAP {'a': 'ab'}),
-  ('2', NULL),
-  ('3', MAP {'b': 'cd'});
-
-query T?
-SELECT * FROM test_null_map ORDER BY column1;
-----
-1 {a: ab}
-2 NULL
-3 {b: cd}
-
-query ?
-SELECT map_keys(column2) FROM test_null_map ORDER BY column1;
-----
-[a]
-NULL
-[b]
-
-query ?
-SELECT map_values(column2) FROM test_null_map ORDER BY column1;
-----
-[ab]
-NULL
-[cd]
-
-# Test with map_entries (this internally calls make_map)
-query ?
-SELECT map_entries(column2) FROM test_null_map ORDER BY column1;
-----
-[{key: a, value: ab}]
-NULL
-[{key: b, value: cd}]
-
-statement ok
-DROP TABLE test_null_map;
-
-# Parquet storage tests
-# This reproduces the issue where NULL map values cause "map key cannot be null" error
-statement ok
-CREATE TABLE test_map_parquet_mixed_temp AS VALUES
-  ('1', MAP {'a': 'ab'}),
-  ('2', NULL),
-  ('3', MAP {'c': 'cd'});
-
-statement ok
-COPY test_map_parquet_mixed_temp TO 'test_files/scratch/test_map_parquet_mixed.parquet' STORED AS PARQUET;
-
-statement ok
-DROP TABLE test_map_parquet_mixed_temp;
-
-statement ok
-CREATE EXTERNAL TABLE test_map_parquet_mixed
-STORED AS PARQUET
-LOCATION 'test_files/scratch/test_map_parquet_mixed.parquet';
-
-query T?
-SELECT * FROM test_map_parquet_mixed ORDER BY column1;
-----
-1 {a: ab}
-2 NULL
-3 {c: cd}
-
-# Test map operations on NULL maps from Parquet
-query ?
-SELECT map_keys(column2) FROM test_map_parquet_mixed ORDER BY column1;
-----
-[a]
-NULL
-[c]
-
-query ?
-SELECT map_values(column2) FROM test_map_parquet_mixed ORDER BY column1;
-----
-[ab]
-NULL
-[cd]
-
-statement ok
-DROP TABLE test_map_parquet_mixed;
-