mihaibudiu commented on code in PR #3847:
URL: https://github.com/apache/calcite/pull/3847#discussion_r1672687245
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -8182,6 +8182,19 @@ void checkRegexpExtract(SqlOperatorFixture f0,
FunctionAlias functionAlias) {
"CHAR(3) NOT NULL ARRAY NOT NULL");
f1.checkScalar("map_keys(map('foo', 1, null, 2))", "[foo, null]",
"CHAR(3) ARRAY NOT NULL");
+
+ f1.checkScalar("map_keys(map('foo', 1, null, 2))", "[foo, null]",
Review Comment:
I am assuming these have been checked against spark.
Perhaps you can add a comment, also listing the spark version - Spark has
been known to change function behaviors in the past.
##########
core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java:
##########
@@ -1491,6 +1496,43 @@ private static class PeriodOperandTypeChecker
}
}
+ /**
+ * Parameter type-checking strategy where types must be Map
+ * and adjust their types to suit the Map function's constructor.
+ */
+ private static class MapSparkFunctionOperandTypeChecker extends
SameOperandTypeChecker {
+ MapSparkFunctionOperandTypeChecker() {
+ super(1);
+ }
+
+ @Override public boolean checkOperandTypes(
+ SqlCallBinding callBinding,
+ boolean throwOnFailure) {
+
+ final SqlNode op0 = callBinding.operand(0);
+ if (!OperandTypes.MAP.checkSingleOperandType(
+ callBinding,
+ op0,
+ 0,
+ throwOnFailure)) {
+ return false;
+ }
+
+ final RelDataType mapKeyType =
+ getKeyTypeOrThrow(SqlTypeUtil.deriveType(callBinding, op0));
+ final RelDataType mapValueType =
+ getValueTypeOrThrow(SqlTypeUtil.deriveType(callBinding, op0));
+
+ // If the key and value types are not unknown,
+ // adjust their types to suit the Map function's constructor.
+ if (mapKeyType.getSqlTypeName() != SqlTypeName.UNKNOWN
Review Comment:
I think we have discussed this in the past: I don't think that the type
checker is supposed to modify the node that is being checked.
There are type checking strategies that will backtrack (e.g.,
SqlOperandTypeChecker.or), and they do not expect the checked data structure to
change.
I think this pattern is very dangerous, and can lead to hard to diagnose
bugs.
On the other hand, I don't know how this should be done properly. The map
function does need to introduce type casts. But maybe the typechecking stage is
not the right stage.
##########
core/src/main/java/org/apache/calcite/sql/type/NonNullableAccessors.java:
##########
@@ -58,4 +58,10 @@ public static RelDataType getKeyTypeOrThrow(RelDataType
type) {
return requireNonNull(type.getKeyType(),
() -> "keyType is null for " + type);
}
+
+ @API(since = "1.37", status = API.Status.EXPERIMENTAL)
Review Comment:
Is this annotation necessary?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
