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

Update SQLite backend Boolean type from int to bool #400

Merged
merged 3 commits into from
Sep 18, 2022

Conversation

anshulxyz
Copy link
Contributor

@anshulxyz anshulxyz commented Aug 1, 2022

As mentioned in #375 the SQLite engine already supports boolean type implicitly. More details are provided in the issue's comments about this.

PR Info

Adds

  • Explicit support for booleans in SQLite backend

Breaking Changes

  • I don't think this is backwards compatible, because after this update the Entity will look for a boolean, until now we were using int there. So in case, someone makes an update, and they provide and int where we expect a bool, it might break some code. This still needs to be checked.
  • Immediate change the this PR should bring is the difference in generated Entity's field type should change from i32 to boolean.

Changes

  • I have added a few unit test cases related to this PR, for SQLite's bool support

- Add test for bool column for table creation in SQLite
- Add test for inserting boolean values in SQLite
@billy1624 billy1624 requested a review from tyt2y3 August 8, 2022 13:30
anshulxyz and others added 2 commits August 8, 2022 19:47
Renamed "bool" to "boolean"

Co-authored-by: Billy Chan <ccw.billy.123@gmail.com>
@billy1624
Copy link
Member

billy1624 commented Aug 9, 2022

Context:

  • image

Hey @anshul#9329, if you look at the rules of type affinity

For tables not declared as STRICT, the affinity of a column is determined by the declared type of the column, according to the following rules in the order shown:

1. If the declared type contains the string "INT" then it is assigned INTEGER affinity.

2. If the declared type of the column contains any of the strings "CHAR", "CLOB", or "TEXT" then that column has TEXT affinity. Notice that the type VARCHAR contains the string "CHAR" and is thus assigned TEXT affinity.

3. If the declared type for a column contains the string "BLOB" or if no type is specified then the column has affinity BLOB.

4. If the declared type for a column contains any of the strings "REAL", "FLOA", or "DOUB" then the column has REAL affinity.

5. Otherwise, the affinity is NUMERIC.

https://www.sqlite.org/datatype3.html#determination_of_column_affinity

The column defined with bool / boolean datatype are essentially treated as NUMERIC inside SQLite internal.

I prefer boolean over bool because of two reasons:

  1. I mentioned above - Just to be aligned with the type affinity listed on SQLite docs: https://www.sqlite.org/datatype3.html#affinity_name_examples
  2. We don't need to update any code inside sea-schema to parse the boolean column correctly. Because it's searching for boolean instead of bool: https://github.com/SeaQL/sea-schema/blob/1d377ed9ef33e3b0bd090c35b54c9c893e3076f2/src/sqlite/def/types.rs#L72

@anshulxyz
Copy link
Contributor Author

@billy1624 Thanks you for explaining. I'd totally missed this part of the table.
Screenshot 2022-08-09 at 5 15 33 PM

I've already renamed it to boolean in previous commits. What should be the next steps from here?

@anshulxyz anshulxyz requested a review from billy1624 August 9, 2022 11:49
@anshulxyz anshulxyz marked this pull request as ready for review August 9, 2022 11:49
@billy1624
Copy link
Member

I've already renamed it to boolean in previous commits. What should be the next steps from here?

I think it's all good! After this PR is being merged, we should mention this bareaking change in the CHANGELOG. This PR should fix the original issue - #375 - once we bumped the SeaORM version on SeaSchema side.

Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

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

Thanks again for contributing!! @anshulxyz

@ikrivosheev
Copy link
Member

@billy1624 should we make fix in sea-schema?

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 20, 2022

We don't need to update any code inside sea-schema to parse the boolean column correctly. Because it's searching for boolean instead of bool: https://github.com/SeaQL/sea-schema/blob/1d377ed9ef33e3b0bd090c35b54c9c893e3076f2/src/sqlite/def/types.rs#L72

According to my understanding, it will 'just work' with SeaSchema. Am I right?

@billy1624
Copy link
Member

According to my understanding, it will 'just work' with SeaSchema. Am I right?

Hey @tyt2y3, correct!

@tyt2y3 tyt2y3 merged commit cbb8e9d into SeaQL:master Sep 18, 2022
@anshulxyz anshulxyz deleted the add-booleans-for-sqlite branch September 19, 2022 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

SQLite: Booleans become integer columns
4 participants