Skip to content

Fix the precedence of NOT LIKE #82

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 2 commits into from
Jun 2, 2019
Merged

Fix the precedence of NOT LIKE #82

merged 2 commits into from
Jun 2, 2019

Conversation

benesch
Copy link
Contributor

@benesch benesch commented May 27, 2019

NOT LIKE has the same precedence as the LIKE operator. The parser was
previously assigning it the precedence of the unary NOT operator.

The fix is a bit involved, as the precedence of NOT depends on the keyword that follows it.

FYI this will conflict with #80, but I'll rebase whichever gets merged second.

NOT LIKE has the same precedence as the LIKE operator. The parser was
previously assigning it the precedence of the unary NOT operator. NOT
BETWEEN and NOT IN are treated similarly, as they are equivalent, from a
precedence perspective, to NOT LIKE.

The fix for this requires associating precedences with sequences of
tokens, rather than single tokens, so that "NOT LIKE" and "NOT "
can have different preferences. Perhaps surprisingly, this change is not
very invasive.

An alternative I considered involved adjusting the tokenizer to lex
NOT, NOT LIKE, NOT BETWEEN, and NOT IN as separate tokens. This broke
symmetry in strange ways, though, as NotLike, NotBetween, and NotIn
gained dedicated tokens, while LIKE, BETWEEN, and IN remained as
stringly identifiers.

Fixes #81.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 205

  • 90 of 98 (91.84%) changed or added relevant lines in 2 files are covered.
  • 35 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.4%) to 89.515%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/sqlparser_common.rs 49 57 85.96%
Files with Coverage Reduction New Missed Lines %
src/test_utils.rs 2 89.06%
src/sqlparser.rs 6 88.71%
tests/sqlparser_common.rs 27 93.45%
Totals Coverage Status
Change from base Build 183: 0.4%
Covered Lines: 2877
Relevant Lines: 3214

💛 - Coveralls

@coveralls
Copy link

coveralls commented May 27, 2019

Pull Request Test Coverage Report for Build 225

  • 95 of 102 (93.14%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 89.501%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/sqlparser_common.rs 52 59 88.14%
Totals Coverage Status
Change from base Build 222: 0.03%
Covered Lines: 3086
Relevant Lines: 3448

💛 - Coveralls

This will be used in a future commit, where looking ahead by two tokens
is important.
@benesch
Copy link
Contributor Author

benesch commented May 31, 2019

This is rebased and ready to go! I split the original patch into two commits to hopefully make the details of the change more clear.

NOT LIKE has the same precedence as the LIKE operator. The parser was
previously assigning it the precedence of the unary NOT operator. NOT
BETWEEN and NOT IN are treated similarly, as they are equivalent, from a
precedence perspective, to NOT LIKE.

The fix for this requires associating precedences with sequences of
tokens, rather than single tokens, so that "NOT LIKE" and "NOT <expr>"
can have different preferences. Perhaps surprisingly, this change is not
very invasive.

An alternative I considered involved adjusting the tokenizer to lex
NOT, NOT LIKE, NOT BETWEEN, and NOT IN as separate tokens. This broke
symmetry in strange ways, though, as NotLike, NotBetween, and NotIn
gained dedicated tokens, while LIKE, BETWEEN, and IN remained as
stringly identifiers.

Fixes apache#81.
@nickolay
Copy link
Contributor

nickolay commented Jun 2, 2019

LGTM, thanks!
(I didn't double-check the testcases, as it looks like you've given it much thought already.)

@benesch benesch merged commit 1cc9d2d into apache:master Jun 2, 2019
@benesch benesch deleted the not-prec branch June 2, 2019 14:49
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.

Precedence of NOT isn't quite right
3 participants