iffyio commented on code in PR #1500:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1500#discussion_r1834691705
##########
src/parser/mod.rs:
##########
@@ -3532,9 +3580,9 @@ impl<'a> Parser<'a> {
}
/// Run a parser method `f`, reverting back to the current position if
unsuccessful.
- pub fn maybe_parse<T, F>(&mut self, mut f: F) -> Result<Option<T>,
ParserError>
+ pub fn maybe_parse<T, F>(&mut self, f: F) -> Result<Option<T>, ParserError>
where
- F: FnMut(&mut Parser) -> Result<T, ParserError>,
+ F: FnOnce(&mut Parser) -> Result<T, ParserError>,
Review Comment:
Oh was it something that required this change from fnmut to fnonce?
##########
src/ast/mod.rs:
##########
@@ -808,6 +808,21 @@ pub enum Expr {
},
/// Scalar function call e.g. `LEFT(foo, 5)`
Function(Function),
+ /// Arbitrary expr method call
+ ///
+ /// Syntax:
+ ///
+ /// `<arbitrary-expr>.<arbitrary-expr>.<arbitrary-expr>...`
+ ///
+ /// > `arbitrary-expr` can be any expression including a function call.
Review Comment:
Ah if i understand correctly I think this comment might not be accurate. My
understanding is that first expression is indeed an arbitrary expression but
subsequent expressions must be function calls?
##########
src/ast/mod.rs:
##########
@@ -5593,6 +5609,21 @@ impl fmt::Display for FunctionArgumentClause {
}
}
+/// A method call
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub struct Method {
+ pub expr: Box<Expr>,
+ pub method: Function,
+}
Review Comment:
representation wise I'm thinking we could use something like
```rust
struct Method {
pub prefix: Box<Expr>,
pub suffix: Vec<Function>
}
```
Actually I'm not super sure naming wise of the properties, maybe we can come
up with something better. But the main point/change I'm thinking would be the
overall representation. Instead of using a nested datastructure for this, that
doesn't seem required, the syntax is just a chain of expression so we can just
use list to store that chain - and it maps nicely to how the parsing is done
since its clear that the first part can be any expr, while subsequent
components must be functions. wdyt?
##########
src/ast/mod.rs:
##########
@@ -808,6 +808,21 @@ pub enum Expr {
},
/// Scalar function call e.g. `LEFT(foo, 5)`
Function(Function),
+ /// Arbitrary expr method call
+ ///
+ /// Syntax:
+ ///
+ /// `<arbitrary-expr>.<arbitrary-expr>.<arbitrary-expr>...`
+ ///
+ /// > `arbitrary-expr` can be any expression including a function call.
+ ///
+ /// Example:
+ ///
+ /// ```sql
+ /// SELECT (SELECT ',' + name FROM sys.objects FOR XML PATH(''),
TYPE).value('.','NVARCHAR(MAX)')
+ /// SELECT
CONVERT(XML,'<Book>abc</Book>').value('.','NVARCHAR(MAX)').value('.','NVARCHAR(MAX)')
+ /// ```
+ Method(Method),
Review Comment:
Should we maybe call this MethodInvocation vs Method? thinking the latter
might suggest similar to Function that we're looking to represent a method body
##########
src/parser/mod.rs:
##########
@@ -1258,23 +1258,27 @@ impl<'a> Parser<'a> {
}
};
self.expect_token(&Token::RParen)?;
- if !self.consume_token(&Token::Period) {
+ if let Some(expr) = self.try_parse_method(&expr)? {
Ok(expr)
} else {
- let tok = self.next_token();
- let key = match tok.token {
- Token::Word(word) => word.to_ident(),
- _ => {
- return parser_err!(
- format!("Expected identifier, found: {tok}"),
- tok.location
- )
- }
- };
- Ok(Expr::CompositeAccess {
- expr: Box::new(expr),
- key,
- })
+ if !self.consume_token(&Token::Period) {
+ Ok(expr)
+ } else {
Review Comment:
I think the else block can be unnested one level to be?
```rust
else if !self.consume_token(...) {
Ok(expr)
} else {
let tok = ...
}
```
##########
src/ast/mod.rs:
##########
@@ -808,6 +808,21 @@ pub enum Expr {
},
/// Scalar function call e.g. `LEFT(foo, 5)`
Function(Function),
+ /// Arbitrary expr method call
+ ///
+ /// Syntax:
+ ///
+ /// `<arbitrary-expr>.<arbitrary-expr>.<arbitrary-expr>...`
+ ///
+ /// > `arbitrary-expr` can be any expression including a function call.
+ ///
+ /// Example:
+ ///
+ /// ```sql
+ /// SELECT (SELECT ',' + name FROM sys.objects FOR XML PATH(''),
TYPE).value('.','NVARCHAR(MAX)')
+ /// SELECT
CONVERT(XML,'<Book>abc</Book>').value('.','NVARCHAR(MAX)').value('.','NVARCHAR(MAX)')
Review Comment:
I saw this doc that seems to describe the feature and could be useful to
include here (for a lack of syntax spec from mssql)?
https://learn.microsoft.com/en-us/sql/relational-databases/server-management-objects-smo/create-program/calling-methods?view=sql-server-ver16
##########
src/parser/mod.rs:
##########
@@ -1345,6 +1354,45 @@ impl<'a> Parser<'a> {
})
}
+ fn try_parse_method(&mut self, expr: &Expr) -> Result<Option<Expr>,
ParserError> {
Review Comment:
instead of passing a reference we can use owned values, such that here we
return the same expression if we didn't parse anything. That would avoid
cloning the expression. I think with that and the linear representation of the
method expression, the parsing logic could be slightly simpler (since it won't
need to mutate anything, given that the maybe_parse section would only deal
with the suffix)
--
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]
