iffyio commented on code in PR #1507:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1507#discussion_r1835643017
##########
src/parser/mod.rs:
##########
@@ -11141,19 +11154,34 @@ impl<'a> Parser<'a> {
/// FIRST_VALUE(x IGNORE NULL);
/// ```
fn parse_function_argument_list(&mut self) -> Result<FunctionArgumentList,
ParserError> {
+ let mut clauses = vec![];
+
+ if dialect_of!(self is MsSqlDialect) {
+ // JSON_ARRAY(NULL ON NULL)
+ if self.parse_keywords(&[Keyword::NULL, Keyword::ON,
Keyword::NULL]) {
+ clauses.push(FunctionArgumentClause::JsonNullClause(
+ JsonNullClause::NullOnNull,
+ ));
+ }
+ // JSON_ARRAY(ABSENT ON NULL)
Review Comment:
```suggestion
if self.parse_keywords(&[Keyword::NULL, Keyword::ON,
Keyword::NULL]) {
clauses.push(FunctionArgumentClause::JsonNullClause(
JsonNullClause::NullOnNull,
));
}
```
nit: since its clear from the parsing and the FunctionArgumentClause
variant, I'm rather thinking we can move the `NULL ON NULL` and `ABSENT ON
NULL` documentation to the respective `JsonNullClause` enum variants? would be
move visibile there since most folks will see the syntax from there vs from
within the parser
##########
src/parser/mod.rs:
##########
@@ -11096,6 +11096,19 @@ impl<'a> Parser<'a> {
arg,
operator: FunctionArgOperator::Assignment,
})
+ } else if dialect_of!(self is MsSqlDialect) {
+ // FUNC(<expr> : <expr>)
+ let name = self.parse_wildcard_expr()?;
+ if self.consume_token(&Token::Colon) {
+ let arg = self.parse_expr()?.into();
+ Ok(FunctionArg::Named {
+ name,
+ arg,
+ operator: FunctionArgOperator::Colon,
+ })
Review Comment:
hmm yeah regarding the requirement I wondered a bit of pros/cons but I
figure it might be the lesser evil to add a new variant like
`FunctionArg::Association { target: Expr, arg, operator }` variant that is able
to represent an arbitrary expression?
The immediate advantage would be that its not a breaking change but we would
also avoid hiding the identifier which is what users expect from a named
function argument since that is a feature used in a lot of scenarios. So I
figure we could be more explicit to represent this as a different class of
functions, and then highlight in the doc the difference between the two
##########
tests/sqlparser_mssql.rs:
##########
@@ -448,6 +448,447 @@ fn parse_for_json_expect_ast() {
);
}
+#[test]
+fn parse_mssql_json_object() {
Review Comment:
noticed we used the parse_wildcard_expr, if that syntax is valid, can we
include a test case with a wildcard argument?
##########
src/parser/mod.rs:
##########
@@ -11141,19 +11154,34 @@ impl<'a> Parser<'a> {
/// FIRST_VALUE(x IGNORE NULL);
/// ```
fn parse_function_argument_list(&mut self) -> Result<FunctionArgumentList,
ParserError> {
+ let mut clauses = vec![];
+
+ if dialect_of!(self is MsSqlDialect) {
Review Comment:
similarly, we can use the `self.dialect.supports...()` method to gate the
behavior
##########
src/parser/mod.rs:
##########
@@ -11096,6 +11096,19 @@ impl<'a> Parser<'a> {
arg,
operator: FunctionArgOperator::Assignment,
})
+ } else if dialect_of!(self is MsSqlDialect) {
+ // FUNC(<expr> : <expr>)
+ let name = self.parse_wildcard_expr()?;
+ if self.consume_token(&Token::Colon) {
+ let arg = self.parse_expr()?.into();
+ Ok(FunctionArg::Named {
+ name,
+ arg,
+ operator: FunctionArgOperator::Colon,
+ })
+ } else {
+ Ok(FunctionArg::Unnamed(name.into()))
+ }
Review Comment:
```suggestion
} else if self.dialect.supports_json_object_functions() &&
self.peek_nth_token(1) == Token::Colon {
let name = self.parse_wildcard_expr(false)?;
let _ = self.expect_token(&Token::Colon)?;
let arg = self.parse_expr()?.into();
Ok(FunctionArg::Named {
name,
arg,
operator: FunctionArgOperator::Colon,
})
}
```
Something like this so that we only enter the branch when we know that there
is a valid expr to parse? We can use a method on the dialect for this feature
instead of the `dialect_of!` macro (the latter is essentially deprecated)
##########
src/ast/mod.rs:
##########
@@ -7317,6 +7329,24 @@ impl Display for UtilityOption {
}
}
+/// MSSQL's json null clause
Review Comment:
We can include a link here to one of the docs like
[here](https://learn.microsoft.com/en-us/sql/t-sql/functions/json-object-transact-sql?view=sql-server-ver16#json_null_clause)?
##########
src/parser/mod.rs:
##########
@@ -11141,19 +11154,34 @@ impl<'a> Parser<'a> {
/// FIRST_VALUE(x IGNORE NULL);
/// ```
fn parse_function_argument_list(&mut self) -> Result<FunctionArgumentList,
ParserError> {
+ let mut clauses = vec![];
+
+ if dialect_of!(self is MsSqlDialect) {
+ // JSON_ARRAY(NULL ON NULL)
+ if self.parse_keywords(&[Keyword::NULL, Keyword::ON,
Keyword::NULL]) {
+ clauses.push(FunctionArgumentClause::JsonNullClause(
+ JsonNullClause::NullOnNull,
+ ));
+ }
+ // JSON_ARRAY(ABSENT ON NULL)
Review Comment:
Ah noticed we have this code few lines below as well, so that the above
comment applies to that one as well. Actually for this line, I'm not sure we
need the changes here at all, since if we've hit `RParen` we don't have any
clauses to parse (so that the one below I imagine should be sufficient)?
--
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]
