[CALCITE-7615] MAP_CONCAT does not accept NULL as an argument#5042
[CALCITE-7615] MAP_CONCAT does not accept NULL as an argument#5042xuzifu666 wants to merge 5 commits into
Conversation
| } | ||
| return requireNonNull(opBinding.getTypeFactory().leastRestrictive(operandTypes)); | ||
| } | ||
| if (mapTypes.isEmpty()) { |
There was a problem hiding this comment.
Here I'm referencing Spark's behavior:
SELECT map_concat(map(1, 'a'), null);
-- result: NULL
SELECT map_concat(null, map(2, 'b'));
-- result: NULL
SELECT map_concat(map(1, 'a'), map(2, 'b'), null);
-- result: NULL
This should be reasonable according to Spark's standards.
| + "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)", |
There was a problem hiding this comment.
this type could actually be NULL too.
There was a problem hiding this comment.
Yes, the main concern here is that the constant map_concat(map['foo', 1], null) might be collapsed into a NULL literal. This would cause the static type to change from MAP to NULL, making checkType unstable or even failing. Therefore, I've removed this test case.
There was a problem hiding this comment.
Why would the type change if you infer it to be NULL?
There was a problem hiding this comment.
Upon closer examination, this should indeed be a NULL type; I have modified the test.
| 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)"); |
There was a problem hiding this comment.
Do you want to add a test with no arguments, if that's really supported?
There was a problem hiding this comment.
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]
}
}
There was a problem hiding this comment.
Maybe this should be documented. Frankly, it looks wrong to me, for empty array the type is inferred to be ANY ARRAY I think
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>.
There was a problem hiding this comment.
OK, had added a test and result is as expected.
| return requireNonNull(opBinding.getTypeFactory().leastRestrictive(operandTypes)); | ||
| } | ||
| if (mapTypes.isEmpty() || hasNull) { | ||
| // All arguments are NULL literals, or at least one argument is a NULL literal; |
There was a problem hiding this comment.
at least one null argument.
| 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)"); |
There was a problem hiding this comment.
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>.
|
|
If you are happy with current changes, I would squash the commits~ @mihaibudiu |
mihaibudiu
left a comment
mihaibudiu
left a comment
There was a problem hiding this comment.
I have already approved
2 participants
jira: https://issues.apache.org/jira/browse/CALCITE-7615