Fallback to identifier parsing if expression parsing fails #1513
Conversation
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
src/parser/mod.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn parse_ident_expr(&mut self, w: &Word) -> Result<Expr, ParserError> { |
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
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 |
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
src/parser/mod.rs
Outdated
| Ok(None) => Ok(self.parse_expr_prefix_by_unnreserved_word(&w)?), | ||
|
|
||
| // If parsing of the word as a special expression failed, we are facing two options: | ||
| // 1. The statement is malformed, e.g. `SELECT INTERVAL '1 DAI` |
There was a problem hiding this comment.
| // 1. The statement is malformed, e.g. `SELECT INTERVAL '1 DAI` | |
| // 1. The statement is malformed, e.g. `SELECT INTERVAL '1 DAY` |
There was a problem hiding this comment.
This is intentional, DAI is what makes this statement malformed :-)
src/parser/mod.rs
Outdated
| /// Parse an expression prefix. | ||
| pub fn parse_prefix2(&mut self) -> Result<Expr, ParserError> { |
There was a problem hiding this comment.
Ah this version seems unused we can remove?
There was a problem hiding this comment.
Shit... Yes, on it.
src/parser/mod.rs
Outdated
| } | ||
|
|
||
| // Tries to parse an expression by a word that is not known to have a special meaning in the dialect. | ||
| fn parse_expr_prefix_by_unnreserved_word(&mut self, w: &Word) -> Result<Expr, ParserError> { |
There was a problem hiding this comment.
| fn parse_expr_prefix_by_unnreserved_word(&mut self, w: &Word) -> Result<Expr, ParserError> { | |
| fn parse_expr_prefix_by_unreserved_word(&mut self, w: &Word) -> Result<Expr, ParserError> { |
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| { |
There was a problem hiding this comment.
| 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?
src/parser/mod.rs
Outdated
| } | ||
|
|
||
| /// Run a parser method `f`, reverting back to the current position if unsuccessful. | ||
| pub fn maybe_parse_internal<T, F>(&mut self, mut f: F) -> Result<T, ParserError> |
There was a problem hiding this comment.
| pub fn maybe_parse_internal<T, F>(&mut self, mut f: F) -> Result<T, ParserError> | |
| pub fn try_parse<T, F>(&mut self, mut f: F) -> Result<T, ParserError> |
We can probably call try_parse which could hint that it returns an error (vs maybe_parse which hints at an option)
| @@ -214,6 +216,16 @@ impl Dialect for SnowflakeDialect { | |||
| fn supports_show_like_before_in(&self) -> bool { | |||
| true | |||
| } | |||
|
|
|||
| fn is_reserved_for_identifier(&self, kw: Keyword) -> bool { | |||
There was a problem hiding this comment.
oh are we missing tests for the new behavior (I can't seem to find them in the current PR if so)?
…ifier if the expression parsing fails
yoavcloud
force-pushed
the
expr_ident_mixup
branch
from
1c5c90a
to
e4ebe44
Compare
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.