Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-34902][table] Fix IndexOutOfBoundsException for VALUES #24724

Merged
merged 2 commits into from Apr 30, 2024

Conversation

jeyhunkarimov
Copy link
Contributor

What is the purpose of the change

The purpose of this PR is to avoid wrong IndexOutOfBoundsException with Values.

For example, a query INSERT INTO t_3_columns (id, name, num) VALUES ('id_0', 'name_0', 5); should not throw an exception.

Brief change log

  • Fix the exception catching conditions with VALUES expression
  • Add tests

Verifying this change

org.apache.flink.table.planner.calcite. FlinkCalciteSqlValidatorTest

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)

@flinkbot
Copy link
Collaborator

flinkbot commented Apr 25, 2024

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@reswqa
Copy link
Member

reswqa commented Apr 26, 2024

@twalthr, would you mind taking a look at this? thanks!

Copy link
Contributor

@twalthr twalthr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this so quickly.

@@ -63,6 +64,55 @@ void testInsertInto2() {
.hasMessageContaining(" Number of columns must match number of query columns");
}

@Test
void testInsertInto3() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests should have a descriptive name. E.g. testInsertIntoValuesColumnsMismatch

void testInsertInto7() {
assertDoesNotThrow(
() -> {
plannerMocks.getParser().parse("INSERT INTO t2 (a,b) Select 1, 2");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a test for INSERT INTO t2 (a) VALUES (1), (3)?

@jeyhunkarimov
Copy link
Contributor Author

Hi @twalthr thanks a lot for your review. Could you please recheck in your available time? Thanks

Copy link
Contributor

@twalthr twalthr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jeyhunkarimov. LGTM

@twalthr twalthr merged commit 8a75f8f into apache:master Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants