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

accept JSON_TABLE both as an unquoted table name and a table-valued function #1134

Merged
merged 6 commits into from
Feb 15, 2024

Conversation

lovasoa
Copy link
Contributor

@lovasoa lovasoa commented Feb 14, 2024

This is an alternative to #1123

closes #1123
related to apache/datafusion#9122

see #1123 (comment)

This makes sqlparser accept both

select * from json_table 

And

select * from json_table(t, '$[*]' COLUMNS (id  INT PATH '$.id'))

in all dialects

@coveralls
Copy link

coveralls commented Feb 14, 2024

Pull Request Test Coverage Report for Build 7905402308

Details

  • -1 of 11 (90.91%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 87.687%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/sqlparser_postgres.rs 6 7 85.71%
Totals Coverage Status
Change from base Build 7878309063: 0.001%
Covered Lines: 19655
Relevant Lines: 22415

💛 - Coveralls

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @lovasoa -- I like this PR a lot

tests/sqlparser_postgres.rs Outdated Show resolved Hide resolved
@alamb
Copy link
Contributor

alamb commented Feb 14, 2024

cc @viirya

@viirya
Copy link
Member

viirya commented Feb 14, 2024

Okay for me. Although I remember this is what @lovasoa is against to at the beginning...

EDIT: looks like it is not valid in supported engines as it is reserved keyword.

@lovasoa
Copy link
Contributor Author

lovasoa commented Feb 14, 2024

@viirya , this PR implements what I had suggested in apache/datafusion#9122 (comment)

@viirya
Copy link
Member

viirya commented Feb 14, 2024

@lovasoa I meant apache/datafusion#9122 (comment) (I think this is what your stand at the beginning).

@lovasoa
Copy link
Contributor Author

lovasoa commented Feb 14, 2024

Yes, my initial comment was about just ignoring the issue, which did not seem to satisfy everyone. This implements something even better that should make everyone happy.

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

Are you sure select * from json_table is valid in the engines which support json_table table function?

It is a reserved keyword in MySQL: https://dev.mysql.com/doc/refman/8.0/en/keywords.html#keywords-8-0-detailed-J

@lovasoa
Copy link
Contributor Author

lovasoa commented Feb 14, 2024

As stated above, this PR makes the parser accept BOTH the table identifier and the function in all dialects. I think this is a reasonable behavior.
Actual databases implement either one or the other.

@alamb
Copy link
Contributor

alamb commented Feb 14, 2024

Are you sure select * from json_table is valid in the engines which support json_table table function?

I tried running select * from json_table using dbfiddle: https://www.db-fiddle.com/f/ejJ4RKUDrtftShwtsLMCfA/1

It works in posgres and sqlite

mysql seems to require quoted identifiers:
Screenshot 2024-02-14 at 3 53 38 PM

Screenshot 2024-02-14 at 3 53 09 PM

@lovasoa
Copy link
Contributor Author

lovasoa commented Feb 15, 2024

@alamb : Yes, databases implement either the unquoted table reference syntax, or the table valued function syntax. But sqlparser now implements both.

@alamb alamb merged commit 1a07c5d into apache:main Feb 15, 2024
10 checks passed
@alamb
Copy link
Contributor

alamb commented Feb 15, 2024

Thanks again @lovasoa and @viirya

@lovasoa
Copy link
Contributor Author

lovasoa commented Feb 15, 2024

👍

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.

4 participants