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

feat: added a type mismatch indicator (fixes #138) #143

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

rentalhost
Copy link
Contributor

When a column is defined with one type (e.g., INTEGER) but stores a value of another type (e.g., TEXT), an indicator is displayed with a tooltip showing the mismatched type. This is very useful since SQLite has flexible typing, which can cause issues when the data is handled in an environment where this behavior is not expected.

image
image
image

Important information:

I had to modify a series of files to make this work properly, but I tried to do it in the best way possible. However, the convertSqliteType() function in the sql-helper.ts file made me the most "uncomfortable," since it's used in many places.

I had to make this change because there are cases where the type is not explicitly defined for SQLite, such as when accessing the default sqlite_sequence table. It defines name and seq, but their types are "" (literally empty), and currently, the code was forcing them to be interpreted as TEXT, which seems incorrect. For example, seq is sent as an integer, so it should be treated as INTEGER.

This was done to ensure that the mismatch comparison system stops considering this field for analysis, as the column type is natively undefined.

When a column is defined with one type (e.g., INTEGER) but stores a value of another type (e.g., TEXT), an indicator is displayed with a tooltip showing the mismatched type. This is very useful since SQLite has flexible typing, which can cause issues when the data is handled in an environment where this behavior is not expected.
Copy link

vercel bot commented Aug 14, 2024

@rentalhost is attempting to deploy a commit to the Mediaload Team on Vercel.

A member of the Team first needs to authorize it.

@invisal
Copy link
Collaborator

invisal commented Aug 15, 2024

convertSqliteType() at the moment is only used for determine the header type and use it for render cell. There is no other use-case. It is everywhere because it is used in each driver implementation. Everything looks good. Let me test further.

@invisal invisal merged commit 926cb16 into outerbase:master Aug 15, 2024
1 of 3 checks passed
@invisal
Copy link
Collaborator

invisal commented Aug 16, 2024

@rentalhost
I commented the mismatch code temporary as it run into a few issues such as

  • Some driver do not support BigInteger which gives a false positive warning (This can easily be fixed)
  • Some driver do not even give us the header type (For sqljs driver does not return header type. I need to find better driver first)

Once I resolve these issues, I will uncomment it back

@rentalhost
Copy link
Contributor Author

@invisal Thanks! I'll check that out right now.

@invisal
Copy link
Collaborator

invisal commented Aug 16, 2024

@invisal Thanks! I'll check that out right now.

Thanks :)
Don't make PR on that part temporary. I am porting MySQL support this weekend and will touch that part. Don't want to conflict with each other :)

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.

2 participants