Adding support for parsing CREATE TRIGGER and DROP TRIGGER statements #1352

LucaCappelletti94 · 2024-07-25T18:01:59Z

This pull request starts from what was left off from this other closed pull request from two years ago. @zidaye had already made a considerable effort, I have primarily updated the PR so that it works with the code base two years later and extended the testing for its methods.

Looking forward to see it merged so I may be able to add support for TRIGGER in GlueSQL

alamb · 2024-07-25T21:43:30Z

fyi @lovasoa

coveralls · 2024-07-25T21:47:44Z

Pull Request Test Coverage Report for Build 10372520650

Details

447 of 505 (88.51%) changed or added relevant lines in 5 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.005%) to 89.047%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/ast/data_type.rs	0	1	0.0%
src/ast/mod.rs	40	48	83.33%
src/ast/trigger.rs	29	39	74.36%
src/parser/mod.rs	101	120	84.17%
tests/sqlparser_postgres.rs	277	297	93.27%

Files with Coverage Reduction	New Missed Lines	%
src/ast/mod.rs	1	81.58%

Totals
Change from base Build 10370172981:	-0.005%
Covered Lines:	28082
Relevant Lines:	31536

💛 - Coveralls

git-hulk · 2024-07-26T03:12:44Z

@LucaCappelletti94 Would you mind fixing format issues and test failures found in CI?

LucaCappelletti94 · 2024-07-26T04:38:17Z

Yeah, will get started shortly.

LucaCappelletti94 · 2024-07-26T07:14:19Z

@alamb, @git-hulk I need another GitHub Action run to be approved please.

src/ast/mod.rs

src/parser/mod.rs

src/test_utils.rs

src/parser/mod.rs

Co-authored-by: hulk <[email protected]>

git-hulk · 2024-07-26T11:00:30Z

cc @alamb

src/ast/mod.rs

src/ast/trigger.rs

src/parser/mod.rs

tests/sqlparser_postgres.rs

src/parser/mod.rs

src/ast/ddl.rs

iffyio · 2024-07-29T10:27:51Z

tests/sqlparser_postgres.rs

