Skip to content
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

Add support for snowflake exclusive create table options #1233

Merged
merged 14 commits into from
Jun 9, 2024

Conversation

balliegojr
Copy link
Contributor

Add support for snowflake exclusive create table options

  [ CLUSTER BY ( <expr> [ , <expr> , ... ] ) ]
  [ ENABLE_SCHEMA_EVOLUTION = { TRUE | FALSE } ]
  [ DATA_RETENTION_TIME_IN_DAYS = <integer> ]
  [ MAX_DATA_EXTENSION_TIME_IN_DAYS = <integer> ]
  [ CHANGE_TRACKING = { TRUE | FALSE } ]
  [ DEFAULT_DDL_COLLATION = '<collation_specification>' ]
  [ COPY GRANTS ]
  [ COMMENT = '<string_literal>' ]
  [ [ WITH ] ROW ACCESS POLICY <policy_name> ON ( <col_name> [ , <col_name> ... ] ) ]
  [ [ WITH ] AGGREGATION POLICY <policy_name> ]
  [ [ WITH ] TAG ( <tag_name> = '<tag_value>' [ , <tag_name> = '<tag_value>' , ... ] ) ]

Since snowflake does not require a strict order in the statement, the create table logic was moved to inside the snowflake dialect

src/ast/mod.rs Outdated Show resolved Hide resolved
Comment on lines 120 to 170
.transient(transient)
.hive_formats(Some(Default::default()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.transient(transient)
.hive_formats(Some(Default::default()));
.transient(transient);

Is hive_formats needed in this case or it can be set to None?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hive_formats is tested in a generic test against all dialects, since I don't know which dialect uses the hive_format I added the default value in the return struct to keep that test happy.

src/dialect/snowflake.rs Show resolved Hide resolved
src/dialect/snowflake.rs Outdated Show resolved Hide resolved
src/dialect/snowflake.rs Outdated Show resolved Hide resolved
tests/sqlparser_snowflake.rs Outdated Show resolved Hide resolved
src/dialect/snowflake.rs Show resolved Hide resolved
src/dialect/snowflake.rs Outdated Show resolved Hide resolved
src/dialect/snowflake.rs Show resolved Hide resolved
@@ -40,6 +40,213 @@ fn test_snowflake_create_table() {
}
}

#[test]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general comment: can we include some negative tests as well? examples like incompatible keywords being used, incomplete keyword sequence etc

@coveralls
Copy link

coveralls commented May 3, 2024

Pull Request Test Coverage Report for Build 9423048544

Details

  • 410 of 452 (90.71%) changed or added relevant lines in 9 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.04%) to 88.878%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ast/mod.rs 16 18 88.89%
tests/sqlparser_postgres.rs 14 17 82.35%
tests/sqlparser_snowflake.rs 192 196 97.96%
src/ast/dml.rs 28 37 75.68%
src/ast/helpers/stmt_create_table.rs 52 62 83.87%
src/dialect/snowflake.rs 92 106 86.79%
Files with Coverage Reduction New Missed Lines %
src/ast/dml.rs 1 70.59%
Totals Coverage Status
Change from base Build 9416866271: 0.04%
Covered Lines: 26258
Relevant Lines: 29544

💛 - Coveralls

Copy link
Collaborator

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @balliegojr, I think this is looking really close to me.

A few more comments and I think it will be ready to go

src/ast/mod.rs Show resolved Hide resolved
src/dialect/snowflake.rs Outdated Show resolved Hide resolved
src/ast/mod.rs Outdated
}
}

/// Helper to indicate if a collection should be wrapped by a symbol when displaying
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some examples would be nice here too.

Also perhaps we should implement Display for this enum which would simplify the callsites

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have implemented Display for Vec<T> only, I believe the HashMap may have different representation depending on the dialect/use case, but let me know if I should implement Display for the HashMap as well

tests/sqlparser_clickhouse.rs Outdated Show resolved Hide resolved
@alamb
Copy link
Collaborator

alamb commented May 3, 2024

Thank you @iffyio for the review

Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@balliegojr sorry for the delay getting back to this PR
The changes LGTM! only the minor comment regarding the clickhouse_generic test and a conflicts to resolve from main

@balliegojr balliegojr force-pushed the snowflake-create-table-support branch from a48498f to a5b771d Compare June 5, 2024 18:53
Copy link
Contributor

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alamb , I think we can merge this

Copy link
Collaborator

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Epic work @balliegojr -- thank you so much

Thank you for the review @lovasoa

@alamb
Copy link
Collaborator

alamb commented Jun 7, 2024

🤔 looks like there are some CI failures,.

@balliegojr balliegojr force-pushed the snowflake-create-table-support branch from a5b771d to e1452bc Compare June 7, 2024 20:59
@balliegojr
Copy link
Contributor Author

@alamb I believe I solved the CI issues

@alamb alamb merged commit be77ce5 into sqlparser-rs:main Jun 9, 2024
10 checks passed
@alamb
Copy link
Collaborator

alamb commented Jun 9, 2024

Thanks again @iffyio @balliegojr and @lovasoa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants