[snowflake] Support `FROM (table_name) alias` by eyalleshem · Pull Request #260 · apache/datafusion-sqlparser-rs

eyalleshem · 2020-08-12T05:26:32Z

Initially discussed in #223 and #244. Closes #223.

The description copied from the commit:

Snowflake diverges from the standard and from most of the other implementations by allowing extra parentheses not only around a join, but around lone table names (e.g. FROM (mytable [AS alias])) and around derived tables (e.g. FROM ((SELECT ...) [AS alias])) as well.

Initially this was implemented in #154 by (ab)using TableFactor::NestedJoin to represent anything nested in extra set of parens.

Afterwards we learned in #223 that in cases of such extraneous nesting Snowflake allows specifying the alias both inside and outside parens, but not both - consider:

FROM (table_factor AS inner_alias) AS outer_alias

We've considered implementing this by changing TableFactor::NestedJoin to a TableFactor::Nested { inner: TableWithJoins, alias: Option<TableAlias> }, but that seemed too generic, as no known dialect supports duplicate aliases, as shown above, nor naming nested joins (foo NATURAL JOIN bar) alias. So we decided on making a smaller change (with no modifications to the AST), that is also more appropriate to the contributors to the Snowflake dialect:

Revert Fail to parse - "select * from (table_name)" #154 by rejecting FROM (table or derived table) in most dialects.
For dialect_of!(self is SnowflakeDialect | GenericDialect) parse
and strip the extraneous parentheses:

(mytable) AS alias -> (mytable AS alias)

coveralls · 2020-08-12T05:29:11Z

Pull Request Test Coverage Report for Build 303837987

51 of 56 (91.07%) changed or added relevant lines in 3 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.08%) to 91.941%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tests/sqlparser_snowflake.rs	21	23	91.3%
src/parser.rs	18	21	85.71%

Files with Coverage Reduction	New Missed Lines	%
src/parser.rs	1	89.23%

Totals
Change from base Build 300155852:	-0.08%
Covered Lines:	4803
Relevant Lines:	5224

💛 - Coveralls

tests/sqlparser_common.rs

nickolay · 2020-08-12T16:14:24Z

tests/sqlparser_common.rs

+#[macro_use]
+#[path = "utils/mod.rs"]
+mod utils;
+
+use utils::*;


There's test_utils.rs for that already.

The actual reason that i don't use the test util is that i want to share the macro between the test ,
And if I understand it correct , the src and the the test are not considered in the same crate , so i can export macro only via #[macro_export] directive, but that mean the macro will be available not only for the test but to all users of the lib , and that something that i want to avoid (please correct me if i wrong..).
Anyway - I moved the join and table macros to test_utils.rs and change the utils dir that i crated to macros dir, hope you ok with that.
btw - why test_util.rs located inside the source ?

tests/sqlparser_snowflake.rs

tests/sqlparser_common.rs

tests/sqlparser_snowflake.rs

Co-authored-by: Eyal Leshem <eyal@satoricyber.com>

nickolay · 2020-10-05T05:57:39Z

I first rebased this onto main without changes, then split into two commits (moving things around and implementing the new functionality), as I wanted to do all along...

nickolay · 2020-10-05T06:03:54Z

Regarding the need to share helper macros, as well as helper functions, discussed in #260 (comment) -- I added a commit a3a0d7b that demonstrates how I'd like to solve this. I added some comments that I hope will explain my reasoning, please let me know if they're not clear.

