Support parametric arguments to `FUNCTION` for ClickHouse dialect #1315

git-hulk · 2024-06-17T10:54:08Z

This closes #1301

iffyio · 2024-06-21T04:45:23Z

src/ast/mod.rs

@@ -4706,6 +4706,9 @@ impl fmt::Display for CloseCursor {
 #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
 pub struct Function {
    pub name: ObjectName,
+    /// The parameters to the function, including any options specified within the
+    /// delimiting parentheses.


Can we include a link to the clickhouse docs?

@iffyio Done

iffyio · 2024-06-21T04:46:40Z

src/parser/mod.rs

+        let mut parameters = FunctionArguments::None;
+        // ClickHouse aggregation support parametric functions like `quantile(0.5)(x)`
+        // which (0.5) is a parameter to the function.
+        if dialect_of!(self is ClickHouseDialect) && self.consume_token(&Token::LParen) {


Suggested change

if dialect_of!(self is ClickHouseDialect) && self.consume_token(&Token::LParen) {

if dialect_of!(self is ClickHouseDialect | GenericDialect) && self.consume_token(&Token::LParen) {

we can include generic dialect if no conflicts

iffyio · 2024-06-21T04:47:02Z

src/parser/mod.rs

-        let args = self.parse_function_argument_list()?;
+        let mut args = self.parse_function_argument_list()?;
+        let mut parameters = FunctionArguments::None;
+        // ClickHouse aggregation support parametric functions like `quantile(0.5)(x)`


Suggested change

// ClickHouse aggregation support parametric functions like `quantile(0.5)(x)`

// ClickHouse aggregations support parametric functions like `quantile(0.5)(x)`

iffyio · 2024-06-21T04:51:32Z

tests/sqlparser_clickhouse.rs

+fn parse_select_parametric_function() {
+    clickhouse().verified_stmt("SELECT quantile(0.5)(x) FROM t");
+}


Could we extend the test asserting that the parameter and arguments lists do indeed show up in the parameters and args fields respectively (i.e not vice versa for example). Also we can include multiple items in both lists e.g quantile(0.5,0.6)(x,y)

git-hulk · 2024-06-21T06:51:08Z

@iffyio Thanks for your review, all comments are resolved now.

iffyio

LGTM! cc @alamb

alamb

Thanks @git-hulk and @iffyio

alamb · 2024-06-23T11:28:51Z

src/parser/mod.rs

+    }
+
+    /// Parse any extra set expressions that may be present in a query body
+    ///


I also pushed a commit to split up this function in e2ec2a6 to get the deeply nested tests to pass

This is the failure without this change: https://github.com/sqlparser-rs/sqlparser-rs/actions/runs/9632995493/job/26566863197

thread 'parse_deeply_nested_parens_hits_recursion_limits' has overflowed its stack fatal runtime error: stack overflow error: test failed, to rerun pass `--test sqlparser_common`

coveralls · 2024-06-23T11:32:31Z

Pull Request Test Coverage Report for Build 9633078247

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

67 of 68 (98.53%) changed or added relevant lines in 12 files are covered.
1510 unchanged lines in 10 files lost coverage.
Overall coverage increased (+0.1%) to 89.008%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	12	13	92.31%

Files with Coverage Reduction	New Missed Lines	%
src/lib.rs	1	12.64%
src/parser/alter.rs	2	66.67%
tests/sqlparser_sqlite.rs	4	98.36%
src/dialect/mod.rs	5	72.08%
tests/sqlparser_snowflake.rs	16	98.07%
src/ast/dml.rs	42	73.47%
tests/sqlparser_bigquery.rs	122	90.18%
src/ast/mod.rs	275	81.49%
src/parser/mod.rs	421	93.21%
tests/sqlparser_common.rs	622	89.6%

Totals
Change from base Build 9439708625:	0.1%
Covered Lines:	26276
Relevant Lines:	29521

💛 - Coveralls

alamb · 2024-06-23T11:36:13Z

Thanks again @git-hulk and @iffyio

coveralls · 2024-06-23T11:36:44Z

Pull Request Test Coverage Report for Build 9633103403

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

67 of 68 (98.53%) changed or added relevant lines in 12 files are covered.
1510 unchanged lines in 10 files lost coverage.
Overall coverage increased (+0.1%) to 89.008%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	12	13	92.31%

Files with Coverage Reduction	New Missed Lines	%
src/lib.rs	1	12.64%
src/parser/alter.rs	2	66.67%
tests/sqlparser_sqlite.rs	4	98.36%
src/dialect/mod.rs	5	72.08%
tests/sqlparser_snowflake.rs	16	98.07%
src/ast/dml.rs	42	73.47%
tests/sqlparser_bigquery.rs	122	90.18%
src/ast/mod.rs	275	81.49%
src/parser/mod.rs	421	93.21%
tests/sqlparser_common.rs	622	89.6%

Totals
Change from base Build 9439708625:	0.1%
Covered Lines:	26276
Relevant Lines:	29521

💛 - Coveralls

Add support of the function parametric for ClickHouse dialect

20528d0

iffyio reviewed Jun 21, 2024

View reviewed changes

Fix review comments

3d187b1

git-hulk requested a review from iffyio June 22, 2024 13:50

iffyio approved these changes Jun 23, 2024

View reviewed changes

alamb changed the title ~~Add support of the function parametric for ClickHouse dialect~~ Support of the parametric arguments to FUNCTIONS for ClickHouse dialect Jun 23, 2024

alamb changed the title ~~Support of the parametric arguments to FUNCTIONS for ClickHouse dialect~~ Support of the parametric arguments to FUNCTION for ClickHouse dialect Jun 23, 2024

alamb approved these changes Jun 23, 2024

View reviewed changes

fix compile errpr

b6a5d1d

alamb changed the title ~~Support of the parametric arguments to FUNCTION for ClickHouse dialect~~ Support parametric arguments to FUNCTION for ClickHouse dialect Jun 23, 2024

Reduce stack size to get tests to pass

e2ec2a6

alamb reviewed Jun 23, 2024

View reviewed changes

fix doctest

2da7ed2

alamb merged commit a685e11 into apache:main Jun 23, 2024
10 checks passed

Jesse-Bakker mentioned this pull request Jul 29, 2024

Parse ClickHouse table-valued function SETTINGS clause langdb/sqlparser-rs#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support parametric arguments to `FUNCTION` for ClickHouse dialect #1315

Support parametric arguments to `FUNCTION` for ClickHouse dialect #1315

Uh oh!

git-hulk commented Jun 17, 2024

Uh oh!

iffyio Jun 21, 2024

Uh oh!

git-hulk Jun 21, 2024

Uh oh!

iffyio Jun 21, 2024

Uh oh!

iffyio Jun 21, 2024

Uh oh!

iffyio Jun 21, 2024

Uh oh!

git-hulk commented Jun 21, 2024

Uh oh!

iffyio left a comment

Uh oh!

alamb left a comment

Uh oh!

alamb Jun 23, 2024 •

edited

Loading

Uh oh!

coveralls commented Jun 23, 2024 •

edited

Loading

Uh oh!

Uh oh!

alamb commented Jun 23, 2024

Uh oh!

coveralls commented Jun 23, 2024 •

edited

Loading

Uh oh!

Uh oh!

	if dialect_of!(self is ClickHouseDialect) && self.consume_token(&Token::LParen) {
	if dialect_of!(self is ClickHouseDialect \| GenericDialect) && self.consume_token(&Token::LParen) {

	// ClickHouse aggregation support parametric functions like `quantile(0.5)(x)`
	// ClickHouse aggregations support parametric functions like `quantile(0.5)(x)`

Support parametric arguments to FUNCTION for ClickHouse dialect #1315

Support parametric arguments to FUNCTION for ClickHouse dialect #1315

Uh oh!

Conversation

git-hulk commented Jun 17, 2024

Uh oh!

iffyio Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

git-hulk Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

iffyio Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

iffyio Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

iffyio Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

git-hulk commented Jun 21, 2024

Uh oh!

iffyio left a comment

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Jun 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 9633078247

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

Uh oh!

alamb commented Jun 23, 2024

Uh oh!

coveralls commented Jun 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 9633103403

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

Uh oh!

Support parametric arguments to `FUNCTION` for ClickHouse dialect #1315

Support parametric arguments to `FUNCTION` for ClickHouse dialect #1315

alamb Jun 23, 2024 •

edited

Loading

coveralls commented Jun 23, 2024 •

edited

Loading

coveralls commented Jun 23, 2024 •

edited

Loading