Add support for named argument functions #250

eyalleshem · 2020-07-29T16:09:34Z

Add support for named argument function

This commit support functions with argument names.
the format is :
"Select some_function( a => exp, b => exp2 .. ) FROM table1
OR
"select * from table(function(a => exp)) f;"

see:
https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#named-argument-assignment-token
or example is snwflake db :
https://docs.snowflake.com/en/sql-reference/functions/flatten.html
This commit does not belong to any branch on this repository.

eyalleshem · 2020-07-29T16:11:46Z

Sorry about some style changes that made by cargo fmt

coveralls · 2020-07-29T16:13:28Z

Pull Request Test Coverage Report for Build 191486507

48 of 50 (96.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.03%) to 91.88%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/tokenizer.rs	16	18	88.89%

Totals
Change from base Build 190279595:	0.03%
Covered Lines:	4481
Relevant Lines:	4877

💛 - Coveralls

nickolay · 2020-07-30T02:10:46Z

I find it weird that you added NamedArgument as a new variant of Expr. This makes the parser accept many weird constructs, which shouldn't be accepted. I expected this to be a new SelectItem-like enum. You can use peek_nth_token(1) == RArrow to determine if it's a named argument.

The argument name is a simple identifier, not an ObjectName.

Please check the PR for typos (FUCNTION, rigth, agument, // vs /// (which led to rustfmt reformatting the whole enum))

the format is : "Select * from ( a => exp, b=> exp2 .. )

Surely you meant SELECT myfunc(args) rather than FROM (args)

eyalleshem · 2020-07-30T09:09:03Z

Sorry for all the typos , I fixed them (Hope that all of them..)
I also changed the the object name to ident - and fixed the commit message with the correct query .

But i don't sure what you means when you say that the named-argument should be SelectItem
It's a comma-separated argument list that passed to function, In the struct Function the args is comma-separated Vec<Expr> , And because that my thought was that creating new Expr type will be the best suit .

nickolay · 2020-07-30T09:12:11Z

I said a new enum, a little like SelectItem (currently with two variants: unnamed argument, named argument).

eyalleshem · 2020-07-31T08:36:27Z

I done the changes according to comments .

nickolay · 2020-07-31T12:12:29Z

src/ast/mod.rs

+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+pub struct FunctionNamedArg {
+    pub name: Ident,
+    pub arg: Expr,
+}
+
+impl fmt::Display for FunctionNamedArg {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{} => {}", self.name, self.arg)?;
+        Ok(())
+    }
+}


Is there a reason to make this a separate struct, rather than FunctionArg::Named { name, arg }?

(Another option is to make the argument type a simple struct with an optional name, and the reason I suggested enum initially because of the other possible kinds of SQL arguments...)

Fixed ,
I agree that enum is better.

nickolay · 2020-07-31T12:18:38Z

src/ast/mod.rs

+            FunctionArg::Named(named_arg) => named_arg.fmt(f),
+            FunctionArg::Unnamed(unnamed_arg) => unnamed_arg.fmt(f),


We use write!(f, "{}", ...) in other places for this.

nickolay · 2020-07-31T12:19:37Z

src/parser.rs

+            // the `=>`
+            let _ = self.next_token();


Please use expect_token instead.

nickolay · 2020-07-31T12:20:26Z

src/tokenizer.rs

-                // identifier or keyword
+                // identifier or Keyword


Why this change?

sorry , by mistake ..

nickolay · 2020-07-31T12:22:03Z

src/ast/mod.rs

-/// The parser does not distinguish between expressions of different types
+/// The parser does not distinguish between qessions of different types


sorry , by mistake ..

This commit support functions with argument names. the format is : "Select some_function( a => exp, b => exp2 .. ) FROM table1 OR "select * from table(function(a => exp)) f;" see: https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#named-argument-assignment-token or example is snwflake db : https://docs.snowflake.com/en/sql-reference/functions/flatten.html

nickolay · 2020-08-02T05:05:25Z

Thanks!

This commit supports functions with argument names. the format is : "Select some_function( a => exp, b => exp2 .. ) FROM table1 OR "select * from table(function(a => exp)) f;" see: https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#named-argument-assignment-token or the motivating example from snowflake: https://docs.snowflake.com/en/sql-reference/functions/flatten.html

eyalleshem changed the title ~~Add support for named argument functiona~~ Add support for named argument functions Jul 29, 2020

eyalleshem mentioned this pull request Jul 29, 2020

Dialect-specific parsing and Snowflake JSON support #241

Open

eyalleshem force-pushed the table_function branch from e400c10 to cc116b9 Compare July 30, 2020 08:42

eyalleshem force-pushed the table_function branch 2 times, most recently from a6d8f94 to 37a4d49 Compare July 31, 2020 07:21

nickolay suggested changes Jul 31, 2020

View reviewed changes

eyalleshem force-pushed the table_function branch from 37a4d49 to b15a7bc Compare August 1, 2020 19:37

eyalleshem requested a review from nickolay August 1, 2020 19:48

nickolay merged commit 1cc3bf4 into apache:main Aug 2, 2020

nickolay removed their request for review August 2, 2020 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for named argument functions #250

Add support for named argument functions #250

eyalleshem commented Jul 29, 2020 •

edited

Loading

eyalleshem commented Jul 29, 2020

coveralls commented Jul 29, 2020 •

edited

Loading

nickolay commented Jul 30, 2020

eyalleshem commented Jul 30, 2020

nickolay commented Jul 30, 2020

eyalleshem commented Jul 31, 2020

nickolay Jul 31, 2020

eyalleshem Aug 1, 2020

nickolay Jul 31, 2020

eyalleshem Aug 1, 2020

nickolay Jul 31, 2020

eyalleshem Aug 1, 2020

nickolay Jul 31, 2020

eyalleshem Aug 1, 2020

nickolay Jul 31, 2020

eyalleshem Aug 1, 2020

nickolay commented Aug 2, 2020

		FunctionArg::Named(named_arg) => named_arg.fmt(f),
		FunctionArg::Unnamed(unnamed_arg) => unnamed_arg.fmt(f),

		/// The parser does not distinguish between expressions of different types
		/// The parser does not distinguish between qessions of different types

Add support for named argument functions #250

Add support for named argument functions #250

Conversation

eyalleshem commented Jul 29, 2020 • edited Loading

eyalleshem commented Jul 29, 2020

coveralls commented Jul 29, 2020 • edited Loading

Pull Request Test Coverage Report for Build 191486507

💛 - Coveralls

nickolay commented Jul 30, 2020

eyalleshem commented Jul 30, 2020

nickolay commented Jul 30, 2020

eyalleshem commented Jul 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickolay commented Aug 2, 2020

eyalleshem commented Jul 29, 2020 •

edited

Loading

coveralls commented Jul 29, 2020 •

edited

Loading