Skip to content

Migrates all boolean-type integers in SQL, to tiny integers. #20759

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

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

Conversation

Generalcamo
Copy link
Contributor

@Generalcamo Generalcamo commented May 14, 2025

NUFC

Modern versions of SQL and its various forks and variations ignores the size specified in data types, such as INT(2) (this will always be 4 bytes). The recommended way to handle boolean values is either:

  • Through the use of the BOOLEAN data type
  • Through the use of a TINYINT(1) data type (which BOOLEAN is equivalent to data-wise)

After consulting with arrow, the second way is preferred. This has been losslessly applied to all prior entries in the table.

@Generalcamo
Copy link
Contributor Author

!review

@github-actions github-actions bot added the Database The PR or issue affects the database. label May 14, 2025
@Generalcamo Generalcamo marked this pull request as draft May 14, 2025 13:51
@Generalcamo Generalcamo marked this pull request as ready for review May 16, 2025 02:28
@Generalcamo
Copy link
Contributor Author

Tested and confirmed to work on a live database.

Copy link
Member

@Arrow768 Arrow768 left a comment

Choose a reason for hiding this comment

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

Not sure why the existing DB columns need to be changed, but the code is fine.
(We dont have that many table entries that there will be a significant change by swapping the columns over to tinyint from int)

@Arrow768
Copy link
Member

Arrow768 commented May 18, 2025

The check is failing due to a lack of permission.
The check is failing because of incorrect indentation.

It also complains about a lack of permissions to post the result on the PR.

#20773 should fix that lack of permissions.

@Arrow768 Arrow768 added Changes Required The PR requires changes before it can be approved and/or merged. and removed Review Required labels May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Required The PR requires changes before it can be approved and/or merged. Database The PR or issue affects the database.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants