Skip to content

Support for CONNECT BY#1138

Merged
alamb merged 4 commits intoapache:mainfrom
jmhain:connectby
Apr 27, 2024
Merged

Support for CONNECT BY#1138
alamb merged 4 commits intoapache:mainfrom
jmhain:connectby

Conversation

@jmhain
Copy link
Copy Markdown
Contributor

@jmhain jmhain commented Feb 16, 2024

This adds support for the CONNECT BY operator, supported by Snowflake, Redshift, and MS SQL.

A bit of awkwardness is the PRIOR expressions used to refer to the upper level in the CONNECT BY relationships. These expressions are only valid in this context, and prior is a perfectly valid identifier elsewhere. Trying to thread that context through the call stack was resulting in a massive diff, so I added some state to the parser to avoid that. Happy to change this is reviewers are not fond of that approach.

@coveralls
Copy link
Copy Markdown

coveralls commented Feb 16, 2024

Pull Request Test Coverage Report for Build 8852954812

Details

  • 161 of 189 (85.19%) changed or added relevant lines in 15 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 89.214%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/dialect/generic.rs 1 2 50.0%
src/dialect/mod.rs 1 2 50.0%
src/dialect/mssql.rs 1 2 50.0%
src/dialect/redshift.rs 1 2 50.0%
src/dialect/snowflake.rs 1 2 50.0%
src/ast/query.rs 5 7 71.43%
src/parser/mod.rs 34 37 91.89%
src/test_utils.rs 0 4 0.0%
tests/sqlparser_common.rs 99 113 87.61%
Files with Coverage Reduction New Missed Lines %
tests/sqlparser_common.rs 6 89.58%
Totals Coverage Status
Change from base Build 8852365976: -0.03%
Covered Lines: 23622
Relevant Lines: 26478

💛 - Coveralls

Copy link
Copy Markdown
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 very much for this contribution @jmhain -- I am truly sorry for the delay in review.

I think the approach taken by this PR looks good to me. In my opinion, all that is needed prior to merge is:

  1. Some additional comments (I left some suggestions)
  2. A few negative test cases

Thanks again for such an easy to read and review PR

Comment thread src/ast/query.rs Outdated
Comment thread src/parser/mod.rs
Comment thread tests/sqlparser_snowflake.rs
@alamb alamb marked this pull request as draft February 29, 2024 14:15
@alamb
Copy link
Copy Markdown
Contributor

alamb commented Feb 29, 2024

Marking as Draft to signify this PR is no longer waiting on review. Please mark it as ready for review when it is ready for another look.

@jmhain jmhain marked this pull request as ready for review April 14, 2024 01:59
@jmhain
Copy link
Copy Markdown
Contributor Author

jmhain commented Apr 14, 2024

@alamb Many thanks for the review feedback, I greatly appreciate it! And sorry for the delay in getting back to this. So I learned that MS SQL and Redshift both also support CONNECT BY, and I did some more testing and found that my approach before was really only well suited to Snowflake, so I reworked this significantly. I also added some more tests. Thanks again!

@jmhain jmhain changed the title snowflake: support for CONNECT BY Support for CONNECT BY Apr 14, 2024
@jmhain
Copy link
Copy Markdown
Contributor Author

jmhain commented Apr 26, 2024

Just resolved conflicts on this again cc @alamb

Copy link
Copy Markdown
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.

Thanks again @jmhain

@alamb
Copy link
Copy Markdown
Contributor

alamb commented Apr 26, 2024

Looks like there is a conflict (perhaps with one of your other PRs 😆 ) -- if you can resolve that I'll merge this PR in.

Thanks again

@jmhain
Copy link
Copy Markdown
Contributor Author

jmhain commented Apr 26, 2024

@alamb Done, thank you!

@alamb
Copy link
Copy Markdown
Contributor

alamb commented Apr 27, 2024

🚀

@alamb alamb merged commit 0b5722a into apache:main Apr 27, 2024
JichaoS pushed a commit to luabase/sqlparser-rs that referenced this pull request May 7, 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.

3 participants