With Order Support for Memory Tables #1401

mertak-synnada · 2024-08-27T08:23:57Z

Add support for WITH ORDER syntax for Memory Tables

…to feature/with-order-for-table

parse with order in correct ordering

iffyio · 2024-08-27T14:25:57Z

src/ast/dml.rs

@@ -124,6 +124,7 @@ pub struct CreateTable {
    pub default_charset: Option<String>,
    pub collation: Option<String>,
    pub on_commit: Option<OnCommit>,
+    pub with_order: Vec<Vec<OrderByExpr>>,


Could we include here a link to the docs where this syntax comes from? (e.g. similar to the on_cluster below)

This was inspired by Datafusion's WITH ORDER statement, so I've added the link and tests, thank you!

FWIW I did some research and could not find any existing databases that use the WITH ORDER syntax.

ClickHouse does seem to have a way to specify order as part of a CREATE TABLE statement: https://clickhouse.com/docs/en/engines/table-engines/mergetree-family/mergetree

@mertak-synnada rather than introducing special DataFusion only syntax support, what do you think about extending DataFusion to use the existing ClickHouse syntax?

CREATE TABLE .... ORDER BY <...>

We might have to change the GenericDialect or add some feature to the Dialect trait to permit other dialects to parse such syntax, but it would be nice to align to some other existing syntax rather than creating something new

I see two possible ways forward:

We can change DF to support ORDER BY after CREATE and deprecate WITH ORDER. This way we would have Clickhouse-like syntax for both memory and external tables, and WITH ORDER syntax only for external tables. The latter asymmetry is a little weird but we can explain it away by deprecating WITH ORDER.

We can support both ORDER BY and WITH ORDER for both memory and external tables in DF (they would be synonyms). We'd need to add WITH ORDER for ordinary CREATEs here though.

I am OK with both, with a slight preference to 2 because DF has had WITH ORDER for a while now. What do you think?

I think in this case we should distinguish between DataFusion's needs and sqlparser-rs's needs

If we are going to introduce WITH ORDER to sqlparser-rs I think it should follow the existing pattern of being connected to a specific dialect (in this case DatafusionDialect).

Thus I would personally vote for option 1 (change DF) so we avoid creating a new SQL dialect (both in this crate and in general) as much as possible

We can change DF to support ORDER BY after CREATE and deprecate WITH ORDER. This way we would have Clickhouse-like syntax for both memory and external tables, and WITH ORDER syntax only for external tables. The latter asymmetry is a little weird but we can explain it away by deprecating WITH ORDER.

Makes sense - let's do it

iffyio · 2024-08-27T14:28:38Z

src/ast/dml.rs

@@ -124,6 +124,7 @@ pub struct CreateTable {
    pub default_charset: Option<String>,
    pub collation: Option<String>,
    pub on_commit: Option<OnCommit>,
+    pub with_order: Vec<Vec<OrderByExpr>>,


Can we include test cases demonstrating the behavior introduced by the changes?

add unit test for WITH ORDER parsing in create table

# Conflicts: # src/ast/helpers/stmt_create_table.rs

iffyio · 2024-09-02T14:35:23Z

src/parser/mod.rs

+        if self.parse_keyword(Keyword::WITH) {
+            if self.parse_keyword(Keyword::ORDER) {


Suggested change

if self.parse_keyword(Keyword::WITH) {

if self.parse_keyword(Keyword::ORDER) {

if self.parse_keywords(&[Keyword::WITH, Keyword::ORDER]) {

Also I think we can keep the postgres path separate since this feature doesn't apply to that dialect? - so that this part remains the same

let with_options = self.parse_options(Keyword::WITH)?;

We can make the fuctionality conditional with e.g. if self.dialect.supports_with_order() or similar method

iffyio · 2024-09-02T14:42:07Z

src/parser/mod.rs

+                loop {
+                    order_exprs.push(vec![self.parse_order_by_expr()?]);
+                    if !self.consume_token(&Token::Comma) {
+                        self.expect_token(&Token::RParen)?;
+                        break;
+                    }
+                }


Can the parse_comma_separated() method be reused for this functionality?

iffyio · 2024-09-02T14:48:23Z

src/parser/mod.rs

@@ -12364,6 +12385,43 @@ mod tests {
        );
    }

+    #[test]


hmm I think this would probably not be the ideal place for the test to live - if the feature isn't bound to any dialect, or if it uses a dialect method to guard the feature, we can add the tests to common e.g. https://github.com/sqlparser-rs/sqlparser-rs/blob/main/tests/sqlparser_common.rs#L4405

remove parse_options_in_parentheses functionality

change tests to all_dialects_where clause

coveralls · 2024-09-03T13:49:24Z

Pull Request Test Coverage Report for Build 10678509735

Details

50 of 55 (90.91%) changed or added relevant lines in 8 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.001%) to 89.25%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/ast/helpers/stmt_create_table.rs	6	7	85.71%
src/dialect/generic.rs	1	2	50.0%
src/dialect/mod.rs	1	2	50.0%
tests/sqlparser_common.rs	24	26	92.31%

Files with Coverage Reduction	New Missed Lines	%
tests/sqlparser_postgres.rs	1	88.56%
tests/sqlparser_bigquery.rs	1	90.27%

Totals
Change from base Build 10653671802:	-0.001%
Covered Lines:	28785
Relevant Lines:	32252

💛 - Coveralls

alamb · 2024-09-10T19:47:41Z

Marking as draft as i think this is still under discussion and not in need of more review. Please mark it as review if I got that wrong or when it is ready

mertak-synnada and others added 17 commits August 6, 2024 14:33

add with_order option

1375a21

format code

a30ad56

Merge branch 'sqlparser-rs:main' into feature/with-order-for-table

21cd618

change type into Vec<Vec<OrderExpr>>

320f7e1

Merge remote-tracking branch 'origin/feature/with-order-for-table' in…

d3d12e9

…to feature/with-order-for-table

skip with order type while creating options

e85062f

add with_order option

acbcd2c

format code

79e0e18

change type into Vec<Vec<OrderExpr>>

3fb59e9

skip with order type while creating options

122f273

Merge remote-tracking branch 'origin/feature/with-order-for-table' in…

cc3c4c7

…to feature/with-order-for-table

add debug statements

10a7b90

remove debug statements

c8a6f14

parse with order in correct ordering

format code

ed705c9

remove redundant counter op

8fb1958

remove Option from with_order attribute

1c2eb75

Merge branch 'refs/heads/main' into feature/with-order-for-table

15d84d7

iffyio reviewed Aug 27, 2024

View reviewed changes

mertak-synnada added 2 commits August 29, 2024 09:17

add Datafusion link to with_order clause

0f31847

add unit test for WITH ORDER parsing in create table

format code

7d222ff

mertak-synnada requested a review from iffyio September 2, 2024 06:00

mertak-synnada added 2 commits September 2, 2024 09:01

Merge branch 'refs/heads/main' into feature/with-order-for-table

1575534

# Conflicts: # src/ast/helpers/stmt_create_table.rs

fix merge conflict

e878149

iffyio reviewed Sep 2, 2024

View reviewed changes

mertak-synnada added 4 commits September 3, 2024 09:57

add supports_with_order function to dialect

17ba408

remove parse_options_in_parentheses functionality

move test to sqlparser_common.rs

565e637

add fmt to with order statement

147cd9e

change tests to all_dialects_where clause

remove redundant println!

5a97a46

mertak-synnada requested a review from iffyio September 3, 2024 07:32

alamb mentioned this pull request Sep 9, 2024

DataFusion weekly project plan (Andrew Lamb) - Sep 9, 2024 apache/datafusion#12391

Closed

5 tasks

alamb marked this pull request as draft September 10, 2024 19:47

alamb mentioned this pull request Sep 11, 2024

DataFusion weekly project plan (Andrew Lamb) - Sep 2, 2024 apache/datafusion#12336

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

With Order Support for Memory Tables #1401

With Order Support for Memory Tables #1401

mertak-synnada commented Aug 27, 2024

iffyio Aug 27, 2024

mertak-synnada Aug 29, 2024

alamb Sep 3, 2024 •

edited

Loading

ozankabak Sep 4, 2024

alamb Sep 10, 2024 •

edited

Loading

ozankabak Sep 11, 2024

iffyio Aug 27, 2024

iffyio Sep 2, 2024

iffyio Sep 2, 2024

iffyio Sep 2, 2024

coveralls commented Sep 3, 2024

alamb commented Sep 10, 2024

		if self.parse_keyword(Keyword::WITH) {
		if self.parse_keyword(Keyword::ORDER) {

	if self.parse_keyword(Keyword::WITH) {
	if self.parse_keyword(Keyword::ORDER) {
	if self.parse_keywords(&[Keyword::WITH, Keyword::ORDER]) {

@@ @@ -12364,6 +12385,43 @@ mod tests { @@
                       );
                   }
+                  #[test]

With Order Support for Memory Tables #1401

Are you sure you want to change the base?

With Order Support for Memory Tables #1401

Conversation

mertak-synnada commented Aug 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb Sep 10, 2024 • edited Loading

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

coveralls commented Sep 3, 2024

Pull Request Test Coverage Report for Build 10678509735

Details

💛 - Coveralls

alamb commented Sep 10, 2024

alamb Sep 3, 2024 •

edited

Loading

alamb Sep 10, 2024 •

edited

Loading