Fallback to identifier parsing if expression parsing fails #1513
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yoavcloud
force-pushed
the
expr_ident_mixup
branch
from
787e396 to
d39af8d
Compare
yoavcloud
changed the title
yoavcloud
force-pushed
the
expr_ident_mixup
branch
2 times, most recently
from
77243eb to
6685cce
Compare
iffyio
reviewed
Nov 14, 2024
src/parser/mod.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn parse_ident_expr(&mut self, w: &Word) -> Result<Expr, ParserError> { |
Contributor
There was a problem hiding this comment.
hmm thinking we would need to rename the function, it seems to be doing more than identifiers (and based on the name its unclear what the word w input argument is for)
yoavcloud
force-pushed
the
expr_ident_mixup
branch
from
6685cce to
a2d92a4
Compare
iffyio
reviewed
Nov 18, 2024
src/parser/mod.rs
Outdated
| Ok(Some(expr)) => Ok(expr), | ||
| // This word indicated an expression prefix but parsing failed. Two options: | ||
| // 1. Malformed statement | ||
| // 2. The dialect may allow this word as identifier as well as indicating an expression |
Contributor
There was a problem hiding this comment.
The comments are indeed helpful to follow thanks! Thinking here we could clarify 2. even further with the SELECT MAX(interval) from tbl example?
yoavcloud
force-pushed
the
expr_ident_mixup
branch
from
a2d92a4 to
1c5c90a
Compare
iffyio
reviewed
Nov 20, 2024
Contributor
iffyio
left a comment
iffyio
left a comment
There was a problem hiding this comment.
The changes look good to me overall! Left some minor comments then one regarding tests to demonstrate the new behavior
src/parser/mod.rs
Outdated
| // special expression (to maintain backwards compatibility of parsing errors). | ||
| Err(e) => { | ||
| if !self.dialect.is_reserved_for_identifier(w.keyword) { | ||
| if let Ok(expr) = self.maybe_parse_internal(|parser| { |
Contributor
There was a problem hiding this comment.
Suggested change
| if let Ok(expr) = self.maybe_parse_internal(|parser| { | |
| if let Some(expr) = self.maybe_parse(|parser| { |
it looks like we can use the normal maybe_parse here since it doesn't have the special requirement?
yoavcloud
force-pushed
the
expr_ident_mixup
branch
from
1c5c90a to
e4ebe44
Compare
iffyio
approved these changes
Nov 22, 2024
Contributor
|
This PR appears to have a small conflict |
…ifier if the expression parsing fails
yoavcloud
force-pushed
the
expr_ident_mixup
branch
from
e4ebe44 to
4486c20
Compare
Contributor
Author
|
@alamb resolved |
Contributor
|
Thanks @yoavcloud |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes the following issue: #1496
The parser encounters situations when the next keyword indicates an expression, but in fact it should be parsed as an identifier. Example from Snowflake:
SELECT MAX(interval) from tbl. When the parser encounters theintervalword it tries to parse anExpr::Intervalbut it fails because in the context of this query,intervalis an identifier that Snowflake (unlike most other dialects) allows in unquoted form.The suggested approach is to try to parse the expression, but if that fails, fallback to parse an identifier under certain conditions. In addition, each dialect can now declare which keywords it reserves for use as an identifier in unquoted form.
Lastly, changed the parsing of the
DEFAULTword to Expr::Default instead of an identifier.