From 3ae9c56f51995474b621a4162bf4d7f5d180a1ee Mon Sep 17 00:00:00 2001 From: Ran Tao Date: Sun, 8 Oct 2023 20:01:35 +0800 Subject: [PATCH] Handle code-reviews --- .../adapter/enumerable/RexImpTable.java | 2 +- .../calcite/prepare/LixToRelTranslator.java | 2 +- .../apache/calcite/runtime/SqlFunctions.java | 7 +++-- .../calcite/sql/fun/SqlLibraryOperators.java | 8 +++++- .../apache/calcite/sql/type/OperandTypes.java | 13 +++++++-- .../apache/calcite/util/BuiltInMethod.java | 6 ++-- site/_docs/reference.md | 3 +- .../calcite/sql/parser/SqlParserTest.java | 9 ++++++ .../apache/calcite/test/SqlOperatorTest.java | 28 +++++++++++-------- 9 files changed, 55 insertions(+), 23 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java index 666a7c22a338..f951ba9a3748 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java +++ b/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()); diff --git a/core/src/main/java/org/apache/calcite/prepare/LixToRelTranslator.java b/core/src/main/java/org/apache/calcite/prepare/LixToRelTranslator.java index 2bd4eb0384fe..9d676a36fc52 100644 --- a/core/src/main/java/org/apache/calcite/prepare/LixToRelTranslator.java +++ b/core/src/main/java/org/apache/calcite/prepare/LixToRelTranslator.java @@ -97,7 +97,7 @@ public RelNode translate(Queryable 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); diff --git a/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java b/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java index f14efee9123a..a287b3d9ed28 100644 --- a/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java +++ b/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. + * + *

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++]; diff --git a/core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java b/core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java index 7e1d8bb426b5..5f8b8df06abf 100644 --- a/core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java +++ b/core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java @@ -1099,9 +1099,15 @@ private static RelDataType mapReturnType(SqlOperatorBinding opBinding) { RelDataTypeFactory typeFactory, List 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))); diff --git a/core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java b/core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java index 39132224d2d4..295c95e0f330 100644 --- a/core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java +++ b/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 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 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))); diff --git a/core/src/main/java/org/apache/calcite/util/BuiltInMethod.java b/core/src/main/java/org/apache/calcite/util/BuiltInMethod.java index 060d1bdef7d2..c5e6986aabc7 100644 --- a/core/src/main/java/org/apache/calcite/util/BuiltInMethod.java +++ b/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 MAP; + public static final ImmutableMap FUNCTIONS_MAPS; static { final ImmutableMap.Builder 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) { diff --git a/site/_docs/reference.md b/site/_docs/reference.md index 33332554df19..0d61f205ffc1 100644 --- a/site/_docs/reference.md +++ b/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 diff --git a/testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java b/testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java index 4e35f50f9743..14af8dbd702e 100644 --- a/testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java +++ b/testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java @@ -6148,10 +6148,13 @@ private static Matcher 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 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`))))"); diff --git a/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java b/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java index fe17c30e446e..29d311153888 100644 --- a/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java +++ b/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))"); + f1.checkType("map_concat(map('foo', 1), cast(null as map))", + "(VARCHAR NOT NULL, INTEGER NOT NULL) MAP"); + f1.checkNull("map_concat(cast(null as map), map['foo', 1])"); + f1.checkType("map_concat(cast(null as map), map['foo', 1])", + "(VARCHAR NOT NULL, INTEGER NOT NULL) MAP"); - // after calcite supports cast(null as map), it should add these tests. - if (TODO) { - f1.checkNull("map_concat(map('foo', 1), cast(null as map))"); - f1.checkType("map_concat(map('foo', 1), cast(null as map))", - "(VARCHAR NOT NULL, INTEGER NOT NULL) MAP"); - f1.checkNull("map_concat(cast(null as map), map['foo', 1])"); - f1.checkType("map_concat(cast(null as map), 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]",