sea-query icon indicating copy to clipboard operation
sea-query copied to clipboard

Update SQLite backend Boolean type from int to bool

Open anshulxyz opened this issue 2 years ago • 4 comments

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

  • Closes #375

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

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

anshulxyz avatar Aug 01 '22 20:08 anshulxyz

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

billy1624 avatar Aug 09 '22 04:08 billy1624

@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 avatar Aug 09 '22 11:08 anshulxyz

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 - https://github.com/SeaQL/sea-query/issues/375 - once we bumped the SeaORM version on SeaSchema side.

billy1624 avatar Aug 09 '22 14:08 billy1624

@billy1624 should we make fix in sea-schema?

ikrivosheev avatar Aug 09 '22 14:08 ikrivosheev

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?

tyt2y3 avatar Aug 20 '22 14:08 tyt2y3

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

Hey @tyt2y3, correct!

billy1624 avatar Aug 29 '22 03:08 billy1624