Skip to content

Precedence of NOT isn't quite right #81

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

Closed
benesch opened this issue May 27, 2019 · 0 comments · Fixed by #82
Closed

Precedence of NOT isn't quite right #81

benesch opened this issue May 27, 2019 · 0 comments · Fixed by #82

Comments

@benesch
Copy link
Contributor

benesch commented May 27, 2019

It took me a while to come up with this example, and it's not totally realistic, but it's probably still worth fixing. Basically, the precedence of NOT LIKE is mishandled, because the precedence of NOT is fixed to its unary operator precedence, when in it needs to vary based on whether the NOT keyword is followed by LIKE/BETWEEN/IN/.

Concretely, SELECT a LIKE b IS NULL parses correctly, with the LIKE binding more tightly than the IS NULL:

SQLIsNull(
    SQLBinaryExpr {
        left: SQLIdentifier(
            "a"
        ),
        op: Like,
        right: SQLIdentifier(
            "b"
        )
    }
)

But when NOT LIKE is substituted for LIKE, the IS NULL incorrectly binds more tightly than the NOT LIKE:

SQLBinaryExpr {
    left: SQLIdentifier(
        "a"
    ),
    op: NotLike,
    right: SQLIsNull(
        SQLIdentifier(
            "b"
        )
    )
}
benesch added a commit to benesch/sqlparser-rs that referenced this issue 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.

Fixes apache#81.
benesch added a commit to benesch/sqlparser-rs that referenced this issue 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.

Fixes apache#81.
benesch referenced this issue in MaterializeInc/materialize May 27, 2019
Udate sqlparser to pull in a fix for andygrove/sqlparser-rs#81 that was
causing sqllogictests to fail.
benesch added a commit to benesch/sqlparser-rs that referenced this issue May 31, 2019
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.
benesch added a commit to benesch/sqlparser-rs that referenced this issue Jun 1, 2019
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.
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 a pull request may close this issue.

1 participant