-
Notifications
You must be signed in to change notification settings - Fork 618
Consolidate MapAccess
, and Subscript
into CompoundExpr
to handle the complex field access chain
#1551
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
Consolidate MapAccess
, and Subscript
into CompoundExpr
to handle the complex field access chain
#1551
Changes from 1 commit
192cab4
6be3c35
0e916dd
767b531
22f4e67
dee8b40
fc1cd59
8590896
4ad37d8
31a1e74
0355290
1de9b21
2a32b9f
495d1b3
e7b55be
6652905
47a5da1
b58e50c
7cb2e00
397335a
ac25e5d
a08e5c2
09b39eb
8968fcc
1328274
d6743e9
7d030c1
90e03eb
5c54d1b
57830e2
4b3818c
67cd877
23aea03
94847d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1148,17 +1148,29 @@ impl<'a> Parser<'a> { | |
Token::Period => self.parse_compound_expr(Expr::Identifier(w.to_ident(w_span)), vec![]), | ||
Token::LParen => { | ||
let id_parts = vec![w.to_ident(w_span)]; | ||
let mut expr = self.parse_function(ObjectName(id_parts))?; | ||
// consume all period if it's a method chain | ||
if self.dialect.supports_methods() { | ||
expr = self.try_parse_method(expr)? | ||
} | ||
let mut fields = vec![]; | ||
// if the function returns an array, it can be subscripted | ||
if self.consume_token(&Token::LBracket) { | ||
self.parse_multi_dim_subscript(&mut fields)?; | ||
// parse_comma_outer_join is used to parse the following pattern: | ||
goldmedal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if dialect_of!(self is SnowflakeDialect | MsSqlDialect) | ||
&& self.consume_tokens(&[Token::LParen, Token::Plus, Token::RParen]) | ||
{ | ||
Ok(Expr::OuterJoin(Box::new( | ||
match <[Ident; 1]>::try_from(id_parts) { | ||
Ok([ident]) => Expr::Identifier(ident), | ||
Err(parts) => Expr::CompoundIdentifier(parts), | ||
}, | ||
))) | ||
} else { | ||
let mut expr = self.parse_function(ObjectName(id_parts))?; | ||
// consume all period if it's a method chain | ||
if self.dialect.supports_methods() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, if this condition was removed, the test for method parsing will fail:
Because the following There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, to clarify what I meant was that if !self.dialect.supports_methods() {
return Ok(expr);
} So that this can be simplified as following (i.e without the extra expr = self.try_parse_method(expr)?; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed. It's more simple. Thanks! |
||
expr = self.try_parse_method(expr)? | ||
} | ||
let mut fields = vec![]; | ||
// if the function returns an array, it can be subscripted | ||
if self.consume_token(&Token::LBracket) { | ||
self.parse_multi_dim_subscript(&mut fields)?; | ||
goldmedal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
self.parse_compound_expr(expr, fields) | ||
} | ||
self.parse_compound_expr(expr, fields) | ||
} | ||
Token::LBracket => { | ||
let _ = self.consume_token(&Token::LBracket); | ||
|
@@ -1420,15 +1432,21 @@ impl<'a> Parser<'a> { | |
mut chain: Vec<AccessField>, | ||
) -> Result<Expr, ParserError> { | ||
let mut ending_wildcard: Option<TokenWithSpan> = None; | ||
let mut ending_lbracket = false; | ||
while self.consume_token(&Token::Period) { | ||
let next_token = self.next_token(); | ||
match next_token.token { | ||
Token::Word(w) => { | ||
let expr = Expr::Identifier(w.to_ident(next_token.span)); | ||
chain.push(AccessField::Expr(expr)); | ||
if self.consume_token(&Token::LBracket) && !self.dialect.supports_partiql() { | ||
self.parse_multi_dim_subscript(&mut chain)? | ||
}; | ||
if self.consume_token(&Token::LBracket) { | ||
if self.dialect.supports_partiql() { | ||
ending_lbracket = true; | ||
break; | ||
} else { | ||
self.parse_multi_dim_subscript(&mut chain)? | ||
} | ||
} | ||
} | ||
Token::Mul => { | ||
// Postgres explicitly allows funcnm(tablenm.*) and the | ||
|
@@ -1450,6 +1468,12 @@ impl<'a> Parser<'a> { | |
} | ||
} | ||
|
||
// if dialect supports partiql, we need to go back one Token::LBracket for the JsonAccess parsing | ||
if self.dialect.supports_partiql() && ending_lbracket { | ||
self.prev_token(); | ||
} | ||
|
||
|
||
if let Some(wildcard_token) = ending_wildcard { | ||
iffyio marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let Some(id_parts) = Self::exprs_to_idents(&root, &chain) else { | ||
return self.expected("an identifier or a '*' after '.'", self.peek_token()); | ||
|
@@ -3075,11 +3099,9 @@ impl<'a> Parser<'a> { | |
{ | ||
let mut chain = vec![]; | ||
self.parse_multi_dim_subscript(&mut chain)?; | ||
Ok(Expr::CompoundExpr { | ||
root: Box::new(expr), | ||
chain, | ||
}) | ||
} else if dialect_of!(self is SnowflakeDialect) || self.dialect.supports_partiql() { | ||
self.parse_compound_expr(expr, chain) | ||
|
||
} else if self.dialect.supports_partiql() { | ||
self.prev_token(); | ||
self.parse_json_access(expr) | ||
} else { | ||
|
@@ -3266,14 +3288,17 @@ impl<'a> Parser<'a> { | |
pub fn parse_map_access(&mut self, expr: Expr) -> Result<Expr, ParserError> { | ||
let key = self.parse_expr()?; | ||
let result = match key { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm I was initially wondering about this match since it looked incorrect to restrict the key expression type. e.g. this BigQuery test case. But then I realised that test case is passing because it now takes a different codepath via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. We can remove it entirely. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed by 5c54d1b |
||
Expr::Identifier(ident) => Ok(Expr::CompositeAccess { | ||
expr: Box::new(expr), | ||
key: ident, | ||
Expr::Identifier(_) => Ok(Expr::CompoundExpr { | ||
root: Box::new(expr), | ||
chain: vec![AccessField::Expr(key)], | ||
}), | ||
Expr::Value(Value::SingleQuotedString(s)) | ||
| Expr::Value(Value::DoubleQuotedString(s)) => Ok(Expr::CompositeAccess { | ||
expr: Box::new(expr), | ||
key: Ident::new(s), | ||
Expr::Value(Value::SingleQuotedString(_)) => Ok(Expr::CompoundExpr { | ||
root: Box::new(expr), | ||
chain: vec![AccessField::Expr(key)], | ||
}), | ||
Expr::Value(Value::DoubleQuotedString(s)) => Ok(Expr::CompoundExpr { | ||
root: Box::new(expr), | ||
chain: vec![AccessField::Expr(Expr::Identifier(Ident::new(s)))], | ||
}), | ||
_ => parser_err!("Expected identifier or string literal", self.peek_token()), | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I don't think this is necessarily correct since partiql is a redshift feature, was this required somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to integrate the conditions at
datafusion-sqlparser-rs/src/parser/mod.rs
Lines 2998 to 3000 in d0fcc06
Then, we can only check
supports_partiql
inparse_compound_expr
. 🤔Indeed, the name is a little weird for
Snowflake
but I think they mean the same thing 🤔