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
Support for table column type identity with sequence options, create sequence, drop sequnece. #587
Support for table column type identity with sequence options, create sequence, drop sequnece. #587
Conversation
"sequence_options" are pendonf
…port' into generated_always_as_identity_support # Conflicts: # tests/sqlparser_postgres.rs
"sequence_options" are for CREATE TABLE statement.
Thanks @sam-mmm I will try and make time to review this next week. |
Pull Request Test Coverage Report for Build 2893987821
💛 - Coveralls |
Fixed issues with check list and added additional test scenarios |
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.
Thank you for the contribution @sam-mmm -- this is a great start. With a few more doc comments and some cleanup I think this PR will be ready to go
I apologize for the belated review
@@ -74,6 +74,7 @@ jobs: | |||
with: | |||
github-token: ${{ secrets.GITHUB_TOKEN }} | |||
|
|||
|
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.
why this change?
|
||
#[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ||
pub enum AlwaysOrByDefaultOrAlwaysAs { |
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.
Can you please add docstrings here (following the examples above) that explain the SQL syntax this enum represents?
Likewise each of the enum variants should have documentation as well.
The same comment applies to AsIdentityOrStored
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 wonder if calling this enum Generated
would better match the SQL it comes from and be more in line with the conventions of the rest of this crate?
@@ -411,6 +433,28 @@ impl fmt::Display for ColumnOption { | |||
DialectSpecific(val) => write!(f, "{}", display_separated(val, " ")), | |||
CharacterSet(n) => write!(f, "CHARACTER SET {}", n), | |||
Comment(v) => write!(f, "COMMENT '{}'", escape_single_quote_string(v)), | |||
Generated { | |||
always_or_by_default_or_always_as: always_or_by_default_or_stored, |
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 wonder why the different names here?
Like why not
always_or_by_default_or_always_as: always_or_by_default_or_stored, | |
always_or_by_default_or_always_as, |
|
||
#[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ||
pub enum AsIdentityOrStored { |
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 enum does not appear to be used
let mut as_type: String = String::from(""); | ||
if let Some(dt) = data_type.as_ref() { | ||
as_type = " AS ".to_string() + &dt.to_string(); | ||
} |
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.
As written this looks like it will do a bunch of string creation and copying that is unnecessary. Perhaps something like this would be less code and more efficient
let mut as_type: String = String::from(""); | |
if let Some(dt) = data_type.as_ref() { | |
as_type = " AS ".to_string() + &dt.to_string(); | |
} | |
let as_type = if let Some(dt) = data_type.as_ref() { | |
format!(" AS {}", dt); | |
} else { | |
"".to_string() | |
} |
if let Some(minvalue) = minvalue.as_ref() { | ||
write!(f, " MINVALUE {minvalue}", minvalue = minvalue) | ||
} else if no.is_some() { | ||
write!(f, " NO MINVALUE") | ||
} else { | ||
write!(f, "") | ||
} |
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.
You can write this type of code more concisely (and have the compiler verify you covered all the cases) like:
if let Some(minvalue) = minvalue.as_ref() { | |
write!(f, " MINVALUE {minvalue}", minvalue = minvalue) | |
} else if no.is_some() { | |
write!(f, " NO MINVALUE") | |
} else { | |
write!(f, "") | |
} | |
match minvalue.as_ref() { | |
(Some(minvalue), _) => write!(f, " MINVALUE {minvalue}", minvalue = minvalue), | |
(None, Some(_)) => write!(f, " NO MINVALUE") | |
(None, _) => write!(f, " NO MINVALUE") | |
///MinValue(minvalue, 'NO MINVALUE') | ||
MinValue(Option<Expr>, Option<bool>), | ||
///MaxValue(maxvalue, 'NO MAXVALUE') | ||
MaxValue(Option<Expr>, Option<bool>), |
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.
Instead of encoding this as Option<Expr>
and an `Option which can have 4 possible states, only three of which are valid, how about encoding it as an enum:
enum MinMaxValue {
// clause is not specified
Empty,
// NO MINVALUE/NO MAX VALUE
None,
// MINVALUE <expr> / MAXVALUE <expr>
Some(Expr)
}
use sqlparser::dialect::{GenericDialect, PostgreSqlDialect}; | ||
use sqlparser::parser::ParserError; | ||
use test_utils::*; |
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.
❤️ very nice tests
}), | ||
expr_from_projection(only(&select.projection)), | ||
); | ||
} | ||
|
||
#[test] | ||
fn parse_array_subquery_expr() { |
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.
Can you please explain why this test was deleted?
@@ -1618,52 +1808,6 @@ fn parse_declare() { | |||
pg_and_generic().verified_stmt("DECLARE \"SQL_CUR0x7fa44801bc00\" BINARY INSENSITIVE SCROLL CURSOR WITH HOLD FOR SELECT * FROM table_name LIMIT 2222"); | |||
} | |||
|
|||
#[test] | |||
fn parse_current_functions() { |
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.
likewise, why is this test deleted?
Support added for
GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( sequence_options ) ] |
(https://www.postgresql.org/docs/current/sql-createtable.html)CREATE [ TEMPORARY | TEMP ] SEQUENCE [ IF NOT EXISTS ] name [ AS data_type ] [ INCREMENT [ BY ] increment ] [ MINVALUE minvalue | NO MINVALUE ] [ MAXVALUE maxvalue | NO MAXVALUE ] [ START [ WITH ] start ] [ CACHE cache ] [ [ NO ] CYCLE ] [ OWNED BY { table_name.column_name | NONE } ]
(https://www.postgresql.org/docs/current/sql-createsequence.html)DROP SEQUENCE [ IF EXISTS ] name [, ...] [ CASCADE | RESTRICT ]
(https://www.postgresql.org/docs/current/sql-dropsequence.html)Only tested in regard to PostgreSQL documentation
Test Cases: