iffyio commented on code in PR #1500:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1500#discussion_r1833051964


##########
src/parser/mod.rs:
##########
@@ -1271,10 +1271,22 @@ impl<'a> Parser<'a> {
                             )
                         }
                     };
-                    Ok(Expr::CompositeAccess {
-                        expr: Box::new(expr),
-                        key,
-                    })
+                    if self.consume_token(&Token::LParen) {
+                        self.prev_token();
+                        let func = match 
self.parse_function(ObjectName(vec![key]))? {
+                            Expr::Function(func) => func,
+                            _ => unreachable!(),

Review Comment:
   Oh I think this panic won't be desirable here in any case given we don't 
have an invariant and `self.parse_function` could be in its own right to return 
something other than `Expr::Function` in a different situation.
   
   Also given the nature of the syntax that we don't have any special keyword 
to match on, feels a bit uneasy that we could end up parsing sql incorrectly in 
other dialects in ambiguous cases.
   
   could we do similar to e.g. 
[try_parse_lambda](https://github.com/apache/datafusion-sqlparser-rs/blob/main/src/parser/mod.rs#L1332-L1346)
 where this is both specific to mssql dialect, as well as potentially relying 
on `self.maybe_parse` to bail out if the lookahead isn't what we expect?



##########
src/parser/mod.rs:
##########
@@ -1288,6 +1300,35 @@ impl<'a> Parser<'a> {
             _ => self.expected("an expression", next_token),
         }?;
 
+        // Composite function chain
+        while matches!(
+            expr,
+            Expr::Function(_)
+                | Expr::CompositeFunction { .. }
+                | Expr::Cast { .. }
+                | Expr::Convert { .. }

Review Comment:
   hmm some of the entries seem a bit arbitary (like cast, convert)? I wonder 
if instead of having this block as a special case, piggy-backing on my other 
comment to do similar to the lambda parsing, we could probably have the chain 
continuation logic in the same helper function that gets called once here (as 
in the function knows to parse not just a single function but also 
continuation/suffix if present of the full expression)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to