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 SQLite strict tables #493

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

evgeniy-terekhin
Copy link
Contributor

@evgeniy-terekhin evgeniy-terekhin commented Oct 31, 2022

this PR adds support for sqlite strict tables
it does so by adding a new variant to TableOpt variant

example:
create table test(a, b) (regular create statement)
create table test(a int, b int) strict (strict create statement)

Questions

  1. Is TableOpt the right place to put this functionality in?
  2. Does TableOpt have to be public given that there are builder methods for each individual option?
  3. I noticed that TableBuilder::table_prepare_create_statement has only a default impl and TableOpts attempt to be rendered for any backend even though only MySQL (and SQLite starting with this PR) need those? I am pretty sure I understand why it is so (for the sake of code sharing probably) but wouldn't it make sense for each backend to have it's own implementation?

@evgeniy-terekhin
Copy link
Contributor Author

it might make sense to rewrite this PR in terms of TableModifier introduced in this PR

@@ -101,6 +101,7 @@ pub trait TableBuilder: IndexBuilder + ForeignKeyBuilder + QuotedBuilder + Table
TableOpt::Engine(s) => format!("ENGINE={}", s),
TableOpt::Collate(s) => format!("COLLATE={}", s),
TableOpt::CharacterSet(s) => format!("DEFAULT CHARSET={}", s),
TableOpt::Strict => "STRICT".to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

Hey @evgeniy-terekhin, sorry for the delay.

Since STRICT is a SQLite only keyword, we better append it on SQLite's table_builder instead of doing it in the common table_builder.

@ikrivosheev
Copy link
Member

@evgeniy-terekhin hello! Sorry with the delay. I think is better split TableOpt into backend specific options, what do you think?

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

Successfully merging this pull request may close these issues.

3 participants