[FLINK-40338][table-runtime] ELT() throws ClassCastException when index is not INT by hulincup · Pull Request #28931 · apache/flink

hulincup · 2026-08-06T01:45:32Z

Problem fixed & how

ELT(index, expr, exprs...) accepts any INTEGER_NUMERIC index (TINYINT/SMALLINT/INT/BIGINT). However, EltFunction#eval declares index as java.lang.Number and indexes the varargs array with exprs[(int) index - 1]. Per JLS 5.5, casting a Number reference to int compiles to a checkcast to Integer followed by unboxing, so a Byte, Short, or Long value throws ClassCastException on the success path (1 <= index <= exprs.length).

SELECT ELT(CAST(2 AS BIGINT), 'scala', 'java');
-- java.lang.ClassCastException: class java.lang.Long cannot be cast to class java.lang.Integer

Same for CAST(2 AS TINYINT) and CAST(2 AS SMALLINT).

The out-of-range guard above the cast uses index.longValue(), so out-of-range indices of any type still return NULL correctly; the exception only fires in the valid range. That is why the existing test ELT(9223372036854775807, 'ab', 'b') passes (returns NULL before reaching the cast) and every other existing test uses an INT literal.

Present since FLINK-35987 introduced ELT; confirmed absent from release-1.20 and present from release-2.0.

Fix: narrow the already-unboxed long idx (computed above for the range check) via primitive narrowing (int) idx (JLS 5.1.3, no checkcast) instead of casting the Number reference.

Behavior modified

previous: ELT with a non-INT INTEGER_NUMERIC index (TINYINT/SMALLINT/BIGINT) in the valid range 1 <= index <= exprs.length threw ClassCastException.
now: non-INT integer indices correctly return the corresponding expression.
impact: only the success path for non-INT integer indices; INT indices, NULL, and out-of-range behavior are unchanged.

Code refactored

EltFunction.eval: exprs[(int) index - 1] → exprs[(int) idx - 1], with a 3-line comment explaining the JLS rationale.

Features added

N/A

Functions optimized

N/A

Test plan

Added 3 regression cases to StringFunctionsITCase.eltTestCases() covering TINYINT, SMALLINT, and BIGINT indices (the three types that previously threw ClassCastException). Each expects the correct expression ("java" for index 2).
The local environment runs Java 8 and cannot build Flink master (requires Java 11+), so verification relies on CI:
```
mvn -pl flink-table/flink-table-planner -am test -Dtest=StringFunctionsITCase
```

…ex is not INT EltFunction.eval declares index as java.lang.Number to support TINYINT/SMALLINT/INT/BIGINT, but indexes the varargs array with exprs[(int) index - 1]. Per JLS 5.5, casting a Number reference to int compiles to a checkcast to Integer followed by unboxing, so a Byte, Short, or Long value throws ClassCastException on the success path (1 <= index <= exprs.length). The out-of-range guard above uses index.longValue(), so out-of-range indices of any type still return NULL correctly; the exception only fires in the valid range. Narrow the already-unboxed long idx (computed above for the range check) via primitive narrowing (JLS 5.1.3) instead, which emits no checkcast and works for every INTEGER_NUMERIC type. Adds TINYINT/SMALLINT/BIGINT index regression cases to StringFunctionsITCase.

flinkbot · 2026-08-06T01:50:00Z

CI report:

f5b2495 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

SEPURI-SAI-KRISHNA · 2026-08-06T03:42:23Z

Hi @hulincup — I'm the reporter of FLINK-40338 and had asked to be assigned before this PR was opened, with a patch and regression tests ready. Per the contribute-code guide, PRs on unassigned tickets aren't reviewed or merged, so could we wait for a committer to decide who takes it?

raminqaf

Changes look good. Left a nit

raminqaf · 2026-08-06T12:01:25Z

                                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


I would remove this

Suggested change

// FLINK-40338: non-INT INTEGER_NUMERIC index must not throw ClassCastException

Done — removed in 778b833f.

SEPURI-SAI-KRISHNA · 2026-08-06T13:35:30Z

+                        .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))


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:

.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.

raminqaf · 2026-08-06T13:58:56Z

+                        .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))


Just in case write a test with non-literals too please

@raminqaf agreed. Here's the patch I have locally, already verified, it fails with ClassCastException on java.lang.Byte, Short and Long before the fix and passes after:

.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())

.testResult( $("f4").elt("scala", "java"), "ELT(f4, 'scala', 'java')", "java", DataTypes.VARCHAR(5)) .testResult( $("f5").elt("scala", "java"), "ELT(f5, 'scala', 'java')", "java", DataTypes.VARCHAR(5)) .testResult( $("f6").elt("scala", "java"), "ELT(f6, 'scala', 'java')", "java", DataTypes.VARCHAR(5))

@hulincup feel free to take these directly.

Done — removed the redundant constant cases and added field-reference tests (f4/f5/f6) per your suggestion, so the codegen'd operator path is now covered alongside the reducer path. Thanks @SEPURI-SAI-KRISHNA for the verified patch and @raminqaf for catching the gap.

@SEPURI-SAI-KRISHNA

…ELT tests Constant test cases are folded by ExpressionReducer during optimization, so they only cover the constant-folding path, not the codegen'd operator path that a real job hits. Extend onFieldsWithData with TINYINT/SMALLINT/ BIGINT fields and add field-reference test cases (f4/f5/f6) to cover the runtime path; keep one constant case so the reducer path stays covered. Test plan suggested by @SEPURI-SAI-KRISHNA and @raminqaf during review. Co-authored-by: SEPURI-SAI-KRISHNA <saik20533@gmail.com>