+
+#[test]
+fn parse_create_trigger() {
+    for referencing in [


The combinatorial loop is primarily the source of complexity in my mind and would ideally be flattened in order to simplify it.

tests/sqlparser_postgres.rs

src/ast/data_type.rs

src/parser/mod.rs

Co-authored-by: Ifeanyi Ubah <[email protected]>

… table name in CREATE TRIGGER

lovasoa · 2024-07-30T13:59:13Z

Cargo.toml

@@ -40,6 +40,7 @@ sqlparser_derive = { version = "0.2.0", path = "derive", optional = true }
 simple_logger = "5.0"
 matches = "0.1"
 pretty_assertions = "1"
+itertools = "0.13"


I'm not a huge fan of adding a new dependency...

It was either that or a much uglier Cartesian product. I agree that I would prefer to not add dependencies, but I was asked to try and clean the combinatorial for loop used to test the CREATE TRIGGER statement. I thought that with this being a dev-dependency of a well-known crate, it may be okay. Do let me know which other solution you would prefer to go for.

In the meantime, I will rewrite it without the use of itertools. Do let me know which variant you prefer.

@iffyio might disagree, but I think the nested fors are clearer than the itertools version.

The various arrays should be defined separately, but then I have not problem with

for var1 in arr1 { for var2 in arr2 { for var3 in arr3 { assert!(some_function(var1, var2, var3));

I have removed the itertools dependency and tried to make a version as clean as possible in commit d8db70e. Do let me know your opinion.

I would have written nested for loops, but I think your version is okay.

Ok! I find the flattened for-loops a bit easier to read, as otherwise, it gets to a very high level of indentation. Do let me know whether you have any more notes on the PR.

…ssible

lovasoa · 2024-07-31T09:00:59Z

tests/test_utils/extend_tuple.rs

+    fn extend_tuple(self, to_extend: (T1, T2, T3, T4, T5, T6, T7)) -> Self::Output {
+        (
+            to_extend.0,
+            to_extend.1,
+            to_extend.2,
+            to_extend.3,
+            to_extend.4,
+            to_extend.5,
+            to_extend.6,
+            self,
+        )
+    }
+}
+
+impl<T1, T2, T3, T4, T5, T6, T7, T8, T9> ExtendTuple<(T1, T2, T3, T4, T5, T6, T7, T8)> for T9 {
+    type Output = (T1, T2, T3, T4, T5, T6, T7, T8, T9);
+
+    fn extend_tuple(self, to_extend: (T1, T2, T3, T4, T5, T6, T7, T8)) -> Self::Output {
+        (
+            to_extend.0,
+            to_extend.1,
+            to_extend.2,
+            to_extend.3,
+            to_extend.4,
+            to_extend.5,
+            to_extend.6,
+            to_extend.7,
+            self,
+        )
+    }
+}
+
+impl<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10> ExtendTuple<(T1, T2, T3, T4, T5, T6, T7, T8, T9)>
+    for T10
+{
+    type Output = (T1, T2, T3, T4, T5, T6, T7, T8, T9, T10);
+
+    fn extend_tuple(self, to_extend: (T1, T2, T3, T4, T5, T6, T7, T8, T9)) -> Self::Output {
+        (
+            to_extend.0,
+            to_extend.1,
+            to_extend.2,
+            to_extend.3,
+            to_extend.4,
+            to_extend.5,
+            to_extend.6,
+            to_extend.7,
+            to_extend.8,
+            self,
+        )
+    }
+}
+
+impl<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11>
+    ExtendTuple<(T1, T2, T3, T4, T5, T6, T7, T8, T9, T10)> for T11
+{
+    type Output = (T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11);
+
+    fn extend_tuple(self, to_extend: (T1, T2, T3, T4, T5, T6, T7, T8, T9, T10)) -> Self::Output {
+        (
+            to_extend.0,
+            to_extend.1,
+            to_extend.2,
+            to_extend.3,
+            to_extend.4,
+            to_extend.5,
+            to_extend.6,
+            to_extend.7,
+            to_extend.8,
+            to_extend.9,
+            self,
+        )
+    }
+}
+
+impl<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12>
+    ExtendTuple<(T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11)> for T12
+{
+    type Output = (T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12);
+
+    fn extend_tuple(
+        self,
+        to_extend: (T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11),
+    ) -> Self::Output {
+        (
+            to_extend.0,
+            to_extend.1,
+            to_extend.2,
+            to_extend.3,
+            to_extend.4,
+            to_extend.5,
+            to_extend.6,
+            to_extend.7,
+            to_extend.8,
+            to_extend.9,
+            to_extend.10,
+            self,
+        )
+    }
+}


This is, in my opinion, worse than a bunch of nested for loops... But I wonldn't prevent merging over that. I think this PR is ok.

Yeah, these are quite ugly - I can wrap them in macros to remove at least the redundancy.

I don't feel like that would be worth it. A few nested for loops are better than :

adding a new dependency

creating a new trait that will be used just in one test case

creating a new trait AND a macro to implement that trait

We are talking about a single test. No need to overcomplicate things.

Even the principle of testing all possible combinations of syntaxes sounds a bit overkill to me.

I don't think we should delay merging that PR for longer. I would have preferred simpler tests, but the current state is okay. We can merge this now, I think.

@alamb

I'm not in favor of merging this (I think its too much machinery/code for a test case). The previous version with the combinatorial for loops I'd rather not either but I don't feel as strongly against merging that one at least so I'm defo OK with doing that if everyone's fine with it. (doesn't mean we can't merge this either ofc, merely my preference)

I agree with @iffyio

Let's keep the code / tests simpler even if it reuires some additional redundancy

I have replaced the complete CreateTrigger test suite with some selected test cases in commit 459dae1, as I believe the request was. I am not sure whether this was what was requested.

LucaCappelletti94 · 2024-08-08T21:10:56Z

I am not sure why the linter screams about that trait, it is definitely being used.

lovasoa · 2024-08-09T08:25:49Z

@LucaCappelletti94 , it doesn't seem to be used in tests/test_utils/mod.rs where it's imported

lovasoa

great, good for me !

alamb

Thank you @LucaCappelletti94 @git-hulk @lovasoa and @iffyio

This is one of the most well documented PRs I have seen in a long time.

I have one more question about some dead looking code, but I also think we could remove this as a follow on PR

alamb · 2024-08-13T15:09:12Z

tests/sqlparser_postgres.rs

@@ -4442,6 +4442,184 @@ fn test_table_unnest_with_ordinality() {
    }
 }

+fn possible_trigger_referencing_variants() -> Vec<Vec<TriggerReferencing>> {


These functions don't seem to be used anywhere 🤔 I would have expected clippy to complain about unused code

Update; it did!

Dead code removed, I am not sure why clippy didn't say anything.

alamb · 2024-08-13T15:18:13Z

I took the liberty of merging up from main and pushing a small test fix to this branch 0182a50 -- I also think that by doing so the CI should run automatically on this PR from now on

alamb · 2024-08-13T15:18:41Z

I plan to wait a day or two so @iffyio has time to review if they would like prior to merge

iffyio

Thanks @LucaCappelletti94!

alamb · 2024-08-14T13:11:26Z

Thanks again everyone -- this was some nice teamwork

…apache#1352) Co-authored-by: hulk <[email protected]> Co-authored-by: Ifeanyi Ubah <[email protected]> Co-authored-by: Andrew Lamb <[email protected]>

LucaCappelletti94 added 6 commits July 25, 2024 19:10

Added tests for the TRIGGERs

a3fa3b5

Added missing keywords needed for triggers

308b1a7

Created new module for the AST-related objects for the TRIGGERs

764707f

Integrated TRIGGERS in AST enum

637c632

Added methods to parse triggers

e760c07

Extended testing and resolved some corner cases in triggers

7474836

LucaCappelletti94 mentioned this pull request Jul 25, 2024

Add support for CREATE TRIGGER statements gluesql/gluesql#1520

Open

LucaCappelletti94 added 3 commits July 26, 2024 09:00

Formatted code

9fa36d9

Derived visitor trait and resolved clippy code smells

d6c3e51

Resolved documentation issue and extended test coverage for Triggers

8104de9

lovasoa approved these changes Jul 26, 2024

View reviewed changes

git-hulk reviewed Jul 26, 2024

View reviewed changes

src/ast/mod.rs Outdated Show resolved Hide resolved

src/parser/mod.rs Outdated Show resolved Hide resolved

src/test_utils.rs Outdated Show resolved Hide resolved

git-hulk reviewed Jul 26, 2024

View reviewed changes

src/parser/mod.rs Outdated Show resolved Hide resolved

LucaCappelletti94 and others added 3 commits July 26, 2024 12:22

Update src/parser/mod.rs

9c72a62

Co-authored-by: hulk <[email protected]>

Added support for optional "EACH" display and extended test suite

5a9a19e

Removed empty spaced with cargo fmt

d5a6af1

git-hulk approved these changes Jul 26, 2024

View reviewed changes

LucaCappelletti94 mentioned this pull request Jul 26, 2024

Migrating from sqlparser 0.46 to 0.50 (dev) gluesql/gluesql#1521

Closed

iffyio reviewed Jul 27, 2024

View reviewed changes

LucaCappelletti94 added 5 commits July 27, 2024 08:19

Renamed TRIGGER-related struct and fixed a copy-paste error and a typo

e6294c1

Reformatted code<

6f7c9c3

Merged DropFunctionDesc and TriggerFunctionDesc into FunctionDesc

429b202

Updated struct documentation

dbaca3a

Refactored code to make use of expect_keyboard where appropriate

3b26bc0

iffyio reviewed Jul 28, 2024

View reviewed changes

src/parser/mod.rs Outdated Show resolved Hide resolved

src/parser/mod.rs Outdated Show resolved Hide resolved

LucaCappelletti94 added 2 commits July 28, 2024 18:49

Removed more uses of and_then and then

3fc2684

Restored and made use in triggers of original ConstraintCharacteristics

09581e7

iffyio reviewed Jul 29, 2024

View reviewed changes

LucaCappelletti94 added 2 commits July 30, 2024 10:37

Reverted change relative to ConstraintCharacteristics

058cf7f

Merge branch 'main' into main

30199bf

iffyio reviewed Jul 30, 2024

View reviewed changes

tests/sqlparser_postgres.rs Outdated Show resolved Hide resolved

src/ast/data_type.rs Show resolved Hide resolved

src/parser/mod.rs Outdated Show resolved Hide resolved

LucaCappelletti94 and others added 4 commits July 30, 2024 15:16

Update tests/sqlparser_postgres.rs

7ab8fdb

Co-authored-by: Ifeanyi Ubah <[email protected]>

Added support for the CONSTRAINT keyword in triggers

6c5487f

Extended test suite and added support for CONSTRAINT keyword and FROM…

c0aab06

… table name in CREATE TRIGGER

Formatted code

c198e24

lovasoa reviewed Jul 30, 2024

View reviewed changes

LucaCappelletti94 added 3 commits July 30, 2024 17:08

Removed itertools dependency and tried to clean up code as much as po…

d8db70e

…ssible

Reformatted code

0f81194

Fixed comments

169a0be

lovasoa approved these changes Jul 31, 2024

View reviewed changes

Replaced complete test with partial tests

459dae1

lovasoa approved these changes Aug 13, 2024

View reviewed changes

alamb approved these changes Aug 13, 2024

View reviewed changes

LucaCappelletti94 and others added 3 commits August 13, 2024 17:11

Removed dead code

be1a682

Merge remote-tracking branch 'origin/main' into LucaCappelletti94/main

4268a9a

Fix clippy

0182a50

iffyio approved these changes Aug 14, 2024

View reviewed changes

alamb merged commit b072ce2 into apache:main Aug 14, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for parsing CREATE TRIGGER and DROP TRIGGER statements #1352

Adding support for parsing CREATE TRIGGER and DROP TRIGGER statements #1352

LucaCappelletti94 commented Jul 25, 2024

alamb commented Jul 25, 2024

coveralls commented Jul 25, 2024 •

edited

Loading

git-hulk commented Jul 26, 2024 •

edited

Loading

LucaCappelletti94 commented Jul 26, 2024

LucaCappelletti94 commented Jul 26, 2024

git-hulk commented Jul 26, 2024

iffyio Jul 29, 2024

lovasoa Jul 30, 2024

LucaCappelletti94 Jul 30, 2024

LucaCappelletti94 Jul 30, 2024

lovasoa Jul 30, 2024 •

edited

Loading

LucaCappelletti94 Jul 30, 2024

lovasoa Jul 31, 2024 •

edited

Loading

LucaCappelletti94 Jul 31, 2024

lovasoa Jul 31, 2024

LucaCappelletti94 Jul 31, 2024

lovasoa Jul 31, 2024

LucaCappelletti94 Jul 31, 2024

iffyio Aug 11, 2024

alamb Aug 13, 2024

LucaCappelletti94 Aug 13, 2024

LucaCappelletti94 commented Aug 8, 2024

lovasoa commented Aug 9, 2024

lovasoa left a comment

alamb left a comment

alamb Aug 13, 2024 •

edited

Loading

LucaCappelletti94 Aug 13, 2024

alamb commented Aug 13, 2024 •

edited

Loading

alamb commented Aug 13, 2024

iffyio left a comment

alamb commented Aug 14, 2024

Adding support for parsing CREATE TRIGGER and DROP TRIGGER statements #1352

Adding support for parsing CREATE TRIGGER and DROP TRIGGER statements #1352

Conversation

LucaCappelletti94 commented Jul 25, 2024

alamb commented Jul 25, 2024

coveralls commented Jul 25, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10372520650

Details

💛 - Coveralls

git-hulk commented Jul 26, 2024 • edited Loading

LucaCappelletti94 commented Jul 26, 2024

LucaCappelletti94 commented Jul 26, 2024

git-hulk commented Jul 26, 2024

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

lovasoa Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lovasoa Jul 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucaCappelletti94 commented Aug 8, 2024

lovasoa commented Aug 9, 2024

lovasoa left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Aug 13, 2024 • edited Loading

alamb commented Aug 13, 2024

iffyio left a comment

Choose a reason for hiding this comment

alamb commented Aug 14, 2024

coveralls commented Jul 25, 2024 •

edited

Loading

git-hulk commented Jul 26, 2024 •

edited

Loading

lovasoa Jul 30, 2024 •

edited

Loading

lovasoa Jul 31, 2024 •

edited

Loading

alamb Aug 13, 2024 •

edited

Loading

alamb commented Aug 13, 2024 •

edited

Loading