Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for DuckDB functions named arguments with assignment operator #1195

Merged
merged 7 commits into from
Apr 6, 2024
Merged

Add support for DuckDB functions named arguments with assignment operator #1195

merged 7 commits into from
Apr 6, 2024

Conversation

gstvg
Copy link
Contributor

@gstvg gstvg commented Mar 26, 2024

Some nested DuckDB functions use assignment operator

struct_insert(struct, name := any, ...)
struct_pack(name := any, ...)
union_value(tag := any)

jayzhan211
jayzhan211 reviewed Mar 26, 2024
View reviewed changes
src/ast/mod.rs Outdated
@@ -4556,13 +4556,16 @@ pub enum FunctionArgOperator {
Equals,
/// function(arg1 => value1)
RightArrow,
/// function(arg1 := value1)
DuckAssignment,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest a general name like Assignment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed both Token and FunctionArg to Assignment , since snowflake uses it too

What do you think @universalmind303 ?

If is best to split the Token rename to another PR I can do it, just let me know, thanks!

@jmhain
Copy link
Contributor

jmhain commented Mar 27, 2024

Other dialects might use := to mean something different, for example on mysql, it appears to be an expression that can be used anywhere in a query to assign to a user-defined variable, e.g. in SELECT sqrt(@var1 := 1), @var1. So this should be put behind a dialect method similar to what I did here to fix a related regression on Snowflake: #1182

@gstvg
Copy link
Contributor Author

gstvg commented Mar 29, 2024

Other dialects might use := to mean something different, for example on mysql, it appears to be an expression that can be used anywhere in a query to assign to a user-defined variable, e.g. in SELECT sqrt(@var1 := 1), @var1. So this should be put behind a dialect method similar to what I did here to fix a related regression on Snowflake: #1182

Good catch @jmhain, thanks!
Is possible to use dialect_of!(DuckDBDialect) instead of adding a new Dialect method?

@jmhain
Copy link
Contributor

jmhain commented Mar 29, 2024

@gstvg Yeah that sounds fine to me!

@coveralls
Copy link

coveralls commented Mar 29, 2024 •
edited

Pull Request Test Coverage Report for Build 8582499731

Details

  • 33 of 36 (91.67%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 87.936%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser/mod.rs 8 11 72.73%
Totals Coverage Status
Change from base Build 8481586362: 0.02%
Covered Lines: 20592
Relevant Lines: 23417
💛 - Coveralls

jayzhan211
jayzhan211 reviewed Mar 30, 2024
View reviewed changes
src/parser/mod.rs Outdated
let name = self.parse_identifier(false)?;

self.expect_token(&Token::Assignment)?;
let arg = self.parse_wildcard_expr()?.into();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is wildcard included?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not, thanks. To be honest, I just copied from the code few lines above which does the same for => and =. Changing them to parse_expr doesn't break any test. Don't know if they need it too.
Anyway, i changed mine to parse_expr, thanks again 🙏

alamb
alamb reviewed Apr 6, 2024
View reviewed changes
src/parser/mod.rs Outdated
@@ -8602,6 +8602,18 @@ impl<'a> Parser<'a> {
arg,
operator: FunctionArgOperator::Equals,
})
} else if dialect_of!(self is DuckDbDialect) && self.peek_nth_token(1) == Token::Assignment
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we also support Generic dialect as well for syntaxes. I took the liberty of adding this in another commit

alamb
alamb approved these changes Apr 6, 2024
View reviewed changes
Copy link
Collaborator

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution @gstvg -- sorry I am behind and it took so long to review this

@alamb alamb merged commit 14b33ac into sqlparser-rs:main Apr 6, 2024
10 checks passed
JichaoS pushed a commit to luabase/sqlparser-rs that referenced this pull request May 7, 2024
@gstvg @alamb @JichaoS
Add support for DuckDB functions named arguments with assignment oper…
b25e8c3 
…ator (sqlparser-rs#1195)

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jayzhan211 jayzhan211 jayzhan211 left review comments

@alamb alamb alamb approved these changes

@universalmind303 universalmind303 universalmind303 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@gstvg @jmhain @coveralls @alamb @jayzhan211 @universalmind303