Skip to content

[CALCITE-7615] MAP_CONCAT does not accept NULL as an argument #5042

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xuzifu666 wants to merge 1 commit into
base: main
Choose a base branch
from
+58 −27
Open

[CALCITE-7615] MAP_CONCAT does not accept NULL as an argument #5042

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/calcite/runtime/CalciteResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -741,7 +741,7 @@ ExInst<CalciteException> illegalArgumentForTableFunctionCall(String a0,
@BaseMessage("Map requires an even number of arguments")
ExInst<SqlValidatorException> mapRequiresEvenArgCount();

@BaseMessage("Function ''{0}'' should all be of type map, but it is ''{1}''")
@BaseMessage("Arguments of function ''{0}'' should all be of type MAP, but ''{1}'' was found")
ExInst<SqlValidatorException> typesShouldAllBeMap(String funcName, String type);

@BaseMessage("Incompatible types")
Expand Down
50 changes: 37 additions & 13 deletions core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1763,23 +1763,47 @@ private static RelDataType deriveTypeArraysZip(SqlOperatorBinding opBinding) {
OperandTypes.ARRAY.or(OperandTypes.ARRAY_BOOLEAN_LITERAL));

private static RelDataType deriveTypeMapConcat(SqlOperatorBinding opBinding) {
final RelDataTypeFactory typeFactory = opBinding.getTypeFactory();
if (opBinding.getOperandCount() == 0) {
final RelDataTypeFactory typeFactory = opBinding.getTypeFactory();
final RelDataType type = typeFactory.createSqlType(SqlTypeName.VARCHAR);
requireNonNull(type, "type");
final RelDataType type = typeFactory.createSqlType(SqlTypeName.ANY);
return SqlTypeUtil.createMapType(typeFactory, type, type, true);
} else {
final List<RelDataType> operandTypes = opBinding.collectOperandTypes();
for (RelDataType operandType : operandTypes) {
if (!SqlTypeUtil.isMap(operandType)) {
throw opBinding.newError(
RESOURCE.typesShouldAllBeMap(
opBinding.getOperator().getName(),
operandType.getFullTypeString()));
}
}
final List<RelDataType> operandTypes = opBinding.collectOperandTypes();
final List<RelDataType> mapTypes = new ArrayList<>();
boolean hasNull = false;
for (RelDataType operandType : operandTypes) {
if (operandType.getSqlTypeName() == SqlTypeName.NULL) {
hasNull = true;
} else if (SqlTypeUtil.isMap(operandType)) {
mapTypes.add(operandType);
} else {
throw opBinding.newError(
RESOURCE.typesShouldAllBeMap(
opBinding.getOperator().getName(),
operandType.getFullTypeString()));
}
}
if (mapTypes.isEmpty() || hasNull) {
// All arguments are NULL literals, or at least one null argument;
// the result is NULL.
return typeFactory.createSqlType(SqlTypeName.NULL);
}
// If there are MAP<ANY, ANY> placeholders (e.g. from map_concat() with no
// arguments) alongside more specific MAP types, ignore the placeholders and
// infer the type from the specific maps.
final List<RelDataType> concreteMapTypes = new ArrayList<>();
for (RelDataType mapType : mapTypes) {
final RelDataType keyType = requireNonNull(mapType.getKeyType());
final RelDataType valueType = requireNonNull(mapType.getValueType());
if (keyType.getSqlTypeName() == SqlTypeName.ANY
&& valueType.getSqlTypeName() == SqlTypeName.ANY) {
continue;
}
return requireNonNull(opBinding.getTypeFactory().leastRestrictive(operandTypes));
concreteMapTypes.add(mapType);
}
final List<RelDataType> typesToUse =
concreteMapTypes.isEmpty() ? mapTypes : concreteMapTypes;
return requireNonNull(typeFactory.leastRestrictive(typesToUse));
}

/** The "MAP_CONCAT(map [, map]*)" function. */
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ DuplicateNameInColumnList=Duplicate name ''{0}'' in column list
RequireAtLeastOneArg=Require at least 1 argument
MapRequiresTwoOrMoreArgs=Map requires at least 2 arguments
MapRequiresEvenArgCount=Map requires an even number of arguments
TypesShouldAllBeMap=Function ''{0}'' should all be of type map, but it is ''{1}''
TypesShouldAllBeMap=Arguments of function ''{0}'' should all be of type MAP, but ''{1}'' was found
IncompatibleTypes=Incompatible types
ColumnCountMismatch=Number of columns must match number of query columns
DuplicateColumnAndNoColumnList=Column has duplicate column name ''{0}'' and no column list specified
Expand Down
31 changes: 19 additions & 12 deletions testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9230,7 +9230,9 @@ void checkArrayReverseFunc(SqlOperatorFixture f0, SqlFunction function,
"(INTEGER NOT NULL, INTEGER) MAP NOT NULL");
// test zero arg, but it should return empty map.
f.checkScalar("map_concat()", "{}",
"(VARCHAR NOT NULL, VARCHAR NOT NULL) MAP");
"(ANY NOT NULL, ANY NOT NULL) MAP");
f.checkScalar("map_concat(map_concat(), map[1, 2])", "{1=2}",
"(INTEGER NOT NULL, INTEGER NOT NULL) MAP NOT NULL");

// after calcite supports cast(null as map<string, int>), it should add these tests.
if (TODO) {
Expand All @@ -9244,16 +9246,18 @@ void checkArrayReverseFunc(SqlOperatorFixture f0, SqlFunction function,

// test only has one operand, but it is not map type.
f.checkFails("^map_concat(1)^",
"Function 'MAP_CONCAT' should all be of type map, but it is 'INTEGER NOT NULL'", false);
f.checkFails("^map_concat(null)^",
"Function 'MAP_CONCAT' should all be of type map, but it is 'NULL'", false);
"Arguments of function 'MAP_CONCAT' should all be of type MAP, "
+ "but 'INTEGER NOT NULL' was found", false);
// test operand is the NULL literal.
f.checkNull("map_concat(null)");
// test operands in same type family, but it is not map type.
f.checkFails("^map_concat(array[1], array[1])^",
"Function 'MAP_CONCAT' should all be of type map, "
+ "but it is 'INTEGER NOT NULL ARRAY NOT NULL'", false);
f.checkFails("^map_concat(map['foo', 1], null)^",
"Function 'MAP_CONCAT' should all be of type map, "
+ "but it is 'NULL'", false);
"Arguments of function 'MAP_CONCAT' should all be of type MAP, "
+ "but 'INTEGER NOT NULL ARRAY NOT NULL' was found", false);
// test map operand with NULL literal.
f.checkNull("map_concat(map['foo', 1], null)");
f.checkType("map_concat(map['foo', 1], null)", "NULL");
f.checkNull("map_concat(null, map['foo', 1])");
// test operands not in same type family.
f.checkFails("^map_concat(map[1, null], array[1])^",
"Parameters must be of the same type", false);
Expand All @@ -9272,6 +9276,9 @@ void checkArrayReverseFunc(SqlOperatorFixture f0, SqlFunction function,
"(INTEGER NOT NULL, INTEGER) MAP NOT NULL");
f1.checkScalar("map_concat(map('foo', 1), map())", "{foo=1}",
"(CHAR(3) NOT NULL, INTEGER NOT NULL) MAP NOT NULL");
// test zero arg, but it should return empty map.
f1.checkScalar("map_concat()", "{}",
"(ANY NOT NULL, ANY NOT NULL) MAP");

// test operand is null map
f1.checkNull("map_concat(map('foo', 1), cast(null as map<varchar, int>))");
Expand All @@ -9281,9 +9288,9 @@ void checkArrayReverseFunc(SqlOperatorFixture f0, SqlFunction function,
f1.checkType("map_concat(cast(null as map<varchar, int>), map['foo', 1])",
"(VARCHAR NOT NULL, INTEGER) MAP");

f1.checkFails("^map_concat(map('foo', 1), null)^",
"Function 'MAP_CONCAT' should all be of type map, "
+ "but it is 'NULL'", false);
f1.checkNull("map_concat(map('foo', 1), null)");

@mihaibudiu mihaibudiu Jun 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to add a test with no arguments, if that's really supported?

@xuzifu666 xuzifu666 Jun 24, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I had added a test with no arguments, according to Spark behaivor this should return Map<StringType, StringType>: https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala#L713

@transient override lazy val dataType: MapType = {
    if (children.isEmpty) {
      MapType(StringType, StringType)   // default
    } else {
      super.dataType.asInstanceOf[MapType]
    }
}

@mihaibudiu mihaibudiu Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be documented. Frankly, it looks wrong to me, for empty array the type is inferred to be ANY ARRAY I think

@xuzifu666 xuzifu666 Jun 24, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, this is based on Spark, and I also find it odd. I changed the type here to ANY because I think it's more appropriate that way.

@mihaibudiu mihaibudiu Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a test like MAP_CONCAT(MAP_CONCAT(), MAP[1, 2]). I think this should work, and the result should be MAP<INT, INT>.

@xuzifu666 xuzifu666 Jun 24, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, had added a test and result is as expected.

f1.checkType("map_concat(map('foo', 1), null)", "NULL");
f1.checkNull("map_concat(null, map('foo', 1))");
// test operands not in same type family.
f1.checkFails("^map_concat(map(1, null), array[1])^",
"Parameters must be of the same type", false);
Expand Down
Loading