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 DEFERRED, IMMEDIATE, and EXCLUSIVE in SQLite's BEGIN TRANSACTION command #1067

Merged
merged 4 commits into from
Dec 20, 2023

Conversation

takaebato
Copy link
Contributor

@takaebato takaebato commented Dec 15, 2023

This PR adds support for the following BEGIN TRANSACTION syntaxes in SQLite. docs

  • BEGIN DEFERRED
  • BEGIN IMMEDIATE
  • BEGIN EXCLUSIVE

Since there was no term in the documentation to refer to the words DEFERRED, IMMEDIATE, and EXCLUSIVE, I decided to use the term modifier. The term mode is also a candidate, but I avoided it to prevent confusion with transaction mode in standard SQL.

This PR's implementation follows the approach used in previous PR with similar changes.

I would love to hear any ideas for better naming or implementations!

Support the following syntaxes
- BEGIN DEFERRED
- BEGIN IMMEDIATE
- BEGIN EXCLUSIVE
@takaebato takaebato force-pushed the sqlite-begin-transaction-modifiers branch from 12370a2 to 1318e49 Compare December 15, 2023 15:06
@takaebato
Copy link
Contributor Author

takaebato commented Dec 16, 2023

I found an implementation that looks better, so I will refactor this PR to take that into account.

@takaebato
Copy link
Contributor Author

This commit f2ad35e is somewhat experimental, and I think it could be helpful for thoroughly testing all dialects.
Additionally, when a new dialect is introduced, updating the ALL_DIALECT_NAMES list ensures it is included in all test cases, lowering the risk of overlooking any updates.

But I think it is a bit complex, so I don't mind at all switching to another approach.
I am looking forward to any feedback!

@coveralls
Copy link

coveralls commented Dec 19, 2023

Pull Request Test Coverage Report for Build 7274283364

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 38 of 46 (82.61%) changed or added relevant lines in 6 files are covered.
  • 1097 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-0.03%) to 87.675%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ast/mod.rs 11 12 91.67%
src/dialect/generic.rs 1 2 50.0%
src/dialect/mod.rs 1 2 50.0%
src/dialect/sqlite.rs 1 2 50.0%
src/parser/mod.rs 7 8 87.5%
tests/sqlparser_sqlite.rs 17 20 85.0%
Files with Coverage Reduction New Missed Lines %
src/dialect/mysql.rs 1 97.78%
tests/sqlparser_mysql.rs 2 99.45%
src/ast/ddl.rs 6 83.78%
tests/sqlparser_sqlite.rs 7 95.88%
src/dialect/mod.rs 20 80.53%
src/ast/query.rs 59 83.08%
src/ast/mod.rs 368 79.25%
src/parser/mod.rs 634 83.26%
Totals Coverage Status
Change from base Build 7009122777: -0.03%
Covered Lines: 18275
Relevant Lines: 20844

💛 - Coveralls

@alamb
Copy link
Contributor

alamb commented Dec 19, 2023

This commit f2ad35e is somewhat experimental, and I think it could be helpful for thoroughly testing all dialects.

I agree it is fairly complex

I think the more standard way for testing multiple dialects is to programatically construct TestedDialects like

https://github.com/sqlparser-rs/sqlparser-rs/blob/1933f194e76c9b9e228135f4cce1ca0936288dc4/tests/sqlparser_common.rs#L1658-L1668

This has all the available dialects -- perhaps you can derive the incude/exclude lists from that https://github.com/sqlparser-rs/sqlparser-rs/blob/64eccdbba2fbc69fdb76ee451a1c43e30e7bfe3c/src/test_utils.rs#L196-L213 instead as that already exists

Copy link
Contributor

@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.

Thank you for the contribution @takaebato . I have a suggestion on testing, but otherwise I think this PR looks very nice to me 👌

@@ -38,4 +38,8 @@ impl Dialect for GenericDialect {
fn supports_group_by_expr(&self) -> bool {
true
}

fn supports_start_transaction_modifier(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

#[test]
fn parse_start_transaction_with_modifier() {
let (supported_dialects, unsupported_dialects) =
partition_all_dialects_by_inclusion(vec!["generic", "sqlite"]);
Copy link
Contributor

Choose a reason for hiding this comment

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

it isn't clear to me that using a String is preferable here to simply explicitly referring to SqliteDialect and GenericDialect

Using a string defers any errors to runtime rather than compile time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback!

(I have not yet been able to learn Rust properly, so I may be missing the point..)
I first tried to pass the arguments like this

partition_all_dialects_by_inclusion(vec![Box::new(SQLiteDialect {}), Box::new(GenericDialect {})])

but I got the following error indicating that trait objects cannot be compared.

error[E0277]: can't compare `dyn dialect::Dialect` with `dyn dialect::Dialect`
    --> src/test_utils.rs:230:36
     |
230  |         .filter(|d| !dialect_names.contains(d))
     |                                    ^^^^^^^^ no implementation for `dyn dialect::Dialect == dyn dialect::Dialect`
     |
     = help: the trait `PartialEq` is not implemented for `dyn dialect::Dialect`
     = note: required for `Box<dyn dialect::Dialect>` to implement `PartialEq`

I couldn't think of a suitable way to deal with this, so I changed to using String which can be compared.
But yes, this is not a good solution, so you can skip it!

@takaebato
Copy link
Contributor Author

takaebato commented Dec 20, 2023

Thanks!
This all_dialects_but_pg is close to what was wanted!
I fixed tests and documentation: c5357bf

@takaebato takaebato requested a review from alamb December 20, 2023 11:32
Copy link
Contributor

@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.

Thank you @takaebato

@alamb alamb merged commit 1baec96 into apache:main Dec 20, 2023
10 checks passed
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.

3 participants