hulincup commented on code in PR #28931:
URL: https://github.com/apache/flink/pull/28931#discussion_r3840151232
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/StringFunctionsITCase.java:
##########
@@ -182,6 +186,26 @@ private Stream<TestSetSpec> eltTestCases() {
DataTypes.VARCHAR(5))
.testResult(
lit(2).elt("a", "b"), "ELT(2, 'a', 'b')", "b",
DataTypes.CHAR(1))
+ .testResult(
+ lit(2).cast(DataTypes.TINYINT()).elt("scala",
"java"),
+ "ELT(CAST(2 AS TINYINT), 'scala', 'java')",
+ "java",
+ DataTypes.VARCHAR(5))
+ .testResult(
+ $("f4").elt("scala", "java"),
+ "ELT(f4, 'scala', 'java')",
+ "java",
+ DataTypes.VARCHAR(5))
+ .testResult(
Review Comment:
@snuyanzin Good question, thanks for raising it.
I did check `SELECT ELT(1, 2, 3)` — it fails at planning time, not runtime.
ELT's `inputTypeStrategy` (in `BuiltInFunctionDefinitions.java`) constrains the
`expr`/`exprs` arguments to `CHARACTER_STRING` or `BINARY_STRING` type families
only. Integer exprs are rejected during validation with "Invalid input
arguments. Expected signatures are: ELT(index <INTEGER_NUMERIC>, expr
<CHARACTER_STRING>, exprs <CHARACTER_STRING>...) / ELT(index <INTEGER_NUMERIC>,
expr <BINARY_STRING>, exprs <BINARY_STRING>...)", so they never reach
`EltFunction.eval` and can't trigger the ClassCastException.
This non-string-expr case is already covered in the same `eltTestCases()`
method — the existing "Validation Error" `TestSetSpec` exercises it (e.g. with
`f2 = DECIMAL(1,0)` via `testTableApiValidationError` /
`testSqlValidationError`).
The ClassCastException this PR fixes is specifically about the **index**
argument, not the exprs. The index is declared as `Number` (`INTEGER_NUMERIC`
family), and the bug was in `exprs[(int) index - 1]` — casting a `Number`
reference (`Byte`/`Short`/`Long`) to `int` triggers a `checkcast` to `Integer`.
The exprs are type-narrowed to string/binary before eval, so they're never
involved in the buggy cast. That's why all test exprs are strings — they have
to be, per the type strategy.
Regarding "strings only" — the `BINARY_STRING` path is actually exercised
too. The existing tests include cases like `lit(2).elt($("f2"), $("f3"),
$("f3"))` where `f2`/`f3` are `BYTES`, returning `byte[]`. So both
`CHARACTER_STRING` and `BINARY_STRING` families are covered.
The new test cases (f4/f5/f6 with `TINYINT`/`SMALLINT`/`BIGINT` indices)
target exactly the bug being fixed — a non-INT `INTEGER_NUMERIC` index reaching
`eval`.
--
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]