hulincup · 2026-08-07T00:54:34Z

Hi @SEPURI-SAI-KRISHNA, you're right, and I apologize for missing your claim on the JIRA ticket before opening this PR. That was my mistake — I should have scanned the comments for assignment signals first.

Since @raminqaf has started reviewing here and you've generously offered to hand over the test cases, I've incorporated your field-reference tests directly (credited in the new commit). The codegen path is now covered via f4/f5/f6 (TINYINT/SMALLINT/BIGINT), with one constant case kept for the reducer path. Thanks for the verified patch and the detailed review.

Address raminqaf review: drop the inline comment block; the JIRA context is already carried by the PR title and commit messages.

SEPURI-SAI-KRISHNA · 2026-08-07T01:56:35Z

Thanks @hulincup, appreciated — and no hard feelings, it's an easy thing to miss.

One build heads-up: the new onFieldsWithData line is 106 chars, over the 100-column AOSP limit that spotless enforces (google-java-format 1.24.0, pom.xml). mvn spotless:apply will wrap it to:

.onFieldsWithData(
null, null, null, new byte[] {1, 2, 3}, (byte) 2, (short) 2, 2L)

Worth running before the next push — CI is red on the earlier commit.

Minor: since the test code was taken as-is, a Co-authored-by: trailer on 8ee7dd5 would be the conventional attribution. Entirely up to you — the mention in the message is already generous.

@SEPURI-SAI-KRISHNA

Spotless (google-java-format 1.24.0) enforces a 100-column AOSP limit; the onFieldsWithData line added in 8ee7dd5 was 106 chars and failed the spotless-check. Wrap as mvn spotless:apply would. Thanks to @SEPURI-SAI-KRISHNA for the heads-up.

hulincup · 2026-08-07T02:22:50Z

Thanks @SEPURI-SAI-KRISHNA — wrapped it in eea2481, exactly as mvn spotless:apply would. Good catch on the 100-col limit; should go green on the next run.

Re: attribution — the test-plan credit already lives in the 8ee7dd53 message ("suggested by @SEPURI-SAI-KRISHNA and @raminqaf during review"), which feels right to me, but happy to add the Co-authored-by trailer too if you'd prefer. Either way, appreciate the heads-up.

SEPURI-SAI-KRISHNA · 2026-08-07T02:36:17Z

Yes please, if it's not too much trouble — the tests went in as-is, so the trailer is the accurate form. Exact line:

Co-authored-by: SEPURI-SAI-KRISHNA saik20533@gmail.com

That address is linked to my GitHub account, so it'll attribute properly. 8ee7dd5 is the natural place, or whichever commit ends up carrying the tests if this gets squashed at
merge — whatever's least disruptive.

Thanks for wrapping the line, and for how you've handled all of this.

raminqaf

LGTM!

hulincup · 2026-08-07T06:00:14Z

Done — added the trailer to the test commit in 8ee7dd53 (rewritten; new SHA after force-push). Tests are now attributed via Co-authored-by: SEPURI-SAI-KRISHNA <saik20533@gmail.com>. Thanks for the thorough review and for flagging both the spotless line and the attribution.

snuyanzin · 2026-08-07T07:08:59Z

+        // Narrow the already-unboxed long instead of casting the Number reference.
+        // Casting `index` (java.lang.Number) to int compiles to a checkcast to Integer
+        // followed by unboxing, which throws ClassCastException for Byte/Short/Long.


why do we need it if we have tests?

other wise with such approach we should comment on every line the same which I don't think is the right way

hulincup · 2026-08-07T07:14:18Z

@flinkbot run azure

raminqaf reviewed Aug 6, 2026

View reviewed changes

SEPURI-SAI-KRISHNA reviewed Aug 6, 2026

View reviewed changes

raminqaf reviewed Aug 6, 2026

View reviewed changes

[FLINK-40338][table-planner] Remove redundant comment per review nit

6991d06

Address raminqaf review: drop the inline comment block; the JIRA context is already carried by the PR title and commit messages.

github-actions Bot added the community-reviewed PR has been reviewed by the community. label Aug 7, 2026

raminqaf approved these changes Aug 7, 2026

View reviewed changes

hulincup force-pushed the fix/elt-non-int-index-classcast branch from eea2481 to f5b2495 Compare August 7, 2026 05:59

snuyanzin reviewed Aug 7, 2026

View reviewed changes

Uh oh!

Conversation

hulincup commented Aug 6, 2026

Problem fixed & how

Behavior modified

Code refactored

Features added

Functions optimized

Test plan

Uh oh!

flinkbot commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

SEPURI-SAI-KRISHNA commented Aug 6, 2026

Uh oh!

raminqaf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hulincup commented Aug 7, 2026

Uh oh!

SEPURI-SAI-KRISHNA commented Aug 7, 2026

Uh oh!

hulincup commented Aug 7, 2026

Uh oh!

SEPURI-SAI-KRISHNA commented Aug 7, 2026

Uh oh!

raminqaf left a comment

Choose a reason for hiding this comment

Uh oh!

hulincup commented Aug 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hulincup commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flinkbot commented Aug 6, 2026 •

edited

Loading