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

Remove SET NAMES usage which is no-op in SingleStore #33

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

pmishchenko-ua
Copy link
Collaborator

No description provided.

@pmishchenko-ua pmishchenko-ua changed the title Remove SET NAMES usage which is no-op in SingleStore WIP: Remove SET NAMES usage which is no-op in SingleStore Aug 30, 2024
@pmishchenko-ua pmishchenko-ua force-pushed the pavlo-fix-set-character-set branch from 0513ee9 to ad9c98a Compare August 30, 2024 13:37
@pmishchenko-ua pmishchenko-ua changed the title WIP: Remove SET NAMES usage which is no-op in SingleStore Remove SET NAMES usage which is no-op in SingleStore Aug 30, 2024
@pmishchenko-ua pmishchenko-ua force-pushed the pavlo-fix-set-character-set branch from ad9c98a to 58f9436 Compare August 30, 2024 14:15
singlestoredb/mysql/connection.py Outdated Show resolved Hide resolved
Update Connection.encoding based on charset.

If charset/collection are being set to utf8mb4, the corresponding global
setting must be also utf8mb4.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm just wondering about this comment, this means, that the user should change it globally manually, before calling this function, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated the comment, please check.
We also need @kesmit13 opinion on this behavior. Maybe we should catch the error and print a warning instead, but ignoring it could lead to weird errors I guess - e.g when the user sets charset=utf8mb4 in connect(), and then tries to write 4-byte symbols to utf8(mb3) tables

Copy link
Collaborator

@kesmit13 kesmit13 left a comment

Choose a reason for hiding this comment

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

Does this change work in SingleStoreDB before 8.7? If not, we need to put in some sort of conditional to only do this change for that version. We can't break compatibility with older server versions.

@pmishchenko-ua
Copy link
Collaborator Author

Does this change work in SingleStoreDB before 8.7? If not, we need to put in some sort of conditional to only do this change for that version. We can't break compatibility with older server versions.

@kesmit13 This change does not depend on 8.7, as these variables have been introduced a while ago. Prior to 8.7, utf8mb4 was not the default database charset, so setting the session character set to utf8mb4 would require changing it on the server first.

@kesmit13 kesmit13 merged commit 31e8124 into main Jan 22, 2025
11 checks passed
@kesmit13 kesmit13 deleted the pavlo-fix-set-character-set branch January 22, 2025 15:14
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.

3 participants