-
Notifications
You must be signed in to change notification settings - Fork 668
With Order Support for Memory Tables #1401
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.
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
Closed
mertak-synnada
wants to merge
25
commits into
apache:main
from
synnada-ai:feature/with-order-for-table
Closed
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
1375a21
add with_order option
a30ad56
format code
21cd618
Merge branch 'sqlparser-rs:main' into feature/with-order-for-table
320f7e1
change type into Vec<Vec<OrderExpr>>
d3d12e9
Merge remote-tracking branch 'origin/feature/with-order-for-table' in…
e85062f
skip with order type while creating options
acbcd2c
add with_order option
79e0e18
format code
3fb59e9
change type into Vec<Vec<OrderExpr>>
122f273
skip with order type while creating options
cc3c4c7
Merge remote-tracking branch 'origin/feature/with-order-for-table' in…
mertak-synnada 10a7b90
add debug statements
mertak-synnada c8a6f14
remove debug statements
mertak-synnada ed705c9
format code
mertak-synnada 8fb1958
remove redundant counter op
mertak-synnada 1c2eb75
remove Option from with_order attribute
mertak-synnada 15d84d7
Merge branch 'refs/heads/main' into feature/with-order-for-table
mertak-synnada 0f31847
add Datafusion link to with_order clause
7d222ff
format code
1575534
Merge branch 'refs/heads/main' into feature/with-order-for-table
e878149
fix merge conflict
17ba408
add supports_with_order function to dialect
565e637
move test to sqlparser_common.rs
147cd9e
add fmt to with order statement
5a97a46
remove redundant println!
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -129,6 +129,9 @@ pub struct CreateTable { | |
pub default_charset: Option<String>, | ||
pub collation: Option<String>, | ||
pub on_commit: Option<OnCommit>, | ||
/// Datafusion "WITH ORDER" clause | ||
/// <https://datafusion.apache.org/user-guide/sql/ddl.html#create-external-table/> | ||
pub with_order: Vec<Vec<OrderByExpr>>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we include test cases demonstrating the behavior introduced by the changes? |
||
/// ClickHouse "ON CLUSTER" clause: | ||
/// <https://clickhouse.com/docs/en/sql-reference/distributed-ddl/> | ||
pub on_cluster: Option<Ident>, | ||
|
@@ -405,6 +408,14 @@ impl Display for CreateTable { | |
write!(f, " WITH AGGREGATION POLICY {with_aggregation_policy}",)?; | ||
} | ||
|
||
if !self.with_order.is_empty() { | ||
write!(f, " WITH ORDER (")?; | ||
for order_by in &self.with_order { | ||
write!(f, "{}", display_comma_separated(order_by))?; | ||
} | ||
write!(f, ")")?; | ||
} | ||
|
||
if let Some(row_access_policy) = &self.with_row_access_policy { | ||
write!(f, " {row_access_policy}",)?; | ||
} | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we include here a link to the docs where this syntax comes from? (e.g. similar to the on_cluster below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was inspired by Datafusion's
WITH ORDER
statement, so I've added the link and tests, thank you!Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
We might have to change the
GenericDialect
or add some feature to theDialect
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 newThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see two possible ways forward:
ORDER BY
afterCREATE
and deprecateWITH ORDER
. This way we would have Clickhouse-like syntax for both memory and external tables, andWITH ORDER
syntax only for external tables. The latter asymmetry is a little weird but we can explain it away by deprecatingWITH ORDER
.ORDER BY
andWITH ORDER
for both memory and external tables in DF (they would be synonyms). We'd need to addWITH ORDER
for ordinaryCREATE
s 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?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 caseDatafusionDialect
).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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense - let's do it