resolve some minor comments

src/common/src/array/map_array.rs

@@ -74,8 +74,22 @@ impl ArrayBuilder for MapArrayBuilder {
     }
 }
 
-/// `MapArray` is physically just a `List<Struct<key: K, value: V>>` array, but with some additional restrictions
-/// on the `key`.
+/// `MapArray` is physically just a `List<Struct<key: K, value: V>>` array, but with some additional restrictions.
+///
+/// Type:
+/// - `key`'s datatype can only be string & integral types. (See [`MapType::assert_key_type_valid`].)
+/// - `value` can be any type.
+///
+/// Value (for each map value in the array):
+/// - `key`s are non-null and unique.
+/// - `key`s and `value`s must be of the same length.
+///   For a `MapArray`, it's sliced by the `ListArray`'s offsets, so it essentially means the
+///   `key` and `value` children arrays have the same length.
+/// - The lists are not sorted by `key`.
+/// - Map values are not comparable.
+///   And the map type should not be used as (primary/group/join/order) keys.
+///   Such usages should be banned in the frontend, and the implementation of `PartialEq`, `Ord` etc. are `unreachable!()`. (See [`cmp`].)
+///   Note that this decision is not definitive. Just be conservative at the beginning.
 #[derive(Debug, Clone, Eq)]
 pub struct MapArray {
     pub(super) inner: ListArray,
@@ -141,6 +155,8 @@ impl MapArray {
         self.inner.offsets()
     }
 }
+
+/// Refer to [`MapArray`] for the invariants of a map value.
 #[derive(Clone, Eq, EstimateSize)]
 pub struct MapValue(pub(crate) ListValue);
 
@@ -240,6 +256,8 @@ impl MapValue {
 
 /// A map is just a slice of the underlying struct array.
 ///
+/// Refer to [`MapArray`] for the invariants of a map value.
+///
 /// XXX: perhaps we can make it `MapRef<'a, 'b>(ListRef<'a>, ListRef<'b>);`.
 /// Then we can build a map ref from 2 list refs without copying the data.
 /// Currently it's impossible.

src/common/src/array/mod.rs

@@ -138,11 +138,11 @@ pub trait ArrayBuilder: Send + Sync + Sized + 'static {
 
     /// Pop an element from the builder.
     ///
+    /// It's used in `rollback` in source parser.
+    ///
     /// # Returns
     ///
     /// Returns `None` if there is no elements in the builder.
-    ///
-    /// XXX: This seems useless. Perhaps we can delete it.
     fn pop(&mut self) -> Option<()>;
 
     /// Append an element in another array into builder.

src/common/src/hash/key.rs

@@ -627,6 +627,8 @@ impl_value_encoding_hash_key_serde!(JsonbVal);
 // use the memcmp encoding for safety.
 impl_memcmp_encoding_hash_key_serde!(StructValue);
 impl_memcmp_encoding_hash_key_serde!(ListValue);
+// Note: Map should not be used as key. It's memcmp_encoding is just unreachable!().
+// We are also effectively banning it here.
 impl_memcmp_encoding_hash_key_serde!(MapValue);
 
 #[cfg(test)]

src/common/src/types/map_type.rs

@@ -18,6 +18,7 @@ use anyhow::*;
 
 use super::*;
 
+/// Refer to [`super::super::array::MapArray`] for the invariants of a map value.
 #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct MapType(Box<(DataType, DataType)>);
 
@@ -73,6 +74,13 @@ impl MapType {
         DataType::List(Box::new(DataType::Struct(self.into_struct())))
     }
 
+    /// String and integral types are allowed.
+    ///
+    /// This is similar to [Protobuf](https://protobuf.dev/programming-guides/proto3/#maps)'s
+    /// decision.
+    ///
+    /// Note that this isn't definitive.
+    /// Just be conservative at the beginning, but not too restrictive (like only allowing strings).
     pub fn assert_key_type_valid(data_type: &DataType) {
         let valid = match data_type {
             DataType::Int16 | DataType::Int32 | DataType::Int64 => true,

src/common/src/types/mod.rs

@@ -208,7 +208,7 @@ impl TryFrom<DataTypeName> for DataType {
             DataTypeName::Interval => Ok(DataType::Interval),
             DataTypeName::Jsonb => Ok(DataType::Jsonb),
             DataTypeName::Struct | DataTypeName::List | DataTypeName::Map => {
-                Err("Functions returning struct or list can not be inferred. Please use `FunctionCall::new_unchecked`.")
+                Err("Functions returning composite types can not be inferred. Please use `FunctionCall::new_unchecked`.")
             }
         }
     }