-
Notifications
You must be signed in to change notification settings - Fork 669
fix: parse error on unnamed arg with default syntax #2091
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
base: main
Are you sure you want to change the base?
Conversation
|
Please let me know if there is a better place for a more general fix. It seems like adding |
src/parser/mod.rs
Outdated
| return self.expected("a name or type", token.clone()); | ||
| } | ||
| // We ensure that the token is a `Word` token, and not other special tokens. | ||
| if !matches!(token.token, Token::Word(_)) { |
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.
| if !matches!(token.token, Token::Word(_)) { | |
| if self.peek_keyword(DEFAULT) || !matches!(token.token, Token::Word(_)) { |
thinking we can simplify the introduced if statement by doing the check here instead?
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.
Won't we return a Result<_, ParserError> from parse_function_arg in this case? I was thinking we do still need to continue on to parse the default_expr. I'll take a closer look.
I agree that the nesting is not ideal in any case, maybe another option would be to push this check into maybe_parse above?
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.
Oh yeah indeed, I misread that block as happening inside of the maybe_parse. Yes the intent would be to ideally do the check in the maybe_parse I think
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 took a stab at this in 3fc3094
4065072 to
624c0f7
Compare
Using the
DEFAULTvariant of theCREATE FUNCTIONdefault argument syntax causes a parse error with unnamed arguments. The proposed fix skips the logic to disambiguateargname argtypefromargtypewhen theDEFAULTkeyword is detected. IIUC, this is safe becauseargnameandargtypecan only take on the nameDEFAULTwhen quoted.e.g, in postgres this is allowed:
But "DEFAULT" (the argname) and "DEFAULT" (the argtype) cannot appear unquoted
Ref: apache/datafusion#18450 (comment)