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

Fix(clickhouse): USING, [PRIMARY|FOREIGN] KEY allows unwrapped col list #1615

Merged
merged 2 commits into from
May 14, 2023

Conversation

pkit
Copy link
Contributor

@pkit pkit commented May 13, 2023

I.e. both USING a, b, c and USING (a, b, c) are allowed
Although the wrapped variant is a canonical one (see docs)
https://clickhouse.com/docs/en/sql-reference/statements/select/join#examples

Unwrapped columns and expression lists are allowed in other places too. Will update later.

I.e. both `USING a, b, c` and `USING (a, b, c)` are allowed
Although the wrapped variant is a canonical one (see docs)
@tobymao
Copy link
Owner

tobymao commented May 13, 2023

everywhere with wrapped id vars in clickhouse allows unwrapped? your change seems a bit dangerous

should you target it just for the using query?

@pkit
Copy link
Contributor Author

pkit commented May 13, 2023

everywhere with wrapped id vars in clickhouse allows unwrapped? your change seems a bit dangerous

Yes, as far as I can see in the ClickHouse code.
But I will re-check it.
It has not a lot of places with id lists: primary|foreign key and using
Will update

@pkit
Copy link
Contributor Author

pkit commented May 13, 2023

It looks good.
Right now in sqlglot _parse_wrapped_id_vars() is used in the follwoing places:

  • primary|foreign key - ok
  • join using - ok
  • references - (only if ( is present)
  • unique - (only if ( is present)

@pkit pkit changed the title Fix(clickhouse): USING allows unwrapped col list Fix(clickhouse): USING, [PRIMARY|FOREIGN] KEY allows unwrapped col list May 13, 2023
@tobymao
Copy link
Owner

tobymao commented May 13, 2023

thanks for checking i’ll review and merge tonight. at a wedding today.

@tobymao tobymao merged commit 6875d07 into tobymao:main May 14, 2023
@pkit pkit deleted the ch_wrapped_opt_id branch May 14, 2023 03:03
adrianisk pushed a commit to adrianisk/sqlglot that referenced this pull request Jun 21, 2023
I.e. both `USING a, b, c` and `USING (a, b, c)` are allowed
Although the wrapped variant is a canonical one (see docs)

Co-authored-by: Toby Mao <toby.mao@gmail.com>
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.

2 participants