Support for table column type identity with sequence options, create sequence, drop sequnece. by samjay000 · Pull Request #587 · apache/datafusion-sqlparser-rs

samjay000 · 2022-08-20T07:45:04Z

Support added for

Table column def type GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY [ ( sequence_options ) ] | (https://www.postgresql.org/docs/current/sql-createtable.html)
Create sequence 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 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:

sqlparser_postgres::parse_create_table_generated_always_as_identity
sqlparser_postgres::parse_create_sequence
sqlparser_postgres::parse_drop_sequence

"sequence_options" are pendonf

…port' into generated_always_as_identity_support # Conflicts: # tests/sqlparser_postgres.rs

…ce added.

"sequence_options" are for CREATE TABLE statement.

andygrove · 2022-08-26T21:21:27Z

Thanks @Sam-mmm I will try and make time to review this next week.

coveralls · 2022-08-26T21:25:12Z

Pull Request Test Coverage Report for Build 2893987821

197 of 273 (72.16%) changed or added relevant lines in 4 files are covered.
168 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-1.5%) to 83.977%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/ast/ddl.rs	11	16	68.75%
src/ast/mod.rs	49	65	75.38%
src/parser.rs	90	145	62.07%

Files with Coverage Reduction	New Missed Lines	%
src/ast/mod.rs	2	77.25%
src/parser.rs	6	82.58%
src/lib.rs	160	7.49%

Totals
Change from base Build 2889184458:	-1.5%
Covered Lines:	9670
Relevant Lines:	11515

💛 - Coveralls

…pport

samjay000 · 2022-08-27T07:57:03Z

Fixed issues with check list and added additional test scenarios

alamb

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

alamb · 2022-09-27T13:30:35Z

      with:
        github-token: ${{ secrets.GITHUB_TOKEN }}

+


why this change?

alamb · 2022-09-27T13:32:05Z

+
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+pub enum AlwaysOrByDefaultOrAlwaysAs {


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

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?

alamb · 2022-09-27T13:32:58Z

            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,


I wonder why the different names here?

Like why not

Suggested change

always_or_by_default_or_always_as: always_or_by_default_or_stored,

always_or_by_default_or_always_as,

alamb · 2022-09-27T13:35:25Z

+
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+pub enum AsIdentityOrStored {


This enum does not appear to be used

alamb · 2022-09-27T13:37:04Z

+                let mut as_type: String = String::from("");
+                if let Some(dt) = data_type.as_ref() {
+                    as_type = " AS ".to_string() + &dt.to_string();
+                }


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

Suggested change

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()

}

alamb · 2022-09-27T13:42:19Z

+                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, "")
+                }


You can write this type of code more concisely (and have the compiler verify you covered all the cases) like:

Suggested change

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")

alamb · 2022-09-27T13:44:46Z

+    ///MinValue(minvalue, 'NO MINVALUE')
+    MinValue(Option<Expr>, Option<bool>),
+    ///MaxValue(maxvalue, 'NO MAXVALUE')
+    MaxValue(Option<Expr>, Option<bool>),


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) }

alamb · 2022-09-27T13:45:34Z

+// use sqlparser::ast::Value::Boolean;
 use sqlparser::dialect::{GenericDialect, PostgreSqlDialect};
 use sqlparser::parser::ParserError;
+use test_utils::*;


❤️ very nice tests

alamb · 2022-09-27T13:45:55Z

 }

-#[test]
-fn parse_array_subquery_expr() {


Can you please explain why this test was deleted?

alamb · 2022-09-27T13:46:07Z

 }

-#[test]
-fn parse_current_functions() {


likewise, why is this test deleted?

samjay000 · 2022-10-14T16:22:23Z

@alamb I have created PR #673, will create series of small PR's for rest of features.
So I'm closing this PR.

samjay000 added 11 commits August 13, 2022 23:35

Implemantation for GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY

9d6a7a3

"sequence_options" are pendonf

Merge remote-tracking branch 'origin/generated_always_as_identity_sup…

f2819a6

…port' into generated_always_as_identity_support # Conflicts: # tests/sqlparser_postgres.rs

Statement::CreateSequence, SequenceOptions Statement::Drop for sequen…

6d92bb0

…ce added.

Untested Statement::CreateSequence and Display impl.

a87f264

Test case added for Statement::CreateSequence

62dcb78

Fix clippy warnings and run fmt

86c795d

Implementation for GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY

c1db58a

"sequence_options" are for CREATE TABLE statement.

Fix clippy warnings

a6f1325

Uncomment all test cases

c4b3aa9

Merge branch 'main' into generated_always_as_identity_support

d0621de

Added DROP SEQUENCE and fix issues

d7f1948

samjay000 added 9 commits August 27, 2022 08:12

Merge branch 'sqlparser-rs:main' into generated_always_as_identity_su…

581025d

…pport

remove publish

6496e5a

remove publish

94cafd9

Updates for checklist

d516a0b

Updates for checklist

2e86fd9

Additional test scenarios

7c90fd2

Additional test scenarios

84e55f1

Additional test scenarios

fe2598e

Additional test scenarios

8fbd08f

alamb reviewed Sep 27, 2022

View reviewed changes

samjay000 mentioned this pull request Oct 3, 2022

Is generated always as identity not supported. #560

Closed

samjay000 closed this Oct 14, 2022

	always_or_by_default_or_always_as: always_or_by_default_or_stored,
	always_or_by_default_or_always_as,

Conversation

samjay000 commented Aug 20, 2022

Uh oh!

andygrove commented Aug 26, 2022

Uh oh!

coveralls commented Aug 26, 2022

Pull Request Test Coverage Report for Build 2893987821

💛 - Coveralls

Uh oh!

samjay000 commented Aug 27, 2022

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samjay000 commented Oct 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants