SEPURI-SAI-KRISHNA commented on code in PR #28931:
URL: https://github.com/apache/flink/pull/28931#discussion_r3729216289
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/StringFunctionsITCase.java:
##########
@@ -182,6 +182,22 @@ private Stream<TestSetSpec> eltTestCases() {
DataTypes.VARCHAR(5))
.testResult(
lit(2).elt("a", "b"), "ELT(2, 'a', 'b')", "b",
DataTypes.CHAR(1))
+ // FLINK-40338: non-INT INTEGER_NUMERIC index must not
throw ClassCastException
+ .testResult(
+ lit(2).cast(DataTypes.TINYINT()).elt("scala",
"java"),
+ "ELT(CAST(2 AS TINYINT), 'scala', 'java')",
+ "java",
+ DataTypes.VARCHAR(5))
+ .testResult(
+ lit(2).cast(DataTypes.SMALLINT()).elt("scala",
"java"),
+ "ELT(CAST(2 AS SMALLINT), 'scala', 'java')",
+ "java",
+ DataTypes.VARCHAR(5))
+ .testResult(
+ lit(2).cast(DataTypes.BIGINT()).elt("scala",
"java"),
+ "ELT(CAST(2 AS BIGINT), 'scala', 'java')",
+ "java",
+ DataTypes.VARCHAR(5))
Review Comment:
Thanks for picking this up, the fix itself looks right. One coverage gap
worth closing before merge.
All three new cases pass constant arguments, so `ExpressionReducer` folds
the entire call during optimization and the generated runtime code is never
reached. The plans:
```
-- ELT(CAST(2 AS TINYINT), 'scala', 'java')
== Optimized Execution Plan ==
Calc(select=[CAST('java' AS VARCHAR(5)) AS EXPR$0]) <- ELT folded away
-- ELT(b, 'scala', 'java') where b TINYINT
== Optimized Execution Plan ==
Calc(select=[ELT(b, 'scala', 'java') AS EXPR$0]) <- ELT reaches the
operator
```
To be clear, these cases *do* fail without the fix, because the reducer
executes the function at plan time. But they only cover the constant-folding
path, not the codegen'd operator path that a real job hits, so a future
regression in the runtime path wouldn't be caught here.
The case just below at line 201 already uses the field-reference pattern, so
extending the existing fields covers it:
```java
.onFieldsWithData(null, null, null, new byte[] {1, 2, 3}, (byte) 2, (short)
2, 2L)
.andDataTypes(
DataTypes.INT(), DataTypes.STRING(), DataTypes.BYTES(),
DataTypes.BYTES(),
DataTypes.TINYINT(), DataTypes.SMALLINT(), DataTypes.BIGINT())
```
and then `$("f4").elt("scala", "java")`, `$("f5")`, `$("f6")`, keeping one
of the constant cases so the reducer path stays covered too.
I'm the reporter of FLINK-40338 and already have these written and verified
locally (they fail with `ClassCastException` on `java.lang.Byte`/`Short`/`Long`
without the fix). Happy to hand them over for you to include here, or to open
them as a follow-up if you'd rather keep this PR as is, whichever you prefer.
--
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]
