luoyuxia commented on code in PR #21927:
URL: https://github.com/apache/flink/pull/21927#discussion_r1213012657
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/type/LeastRestrictiveOrDefaultReturnTypeInference.java:
##########
@@ -56,7 +57,7 @@ public RelDataType inferReturnType(SqlOperatorBinding
opBinding) {
List<RelDataType> types = new ArrayList<>();
for (int i = startTypeIdx; i < nOperands; i++) {
RelDataType type = opBinding.getOperandType(i);
- if (SqlTypeUtil.isNumeric(type)) {
+ if (SqlTypeUtil.isNumeric(type) || SqlTypeUtil.isCharacter(type)) {
Review Comment:
My gut feeling is if the type is `BINARY`, the cast will also fail, right?
For example, some thing like IF(false, binary(10), binary(20)).
Seem then it will also cast to binary(10) which cause precision loss.
If that's the case, we'll need to enumerate all the types that will be
affected in here.
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/stream/sql/CalcITCase.scala:
##########
@@ -366,6 +366,50 @@ class CalcITCase extends StreamingTestBase {
sink.getAppendResults.foreach(result => assertEquals(expected, result))
}
+ @Test
+ def testIfFunctionWithNestedInput(): Unit = {
+ implicit val typeInfo: TypeInformation[Row] =
+ Types.ROW(
+ Array("words", "c"),
+ Array(
+ Types.ROW(
+ Array("a", "b"),
+ Array(Types.STRING,
Types.STRING).asInstanceOf[Array[TypeInformation[_]]]),
+ Types.INT).asInstanceOf[Array[TypeInformation[_]]]
+ )
+ val t = env.fromCollection(TestData.nullableNestedRow).toTable(tEnv)
+ tEnv.createTemporaryView("t", t)
+ val cmp = (l: Row, r: Row) => l.getField(1).asInstanceOf[Int] >
r.getField(1).asInstanceOf[Int]
+ var result = tEnv
+ .executeSql("SELECT IF(c > 3, words.a, words.b), c, words.a, words.b
from t")
+ .collect()
+ .toList
+ .sortWith(cmp)
+ var expected = List(
+ Row.of("Worlds", Int.box(1), "Hello", "Worlds"),
+ Row.of("Hello", Int.box(5), "Hello", "Hidden"),
+ Row.of(null, Int.box(2), "Hello again", null),
+ Row.of("World", Int.box(0), null, "World"),
+ Row.of("Hello again", Int.box(6), "Hello again", "Hide")
+ ).sortWith(cmp)
+ assertEquals(expected, result)
+
+ // IF with constant values
+ result = tEnv
Review Comment:
How about simplying the test with the following code?
```
val expected = List("true, 6", "xxx")
val actual = CollectionUtil.iteratorToList(tEnv
.executeSql("SELECT IF(c > 3, 'true', 'false'), c from
t").collect()).map(r => r.toString)
assertEquals(expected.sorted, actual.sorted)
```
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/stream/sql/CalcITCase.scala:
##########
@@ -366,6 +366,50 @@ class CalcITCase extends StreamingTestBase {
sink.getAppendResults.foreach(result => assertEquals(expected, result))
}
+ @Test
+ def testIfFunctionWithNestedInput(): Unit = {
Review Comment:
Also, is it really related to`nested input`?
Can we just simply the test like
[here](https://github.com/apache/flink/pull/21657/files#diff-117387d9452230d1d66c1899d51ec1692abd437b03b4359d4794768121504fcdR1843)
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/stream/sql/CalcITCase.scala:
##########
@@ -366,6 +366,50 @@ class CalcITCase extends StreamingTestBase {
sink.getAppendResults.foreach(result => assertEquals(expected, result))
}
+ @Test
+ def testIfFunctionWithNestedInput(): Unit = {
Review Comment:
How about renaming to `testIfFunction`?
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/type/LeastRestrictiveOrDefaultReturnTypeInference.java:
##########
@@ -27,25 +27,26 @@
import java.util.List;
/**
- * Determine the return type of functions with numeric arguments. The return
type is the type of the
- * argument with the largest range. We start to consider the arguments from
the `startTypeIdx`-th
- * one. If one of the arguments is not of numeric type, we return the type of
the
+ * Determine the return type of functions with arguments that has the least
restrictive (eg:
+ * numeric, character). The return type is the type of the argument with the
largest range. We start
+ * to consider the arguments from the `startTypeIdx`-th one. If one of the
arguments is not of the
+ * type that has the least restrictive (eg: numeric, character), we return the
type of the
* `defaultTypeIdx`-th argument instead.
*/
-public class NumericOrDefaultReturnTypeInference implements
SqlReturnTypeInference {
+public class LeastRestrictiveOrDefaultReturnTypeInference implements
SqlReturnTypeInference {
Review Comment:
I found the name if hard to understand to me. Since we only use it in `IF`
sql function, how about rename it to `IF_NULLABLE` like `STR_MAP_NULLABLE`?
--
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]
