Skip to content

Support nested expressions in BETWEEN #80

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

Merged
merged 1 commit into from
May 29, 2019

Conversation

benesch
Copy link
Contributor

@benesch benesch commented May 24, 2019

BETWEEN <thing> AND <thing> allows to be any expr that doesn't
contain boolean operators. (Allowing boolean operators would wreak
havoc, because of the repurposing of AND as both a boolean operation
and part of the syntax of BETWEEN.)

@coveralls
Copy link

coveralls commented May 24, 2019

Pull Request Test Coverage Report for Build 207

  • 35 of 43 (81.4%) changed or added relevant lines in 2 files are covered.
  • 7 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.2%) to 89.368%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/sqlparser_common.rs 32 40 80.0%
Files with Coverage Reduction New Missed Lines %
src/test_utils.rs 2 89.06%
src/sqlparser.rs 5 88.69%
Totals Coverage Status
Change from base Build 183: 0.2%
Covered Lines: 2900
Relevant Lines: 3245

💛 - Coveralls

@nickolay
Copy link
Contributor

Huh, I completely missed the fact that BETWEEN supports expressions when I wrote this (andygrove@786b1cf). Thanks for figuring this out!

I think this works by re-using BETWEEN's precedence in two distinct scenarios:

  1. as the left-binding precedence (this worked before this change), which:
    • being lower than the precedence of binary arithmetic operators makes 1+1 BETWEEN .. parse as (1+1) BETWEEN ..
    • being higher than the precedence of AND/OR makes expr AND bar BETWEEN .. parse as expr AND (bar BETWEEN ..)
  2. as the right-binding precedence, which stops parsing the expressions for the range bounds whenever a token with a lower precedence (such as AND, but also IS and others) is encountered.

I feel an urge to add a comment to that effect, maybe:

// Stop parsing subexpressions for <low> and <high> on tokens with
// precedence lower than that of `BETWEEN` (such as `AND`, `IS`, etc.)
let prec = self.get_precedence(&Token::make_keyword("BETWEEN"))?; 
let low = self.parse_subexpr(prec)?;

I'd also extend the test to check that where 1=1 and 1+x between ... still parses correctly as we don't appear to have an existing test for this.

`BETWEEN <thing> AND <thing>` allows <thing> to be any expr that doesn't
contain boolean operators. (Allowing boolean operators would wreak
havoc, because of the repurposing of AND as both a boolean operation
and part of the syntax of BETWEEN.)
@benesch
Copy link
Contributor Author

benesch commented May 28, 2019

Huh, I completely missed the fact that BETWEEN supports expressions when I wrote this (786b1cf). Thanks for figuring this out!

You bet! I'm working towards getting the parser to parse all the queries in https://www.sqlite.org/sqllogictest. There's only about 3k queries left that cause parse failures.

Your explanation of how this works is spot on. I've added the comment as you suggested.

I'd also extend the test to check that where 1=1 and 1+x between ... still parses correctly as we don't appear to have an existing test for this.

Good idea. Done.

@nickolay nickolay merged commit d80f9f3 into apache:master May 29, 2019
@benesch benesch deleted the between-expr branch June 21, 2019 22:34
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