The second part of the PR (and #260 (comment) ) I want to look over again before merging.

eyalsatori · 2020-10-05T20:32:21Z

Thanks a lot for the rebasing and the fixes!
The solution of the test_utils indeed much more elegant .

Is there any change needed from me , or you just want to continue review it?

nickolay · 2020-10-06T09:55:31Z

No, I was just running out of time that night, and wanted to share the part I believed to be ready. I'll try to get to the other one sometime "soon"...

To share helper macros between various tests/* we added a new module (tests/macros/mod.rs). This made the prologue to be used in tests quite long and a little weird: ``` #[macro_use] #[path = "macros/mod.rs"] mod macros; use sqlparser::test_utils::*; ``` This simplifies it to: ``` #[macro_use] mod test_utils; use test_utils::*; ``` - and switches all existing tests to the new prologue simultaneously... ...while fixing a few other inconsistencies and adding a few comments about the way `test_utils` work.

It was an omission of the original implementation.

nickolay · 2020-10-12T04:02:12Z

Can't you use verified_only_select(sql).from instead of creating a new helper?

It's a problem to use it because we remove the information about we ignore the information about the redundant parenthesis in the AST , so the input sql will be different than the output .

I don't know why it took me so long to realize it, but we use one_statement_parses_to in cases like this. I added commits that do that to this PR and if you're OK with this, I'm ready to merge.

eyalsatori · 2020-10-12T06:17:28Z

I am ok with that ,
Thanks a lot :)

Snowflake diverges from the standard and from most of the other implementations by allowing extra parentheses not only around a join, but around lone table names (e.g. `FROM (mytable [AS alias])`) and around derived tables (e.g. `FROM ((SELECT ...) [AS alias])`) as well. Initially this was implemented in apache#154 by (ab)using `TableFactor::NestedJoin` to represent anything nested in extra set of parens. Afterwards we learned in apache#223 that in cases of such extraneous nesting Snowflake allows specifying the alias both inside and outside parens, but not both - consider: FROM (table_factor AS inner_alias) AS outer_alias We've considered implementing this by changing `TableFactor::NestedJoin` to a `TableFactor::Nested { inner: TableWithJoins, alias: Option<TableAlias> }`, but that seemed too generic, as no known dialect supports duplicate aliases, as shown above, nor naming nested joins `(foo NATURAL JOIN bar) alias`. So we decided on making a smaller change (with no modifications to the AST), that is also more appropriate to the contributors to the Snowflake dialect: 1) Revert apache#154 by rejecting `FROM (table or derived table)` in most dialects. 2) For `dialect_of!(self is SnowflakeDialect | GenericDialect)` parse and strip the extraneous parentheses, e.g. `(mytable) AS alias` -> `(mytable AS alias)` Co-authored-by: Eyal Leshem <eyal@satoricyber.com>

waitingkuo · 2022-08-03T15:32:42Z

@eyalleshem @nickolay i made a pull request #551 for nested table with alias

nickolay changed the title ~~support the single table in parnes with alias in snowflake by dialect~~ [snowflake] Support FROM (table_name) alias Aug 12, 2020

nickolay suggested changes Aug 12, 2020

View reviewed changes

eyalleshem force-pushed the single_tables_in_parens branch from 1ad467f to 44290ab Compare August 13, 2020 14:38

eyalleshem requested a review from nickolay August 13, 2020 14:59

eyalleshem force-pushed the single_tables_in_parens branch from 44290ab to 7355eff Compare August 13, 2020 15:00

Move existing SF tests to sqlparser_snowflake.rs

99fb633

Co-authored-by: Eyal Leshem <eyal@satoricyber.com>

nickolay force-pushed the single_tables_in_parens branch 2 times, most recently from 2280580 to 92c6b49 Compare October 5, 2020 05:54

nickolay added 2 commits October 12, 2020 06:52

Extend one_statement_parses_to to also test parsing canonical SQL

d9e044a

It was an omission of the original implementation.

nickolay force-pushed the single_tables_in_parens branch from 1c546de to ad72cda Compare October 13, 2020 06:55

nickolay approved these changes Oct 13, 2020

View reviewed changes

nickolay merged commit 929fc67 into apache:main Oct 13, 2020

alamb mentioned this pull request Aug 3, 2022

Support NestedJoin with an alias #551

Merged

Conversation

eyalleshem commented Aug 12, 2020 • edited by nickolay Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 303837987

💛 - Coveralls

Uh oh!

Uh oh!

nickolay Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

eyalleshem Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickolay commented Oct 5, 2020

Uh oh!

nickolay commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eyalsatori commented Oct 5, 2020

Uh oh!

nickolay commented Oct 6, 2020

Uh oh!

nickolay commented Oct 12, 2020

Uh oh!

eyalsatori commented Oct 12, 2020

Uh oh!

waitingkuo commented Aug 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eyalleshem commented Aug 12, 2020 •

edited by nickolay

Loading

coveralls commented Aug 12, 2020 •

edited

Loading

nickolay commented Oct 5, 2020 •

edited

Loading