support clickhouse parameterized views #1181

Lordworms · 2024-03-15T03:24:12Z

coveralls · 2024-03-16T15:20:36Z

Pull Request Test Coverage Report for Build 8669253871

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

29 of 33 (87.88%) changed or added relevant lines in 4 files are covered.
21 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.02%) to 88.089%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	16	20	80.0%

Files with Coverage Reduction	New Missed Lines	%
src/tokenizer.rs	21	90.37%

Totals
Change from base Build 8622482464:	0.02%
Covered Lines:	21314
Relevant Lines:	24196

💛 - Coveralls

src/parser/mod.rs

iffyio · 2024-03-17T05:32:01Z

src/ast/mod.rs

@@ -733,6 +733,10 @@ pub enum Expr {
    ///
    /// See <https://docs.snowflake.com/en/sql-reference/constructs/where#joins-in-the-where-clause>.
    OuterJoin(Box<Expr>),
+    ColumnWithDataType {


Could we include a description for this new variant (ideally including a link to the docs)?

iffyio · 2024-03-17T05:39:11Z

src/ast/mod.rs

+    ColumnWithDataType {
+        columns: Box<Expr>,
+        data_type: DataType,
+    },


I'm wondering if naming wise, 'ColumnWithDataType' sounds a bit too generic since it applies to a lot of unrelated scenarios. From the docs, it sounds incorrect as well to have the columns: Box<Expr>?

The actual column in WHERE a = {b:INT} is a, while b would be more of a function parameter

b given its a function parameter likely needs to be an Ident instead of an Expr

thinking roughly something along this line could be more descriptive

ParameterizedViewArg{ name: Ident, data_type: DataType, }

Sure, I'll change it

iffyio · 2024-03-17T05:50:05Z

src/parser/mod.rs

+        if dialect_of!(self is ClickHouseDialect) && self.peek_token().token == Token::LBrace {
+            return self.parse_column_with_data_type();
+        }


Currently the parse logic for this feature is spread across this and the parse_infix function where one triggers the other indirectly, slightly harder to follow as a result - one suggestion to simplify this would be:

The current function already includes a match onthe token type, so we can introduce a clause
Token::LBRACE if dialect_of!(self is Clickhouse) => { self.prev_token(); self.parse_column_with_data_type() }

The current parse_column_with_data_type can parse the components directly - especially given that it does not seem correct for it to parse an inner expression. It becomes
// consume LBRACE let name = self.parse_identifier()?; self.expect(Token::Colon)?; let data_type = self.parse_data_type()?; // consume RBRACE

makes sense to me.

alamb

Thank you @Lordworms -- this is a really nice contribution . I think it just needs a little polish and would be ready to go

fyi @flipchart

src/ast/mod.rs

src/parser/mod.rs

alamb · 2024-04-07T12:24:44Z

src/parser/mod.rs

-
+        // ClickHouse support using {column:Datatype} format in its creation of parametrised views
+        match self.peek_token().token {
+            Token::LBrace if dialect_of!(self is ClickHouseDialect) => {


Can you please also support this syntax in GenericDialect as well as explained in https://github.com/sqlparser-rs/sqlparser-rs?tab=readme-ov-file#new-syntax ?

(also add test for it below)

alamb · 2024-04-07T12:26:15Z

tests/sqlparser_clickhouse.rs

+
+#[test]
+fn parse_create_parametrised_views() {
+    clickhouse().verified_stmt("CREATE VIEW view AS SELECT * FROM A WHERE Column1 = {column1:datatype1} AND Column2 = {column2:datatype2}");


Can you also please test:

In generic dialect

A single column definition

Error case (like where Column1 = {column1})

Error case (like where) (no column definition)

Sure, I'll add these tommorrow

alamb

Thank you @Lordworms

alamb · 2024-04-09T19:44:21Z

I took the liberty of fixing the compile error and pushing a commit to fix it

CI is failing

alamb · 2024-04-09T19:56:56Z

Hi @Lordworms -- it seems like there is a real test failure on this PR. Can you investigate?

alamb · 2024-04-09T20:08:05Z

Marking as draft as the PR checks are failing

Lordworms · 2024-04-09T22:21:01Z

Hi @Lordworms -- it seems like there is a real test failure on this PR. Can you investigate?

It seems to have some conflict with the DuckDB literal. I'll fix this.

tiny change

Lordworms · 2024-04-09T23:47:51Z

@alamb I have resolved it and would you please take a look at it when you are available? Thanks! Sorry for the failure since this PR has lasted for 3 weeks and I forgot to rebase this branch.

alamb

Thanks @Lordworms . I have a few more comments -- sorry it takes me so long to review these

src/parser/mod.rs

alamb · 2024-04-12T10:33:24Z

tests/sqlparser_common.rs

+    }
+}
+#[test]
+fn parse_create_parametrised_views() {


I may be mistaken, but thinking about this PR some more and I am not sure about the change to parse_prefix

Specifically, by changing that I think it now allows {column:data_type} to appear in any expression (not just a parameterized view).

For example, wouldn't these queries now parse SELECT * FROM foo WHERE x = {column:int} or SELECT x={column:int} FROM foo?

Can you add test coverage for those cases?

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb · 2024-05-03T17:59:51Z

I am not sure what the current status of this PR is -- it has a bunch of conflicts and I am not quite sure if it is ready for another review. Sorry for the delay. Marking it as a draft until we sort that out so it isn't on my review queue

Lordworms force-pushed the issue_1167 branch from 2515153 to e9e0719 Compare March 15, 2024 03:30

iffyio reviewed Mar 17, 2024

View reviewed changes

Lordworms force-pushed the issue_1167 branch from 5a1419c to ef57730 Compare March 18, 2024 01:04

Lordworms requested a review from iffyio March 18, 2024 02:46

alamb changed the title ~~support clickhouse parametrised views~~ support clickhouse parameterized views Apr 7, 2024

alamb reviewed Apr 7, 2024

View reviewed changes

Lordworms requested a review from alamb April 8, 2024 15:03

alamb mentioned this pull request Apr 8, 2024

DataFusion weekly project plan (Andrew Lamb) - April 8, 2024 apache/datafusion#10002

Closed

9 tasks

alamb previously approved these changes Apr 9, 2024

View reviewed changes

alamb marked this pull request as draft April 9, 2024 20:07

Lordworms force-pushed the issue_1167 branch from 03fb418 to cba9213 Compare April 9, 2024 23:41

Adding support for ClickHouse parameterized view

2bce6f5

tiny change

Lordworms force-pushed the issue_1167 branch from cba9213 to 2bce6f5 Compare April 9, 2024 23:45

Lordworms marked this pull request as ready for review April 9, 2024 23:45

flipchart mentioned this pull request Apr 10, 2024

ClickHouse parameterized view TylerBrinks/SqlParser-cs#10

Closed

alamb reviewed Apr 12, 2024

View reviewed changes

Update src/parser/mod.rs

b1d387a

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb marked this pull request as draft May 3, 2024 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support clickhouse parameterized views #1181

support clickhouse parameterized views #1181

Lordworms commented Mar 15, 2024 •

edited by alamb

coveralls commented Mar 16, 2024 •

edited

iffyio Mar 17, 2024

Lordworms Mar 17, 2024

iffyio Mar 17, 2024

Lordworms Mar 17, 2024

iffyio Mar 17, 2024

Lordworms Mar 17, 2024 •

edited

alamb left a comment

alamb Apr 7, 2024

Lordworms Apr 8, 2024

alamb Apr 7, 2024

Lordworms Apr 8, 2024

alamb left a comment

alamb commented Apr 9, 2024

alamb commented Apr 9, 2024

alamb commented Apr 9, 2024

Lordworms commented Apr 9, 2024

Lordworms commented Apr 9, 2024 •

edited

alamb left a comment

alamb Apr 12, 2024

Lordworms Apr 12, 2024

alamb commented May 3, 2024

support clickhouse parameterized views #1181

Are you sure you want to change the base?

support clickhouse parameterized views #1181

Conversation

Lordworms commented Mar 15, 2024 • edited by alamb

coveralls commented Mar 16, 2024 • edited

Pull Request Test Coverage Report for Build 8669253871

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

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

Lordworms Mar 17, 2024 • edited

Choose a reason for hiding this comment

alamb left a 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

alamb left a comment

Choose a reason for hiding this comment

alamb commented Apr 9, 2024

alamb commented Apr 9, 2024

alamb commented Apr 9, 2024

Lordworms commented Apr 9, 2024

Lordworms commented Apr 9, 2024 • edited

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented May 3, 2024

Lordworms commented Mar 15, 2024 •

edited by alamb

coveralls commented Mar 16, 2024 •

edited

Lordworms Mar 17, 2024 •

edited

Lordworms commented Apr 9, 2024 •

edited