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

Add validation to Insert statement parsing to match the column names with supplied values #3070

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

g31pranjal
Copy link
Member

@g31pranjal g31pranjal commented Jan 28, 2025

fixes #3069. This PR also provide a short-term solution to #3068

In essence, it does the following things:

  1. Throws an exception, if a column is declared in the INSERT statement but its value is not provided.
  2. Throws an exception, if the number of columns declared in the INSERT statement is not equal to the number of values provided for a row
  3. Throws an exception, if a not-null column is not declared in the INSERT statement or its value is not provided
  4. Restricts declaring types (except ARRAY) as NOT NULL in the DDL since this feature is not fully supported throughout.
  5. Removes providing system default value in any scenario. {Since we do not formally support DEFAULT}

@foundationdb-ci
Copy link
Contributor

Result of fdb-record-layer-pr on Linux CentOS 7

  • Commit ID: 596776c
  • Duration 0:55:36
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

Copy link
Collaborator

@arnaud-lacurie arnaud-lacurie left a comment

Choose a reason for hiding this comment

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

Can you also fix up the title of the PR and add a more comprehensive description?

// This phenomenon is also true with basic types, i.e., non-nullability is not enforced and is tracked by
// TODO (Add support + tests for column nullable/not null)
insertQuery("INSERT INTO T (pk) VALUES (4)");
Assert.assertThrows(SQLException.class, () -> insertQuery("INSERT INTO T (pk) VALUES (4)"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use RelationalAssertions.assertThrowsSqlException that allows you to check for the error message and SQLState?

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you check the SQLState as well?
RelationalAssertions.assertThrowsSqlException has a method withErrorCode or something like this that you can use.

@g31pranjal g31pranjal changed the title fix INSERT statement cases Add validation to Insert statement parsing to match the column names with supplied values Jan 30, 2025
@foundationdb-ci
Copy link
Contributor

Result of fdb-record-layer-pr on Linux CentOS 7

  • Commit ID: cddee53
  • Duration 0:35:55
  • Result: ❌ FAILED
  • Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of fdb-record-layer-pr on Linux CentOS 7

  • Commit ID: e5cd8c2
  • Duration 0:56:07
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

Copy link
Collaborator

@arnaud-lacurie arnaud-lacurie left a comment

Choose a reason for hiding this comment

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

Two small comments, then I think this one can come in.
Good work!

-
# Case when the number of values is more than the number of columns specified.
- query: insert into A(A1, A2, A3) values (5, 6, 7, 8, 9);
- supported_version: !current_version
- error: "42601"
-
# Case when a nullable column's value is not provided
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to cover that, could you add a test where the not provided column is the first one and another case where it's the last one?

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Insert statement does not fully validate the column names with supplied values
3 participants