Handle code-reviews

core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java

@@ -881,7 +881,7 @@ Builder populate2() {
       map.put(MAP_VALUE_CONSTRUCTOR, value);
       map.put(ARRAY_VALUE_CONSTRUCTOR, value);
       defineMethod(ARRAY, BuiltInMethod.ARRAYS_AS_LIST.method, NullPolicy.NONE);
-      defineMethod(MAP, BuiltInMethod.MAP_FUNCTION.method, NullPolicy.NONE);
+      defineMethod(MAP, BuiltInMethod.MAP.method, NullPolicy.NONE);
 
       // ITEM operator
       map.put(ITEM, new ItemImplementor());

core/src/main/java/org/apache/calcite/prepare/LixToRelTranslator.java

@@ -97,7 +97,7 @@ public <T> RelNode translate(Queryable<T> queryable) {
   public RelNode translate(Expression expression) {
     if (expression instanceof MethodCallExpression) {
       final MethodCallExpression call = (MethodCallExpression) expression;
-      BuiltInMethod method = BuiltInMethod.MAP.get(call.method);
+      BuiltInMethod method = BuiltInMethod.FUNCTIONS_MAPS.get(call.method);
       if (method == null) {
         throw new UnsupportedOperationException(
             "unknown method " + call.method);

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

@@ -5323,8 +5323,11 @@ public static Map mapFromArrays(List keysArray, List valuesArray) {
     return map;
   }
 
-  /** Support the MAP function. */
-  public static Map mapFunction(Object... args) {
+  /** Support the MAP function.
+   *
+   * <p>odd-indexed elements are keys and even-indexed elements are values.
+   */
+  public static Map map(Object... args) {
     final Map map = new LinkedHashMap<>();
     for (int i = 0; i < args.length; i++) {
       Object key = args[i++];

core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java

@@ -1099,9 +1099,15 @@ private static RelDataType mapReturnType(SqlOperatorBinding opBinding) {
       RelDataTypeFactory typeFactory,
       List<RelDataType> argTypes) {
     // special case, allows empty map
-    if (argTypes.size() == 0) {
+    if (argTypes.isEmpty()) {
       return Pair.of(typeFactory.createUnknownType(), typeFactory.createUnknownType());
     }
+    // Util.quotientList(argTypes, 2, 0):
+    // This extracts all elements at even indices from argTypes.
+    // It represents the types of keys in the map as they are placed at even positions
+    // e.g. 0, 2, 4, etc.
+    // Symmetrically, Util.quotientList(argTypes, 2, 1) represents odd-indexed elements.
+    // details please see Util.quotientList.
     return Pair.of(
         typeFactory.leastRestrictive(Util.quotientList(argTypes, 2, 0)),
         typeFactory.leastRestrictive(Util.quotientList(argTypes, 2, 1)));

core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java

@@ -1232,6 +1232,9 @@ private static class MapFunctionOperandTypeChecker
       extends SameOperandTypeChecker {
 
     MapFunctionOperandTypeChecker() {
+      // The args of map are non-fixed, so we set to -1 here. then operandCount
+      // can dynamically set according to the number of input args.
+      // details please see SameOperandTypeChecker#getOperandList.
       super(-1);
     }
 
@@ -1240,11 +1243,11 @@ private static class MapFunctionOperandTypeChecker
       final List<RelDataType> argTypes =
           SqlTypeUtil.deriveType(callBinding, callBinding.operands());
       // allows empty map
-      if (argTypes.size() == 0) {
+      if (argTypes.isEmpty()) {
         return true;
       }
       // the size of map arg types must be even.
-      if (argTypes.size() % 2 > 0) {
+      if (argTypes.size() % 2 != 0) {
         throw callBinding.newValidationError(RESOURCE.mapRequiresEvenArgCount());
       }
       final Pair<@Nullable RelDataType, @Nullable RelDataType> componentType =
@@ -1266,6 +1269,12 @@ private static class MapFunctionOperandTypeChecker
     private static Pair<@Nullable RelDataType, @Nullable RelDataType> getComponentTypes(
         RelDataTypeFactory typeFactory,
         List<RelDataType> argTypes) {
+      // Util.quotientList(argTypes, 2, 0):
+      // This extracts all elements at even indices from argTypes.
+      // It represents the types of keys in the map as they are placed at even positions
+      // e.g. 0, 2, 4, etc.
+      // Symmetrically, Util.quotientList(argTypes, 2, 1) represents odd-indexed elements.
+      // details please see Util.quotientList.
       return Pair.of(
           typeFactory.leastRestrictive(Util.quotientList(argTypes, 2, 0)),
           typeFactory.leastRestrictive(Util.quotientList(argTypes, 2, 1)));

core/src/main/java/org/apache/calcite/util/BuiltInMethod.java

@@ -768,13 +768,13 @@ public enum BuiltInMethod {
   ARRAYS_OVERLAP(SqlFunctions.class, "arraysOverlap", List.class, List.class),
   ARRAYS_ZIP(SqlFunctions.class, "arraysZip", List.class, List.class),
   SORT_ARRAY(SqlFunctions.class, "sortArray", List.class, boolean.class),
+  MAP(SqlFunctions.class, "map", Object[].class),
   MAP_CONCAT(SqlFunctions.class, "mapConcat", Map[].class),
   MAP_ENTRIES(SqlFunctions.class, "mapEntries", Map.class),
   MAP_KEYS(SqlFunctions.class, "mapKeys", Map.class),
   MAP_VALUES(SqlFunctions.class, "mapValues", Map.class),
   MAP_FROM_ARRAYS(SqlFunctions.class, "mapFromArrays", List.class, List.class),
   MAP_FROM_ENTRIES(SqlFunctions.class, "mapFromEntries", List.class),
-  MAP_FUNCTION(SqlFunctions.class, "mapFunction", Object[].class),
   STR_TO_MAP(SqlFunctions.class, "strToMap", String.class, String.class, String.class),
   SELECTIVITY(Selectivity.class, "getSelectivity", RexNode.class),
   UNIQUE_KEYS(UniqueKeys.class, "getUniqueKeys", boolean.class),
@@ -851,7 +851,7 @@ public enum BuiltInMethod {
   @SuppressWarnings("ImmutableEnumChecker")
   public final Field field;
 
-  public static final ImmutableMap<Method, BuiltInMethod> MAP;
+  public static final ImmutableMap<Method, BuiltInMethod> FUNCTIONS_MAPS;
 
   static {
     final ImmutableMap.Builder<Method, BuiltInMethod> builder =
@@ -861,7 +861,7 @@ public enum BuiltInMethod {
         builder.put(value.method, value);
       }
     }
-    MAP = builder.build();
+    FUNCTIONS_MAPS = builder.build();
   }
 
   BuiltInMethod(@Nullable Method method, @Nullable Constructor constructor, @Nullable Field field) {

site/_docs/reference.md

@@ -2770,7 +2770,8 @@ BigQuery's type system uses confusingly different names for types and functions:
 | b | TO_HEX(binary)                                 | Converts *binary* into a hexadecimal varchar
 | b | FROM_HEX(varchar)                              | Converts a hexadecimal-encoded *varchar* into bytes
 | b o | LTRIM(string)                                | Returns *string* with all blanks removed from the start
-| s | MAP(key, value [, key, value]*)                | Returns a map with the given key/value pairs
+| s | MAP()                                          | Returns an empty map
+| s | MAP(key, value [, key, value]*)                | Returns a map with the given *key*/*value* pairs
 | s | MAP_CONCAT(map [, map]*)                       | Concatenates one or more maps. If any input argument is `NULL` the function returns `NULL`. Note that calcite is using the LAST_WIN strategy
 | s | MAP_ENTRIES(map)                               | Returns the entries of the *map* as an array, the order of the entries is not defined
 | s | MAP_KEYS(map)                                  | Returns the keys of the *map* as an array, the order of the entries is not defined

testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java

@@ -6148,10 +6148,13 @@ private static Matcher<SqlNode> isCharLiteral(String s) {
 
   @Test void testMapFunction() {
     expr("map()").ok("MAP()");
+    expr("MAP()").same();
     // parser allows odd elements; validator will reject it
     expr("map(1)").ok("MAP(1)");
     expr("map(1, 'x', 2, 'y')")
         .ok("MAP(1, 'x', 2, 'y')");
+    // with upper case
+    expr("MAP(1, 'x', 2, 'y')").same();
     // with space
     expr("map (1, 'x', 2, 'y')")
         .ok("MAP(1, 'x', 2, 'y')");
@@ -6163,6 +6166,12 @@ private static Matcher<SqlNode> isCharLiteral(String s) {
         .ok("SELECT (MAP ((SELECT 1)))");
     sql("SELECT map(SELECT 1, 2)")
         .ok("SELECT (MAP ((SELECT 1, 2)))");
+    // with upper case
+    sql("SELECT MAP(SELECT 1, 2)")
+        .ok("SELECT (MAP ((SELECT 1, 2)))");
+    // with space
+    sql("SELECT map (SELECT 1, 2)")
+        .ok("SELECT (MAP ((SELECT 1, 2)))");
     sql("SELECT map(SELECT T.x, T.y FROM (VALUES(1, 2)) AS T(x, y))")
         .ok("SELECT (MAP ((SELECT `T`.`X`, `T`.`Y`\n"
             + "FROM (VALUES (ROW(1, 2))) AS `T` (`X`, `Y`))))");

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

@@ -6780,19 +6780,17 @@ private static void checkIf(SqlOperatorFixture f) {
         "(NULL, INTEGER NOT NULL) MAP NOT NULL");
     f1.checkScalar("map_concat(map(1, 2), map(1, null))", "{1=null}",
         "(INTEGER NOT NULL, INTEGER) MAP NOT NULL");
-    // test zero arg, but it should return empty map.
-    f1.checkScalar("map_concat()", "{}",
-        "(VARCHAR NOT NULL, VARCHAR NOT NULL) MAP");
+    f1.checkScalar("map_concat(map('foo', 1), map())", "{foo=1}",
+        "(UNKNOWN NOT NULL, UNKNOWN NOT NULL) MAP NOT NULL");
+
+    // test operand is null map
+    f1.checkNull("map_concat(map('foo', 1), cast(null as map<varchar, int>))");
+    f1.checkType("map_concat(map('foo', 1), cast(null as map<varchar, int>))",
+        "(VARCHAR NOT NULL, INTEGER NOT NULL) MAP");
+    f1.checkNull("map_concat(cast(null as map<varchar, int>), map['foo', 1])");
+    f1.checkType("map_concat(cast(null as map<varchar, int>), map['foo', 1])",
+        "(VARCHAR NOT NULL, INTEGER NOT NULL) MAP");
 
-    // after calcite supports cast(null as map<string, int>), it should add these tests.
-    if (TODO) {
-      f1.checkNull("map_concat(map('foo', 1), cast(null as map<string, int>))");
-      f1.checkType("map_concat(map('foo', 1), cast(null as map<string, int>))",
-          "(VARCHAR NOT NULL, INTEGER NOT NULL) MAP");
-      f1.checkNull("map_concat(cast(null as map<string, int>), map['foo', 1])");
-      f1.checkType("map_concat(cast(null as map<string, int>), map['foo', 1])",
-          "(VARCHAR NOT NULL, INTEGER NOT NULL) MAP");
-    }
     f1.checkFails("^map_concat(map('foo', 1), null)^",
         "Function 'MAP_CONCAT' should all be of type map, "
             + "but it is 'NULL'", false);
@@ -6835,6 +6833,8 @@ private static void checkIf(SqlOperatorFixture f) {
     final SqlOperatorFixture f1 = fixture()
         .setFor(SqlLibraryOperators.MAP_ENTRIES)
         .withLibrary(SqlLibrary.SPARK);
+    f1.checkScalar("map_entries(map())", "[]",
+        "RecordType(UNKNOWN NOT NULL f0, UNKNOWN NOT NULL f1) NOT NULL ARRAY NOT NULL");
     f1.checkScalar("map_entries(map('foo', 1, 'bar', 2))", "[{foo, 1}, {bar, 2}]",
         "RecordType(CHAR(3) NOT NULL f0, INTEGER NOT NULL f1) NOT NULL ARRAY NOT NULL");
     f1.checkScalar("map_entries(map('foo', 1, null, 2))", "[{foo, 1}, {null, 2}]",
@@ -6874,6 +6874,8 @@ private static void checkIf(SqlOperatorFixture f) {
     final SqlOperatorFixture f1 = fixture()
         .setFor(SqlLibraryOperators.MAP_KEYS)
         .withLibrary(SqlLibrary.SPARK);
+    f1.checkScalar("map_keys(map())", "[]",
+        "UNKNOWN NOT NULL ARRAY NOT NULL");
     f1.checkScalar("map_keys(map('foo', 1, 'bar', 2))", "[foo, bar]",
         "CHAR(3) NOT NULL ARRAY NOT NULL");
     f1.checkScalar("map_keys(map('foo', 1, null, 2))", "[foo, null]",
@@ -6898,6 +6900,8 @@ private static void checkIf(SqlOperatorFixture f) {
     final SqlOperatorFixture f1 = fixture()
         .setFor(SqlLibraryOperators.MAP_VALUES)
         .withLibrary(SqlLibrary.SPARK);
+    f1.checkScalar("map_values(map())", "[]",
+        "UNKNOWN NOT NULL ARRAY NOT NULL");
     f1.checkScalar("map_values(map('foo', 1, 'bar', 2))", "[1, 2]",
         "INTEGER NOT NULL ARRAY NOT NULL");
     f1.checkScalar("map_values(map('foo', 1, 'bar', cast(null as integer)))", "[1, null]",