Aaaaaaron commented on a change in pull request #2056:
URL: https://github.com/apache/calcite/pull/2056#discussion_r451378059
##########
File path:
core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
##########
@@ -53,6 +67,26 @@
}
/** Test cases for {@link TypeCoercion#inOperationCoercion}. */
+ @Test void testInOperationCoercion() throws SqlParseException {
+ assertFalse(isInCoercion("'1' in ('1', '2', '3')"));
+ assertTrue(isInCoercion("1 in ('1', '2', '3')"));
+ assertTrue(isInCoercion("1 not in ('1', '2', '3')"));
+ }
+
+ private boolean isInCoercion(String expr) throws SqlParseException {
+ final SqlCall inExpr = (SqlCall) SqlParser.create(expr).parseExpression();
+ final RelDataTypeFactory typeFactory =
+ new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+ final RelDataType charType = typeFactory.createSqlType(SqlTypeName.CHAR);
Review comment:
> After some dig into the code, i found that if the RHS is a node list,
the common type is pre-computed for SQL in operator, and the nodes type was
actually changed during the implicit type coercion, so the final validation
passes and are still valid.
>
> So it is hard to write a test, i would be ok if this patch does not have a
test, the test is complex and it is hard to understand what it is testing.
Yes, the fix only affects the return of "inOperationCoercion", the node type
was updated during "inOperationCoercion", it is a side effect of this function.
IMO, having the test is better than no ut. On the one hand, ut puts
constraints on the method, on the other hand, it is easy for debugging the
method "inOperationCoercion".
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
