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

[CALCITE-6584] Validate prefixed column identifiers in SET clause of UPDATE statement #3972

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nielspardon
Copy link
Contributor

@nielspardon nielspardon commented Sep 19, 2024

This PR adds validation of prefixed column identifiers in SET clause of UPDATE statement (and in the UPDATE statement within a MERGE statement)

  • This is a follow-up PR to [CALCITE-6576] SqlParser: allow using table alias with column identifiers in SET section of UPDATE statement #3959
  • I first had to change SqlValidatorUtil.addFields() to use the Util.last() method for getting the last element of a compound identifier instead of relying on simple identifiers
  • I then implemented a validation logic in SqlValidatorImpl.createTargetRowType() which is being used for validating INSERT, UPDATE and MERGE
  • in presence of the new targetTableAlias parameter the validation logic checks the prefix of any non-simple identifiers or in other words it checks that any prefixed column identifiers are prefixed with the target table alias
  • I tried to check for any existing validation logic that I could reuse and it's possible I might have missed a utility method that I could have used.
  • The validation logic supports a mix of prefixed and unprefixed columns in the SET clause. Not sure if I should only allow prefixed columns here.

Let me know your thoughts.

Copy link

sonarcloud bot commented Sep 19, 2024

Copy link
Member

@caicancai caicancai left a comment

Choose a reason for hiding this comment

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

LGTM

@Test void testAliasInSetClauseOfUpdate() {
// good examples
sql("UPDATE sales.emp AS e SET e.deptno = 10").ok();
sql("UPDATE emp AS e SET e.deptno = 10").ok();
Copy link
Contributor

Choose a reason for hiding this comment

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

postgres actually rejects this: Query Error: error: column "e" of relation "emp" does not exist

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.

3 participants