tanclary commented on code in PR #3459:
URL: https://github.com/apache/calcite/pull/3459#discussion_r1357038522
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -5251,6 +5251,17 @@ public static Map mapFromArrays(List keysArray, List
valuesArray) {
return map;
}
+ /** Support the MAP function. */
+ public static Map mapFunction(Object... args) {
Review Comment:
same comment as above
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -873,6 +874,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);
Review Comment:
should we call it just MAP to be consistent with other functions?
##########
core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java:
##########
@@ -1221,6 +1225,53 @@ private static class MapFromEntriesOperandTypeChecker
}
}
+ /**
+ * Operand type-checking strategy for a MAP function, it allows empty map.
+ */
+ private static class MapFunctionOperandTypeChecker
+ extends SameOperandTypeChecker {
+
+ MapFunctionOperandTypeChecker() {
+ super(-1);
Review Comment:
where does the -1 come from?
##########
core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java:
##########
@@ -1221,6 +1225,53 @@ private static class MapFromEntriesOperandTypeChecker
}
}
+ /**
+ * Operand type-checking strategy for a MAP function, it allows empty map.
+ */
+ private static class MapFunctionOperandTypeChecker
+ extends SameOperandTypeChecker {
+
+ MapFunctionOperandTypeChecker() {
+ super(-1);
+ }
+
+ @Override public boolean checkOperandTypes(final SqlCallBinding
callBinding,
+ final boolean throwOnFailure) {
+ final List<RelDataType> argTypes =
+ SqlTypeUtil.deriveType(callBinding, callBinding.operands());
+ // allows empty map
+ if (argTypes.size() == 0) {
Review Comment:
`isEmpty()`
##########
core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java:
##########
@@ -568,6 +569,9 @@ public static SqlOperandTypeChecker variadic(
public static final SqlSingleOperandTypeChecker MAP_FROM_ENTRIES =
new MapFromEntriesOperandTypeChecker();
+ public static final SqlSingleOperandTypeChecker MAP_FUNCTION =
Review Comment:
same naming comment as above
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -5251,6 +5251,17 @@ public static Map mapFromArrays(List keysArray, List
valuesArray) {
return map;
}
+ /** Support the MAP function. */
+ public static Map mapFunction(Object... args) {
+ final Map map = new LinkedHashMap<>();
+ for (int i = 0; i < args.length; i++) {
+ Object key = args[i++];
Review Comment:
Could be helpful to add a comment explaining the odd elements are keys and
the even elements are values.
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -1082,6 +1084,38 @@ private static RelDataType
arrayReturnType(SqlOperatorBinding opBinding) {
SqlLibraryOperators::arrayReturnType,
OperandTypes.SAME_VARIADIC);
+ private static RelDataType mapReturnType(SqlOperatorBinding opBinding) {
+ Pair<@Nullable RelDataType, @Nullable RelDataType> type =
+ getComponentTypes(
+ opBinding.getTypeFactory(), opBinding.collectOperandTypes());
+ return SqlTypeUtil.createMapType(
+ opBinding.getTypeFactory(),
+ requireNonNull(type.left, "inferred key type"),
+ requireNonNull(type.right, "inferred value type"),
+ false);
+ }
+
+ private static Pair<@Nullable RelDataType, @Nullable RelDataType>
getComponentTypes(
+ RelDataTypeFactory typeFactory,
+ List<RelDataType> argTypes) {
+ // special case, allows empty map
+ if (argTypes.size() == 0) {
Review Comment:
should we use `isEmpty()` instead?
##########
core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java:
##########
@@ -1221,6 +1225,53 @@ private static class MapFromEntriesOperandTypeChecker
}
}
+ /**
+ * Operand type-checking strategy for a MAP function, it allows empty map.
+ */
+ private static class MapFunctionOperandTypeChecker
+ extends SameOperandTypeChecker {
+
+ MapFunctionOperandTypeChecker() {
+ super(-1);
+ }
+
+ @Override public boolean checkOperandTypes(final SqlCallBinding
callBinding,
+ final boolean throwOnFailure) {
+ final List<RelDataType> argTypes =
+ SqlTypeUtil.deriveType(callBinding, callBinding.operands());
+ // allows empty map
+ if (argTypes.size() == 0) {
+ return true;
+ }
+ // the size of map arg types must be even.
+ if (argTypes.size() % 2 > 0) {
Review Comment:
is the condition more clear if we say `argTypes.size() % 2 != 0`
##########
site/_docs/reference.md:
##########
@@ -2769,6 +2769,7 @@ 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
Review Comment:
Also when referencing the function arguments please surround them with * to
remain consistent with other functions
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6703,11 +6704,53 @@ private static void checkIf(SqlOperatorFixture f) {
// test operands not in same type family.
f.checkFails("^map_concat(map[1, null], array[1])^",
"Parameters must be of the same type", false);
+
+ // 2. check with map function, map(k, v ...)
+ final SqlOperatorFixture f1 = fixture()
+ .setFor(SqlLibraryOperators.MAP_CONCAT)
+ .withLibrary(SqlLibrary.SPARK);
+ f1.checkScalar("map_concat(map('foo', 1), map('bar', 2))", "{foo=1,
bar=2}",
+ "(CHAR(3) NOT NULL, INTEGER NOT NULL) MAP NOT NULL");
+ f1.checkScalar("map_concat(map('foo', 1), map('bar', 2), map('foo', 2))",
"{foo=2, bar=2}",
+ "(CHAR(3) NOT NULL, INTEGER NOT NULL) MAP NOT NULL");
+ f1.checkScalar("map_concat(map(null, 1), map(null, 2))", "{null=2}",
+ "(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");
+
+ // test concat with empty map, in fact, spark will return MAP(CHAR,
INTEGER), because
+ // it will add a cast 'cast(map() as map<string, int>)' to convert UNKNOWN
type,
+ // but currently calcite not support cast to map type.
+ f1.checkScalar("map_concat(map('foo', 1), map())", "{foo=1}",
+ "(UNKNOWN NOT NULL, UNKNOWN NOT NULL) MAP NOT NULL");
+
+ // after calcite supports cast(null as map<string, int>), cast(map() as
map<string, int>)
+ // it should add these tests.
+ if (TODO) {
Review Comment:
instead of `if (TODO)` would it be better to add an enum to the `Bug` class?
Here is an example of what I mean:
https://github.com/apache/calcite/blob/main/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java#L624
##########
core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java:
##########
@@ -1221,6 +1225,53 @@ private static class MapFromEntriesOperandTypeChecker
}
}
+ /**
+ * Operand type-checking strategy for a MAP function, it allows empty map.
+ */
+ private static class MapFunctionOperandTypeChecker
+ extends SameOperandTypeChecker {
+
+ MapFunctionOperandTypeChecker() {
+ super(-1);
+ }
+
+ @Override public boolean checkOperandTypes(final SqlCallBinding
callBinding,
+ final boolean throwOnFailure) {
+ final List<RelDataType> argTypes =
+ SqlTypeUtil.deriveType(callBinding, callBinding.operands());
+ // allows empty map
+ if (argTypes.size() == 0) {
+ return true;
+ }
+ // the size of map arg types must be even.
+ if (argTypes.size() % 2 > 0) {
+ throw
callBinding.newValidationError(RESOURCE.mapRequiresEvenArgCount());
+ }
+ final Pair<@Nullable RelDataType, @Nullable RelDataType> componentType =
+ getComponentTypes(
+ callBinding.getTypeFactory(), argTypes);
+ // check key type & value type
+ if (null == componentType.left || null == componentType.right) {
+ if (throwOnFailure) {
+ throw
callBinding.newValidationError(RESOURCE.needSameTypeParameter());
+ }
+ return false;
+ }
+ return true;
+ }
+
+ /**
+ * Extract the key type and value type of arg types.
+ */
+ private static Pair<@Nullable RelDataType, @Nullable RelDataType>
getComponentTypes(
+ RelDataTypeFactory typeFactory,
+ List<RelDataType> argTypes) {
+ return Pair.of(
+ typeFactory.leastRestrictive(Util.quotientList(argTypes, 2, 0)),
Review Comment:
Where are these constants coming from and what do they signify?
##########
site/_docs/reference.md:
##########
@@ -2769,6 +2769,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() | Returns a empty map
Review Comment:
nit: an
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
