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

sql-schema-connector: remove BINARY from queries #4131

Closed
wants to merge 1 commit into from

Conversation

Jolg42
Copy link
Contributor

@Jolg42 Jolg42 commented Aug 11, 2023

From #4130

Internal Slack

Context: vitessio/vitess#13764 (comment)

Docs: https://dev.mysql.com/doc/refman/8.0/en/cast-functions.html

The BINARY operator converts the expression to a binary string (a string that has the binary character set and binary collation). A common use for BINARY is to force a character string comparison to be done byte by byte using numeric byte values rather than character by character. The BINARY operator also causes trailing spaces in comparisons to be significant.

Deprecation note:

The BINARY operator is deprecated as of MySQL 8.0.27, and you should expect its removal in a future version of MySQL. Use CAST(... AS BINARY) instead.

So one question to resolve before we can merge this is to know if the behavior mentioned in the docs is required here for us.

@Jolg42 Jolg42 requested a review from a team as a code owner August 11, 2023 09:58
@Jolg42 Jolg42 changed the title remove BINARY from query sql-schema-connector: remove BINARY from queries Aug 11, 2023
@Jolg42 Jolg42 marked this pull request as draft August 11, 2023 10:01
@Jolg42 Jolg42 requested a review from tomhoule August 11, 2023 10:04
@Jolg42 Jolg42 added kind/tech A technical change. topic: tech debt labels Aug 11, 2023
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 11, 2023

CodSpeed Performance Report

Merging #4131 will improve performances by 5.01%

Comparing joel/remove-BINARY (aec6ac6) with main (9fe3fe4)

Summary

🔥 1 improvements
✅ 10 untouched benchmarks

Benchmarks breakdown

Benchmark main joel/remove-BINARY Change
🔥 large_read 7.7 ms 7.3 ms +5.01%

@Jolg42
Copy link
Contributor Author

Jolg42 commented Aug 11, 2023

Note that some tests are failing, I didn't check why yet.

@tomhoule
Copy link
Contributor

The reason for the casts to BINARY is sort order (so the ordering matches between the database and application code), so we should still cast, but the non-deprecated way.

@Jolg42
Copy link
Contributor Author

Jolg42 commented Aug 17, 2023

Closing this for now, I persisted the deprecation into a new issue #4146

@Jolg42 Jolg42 closed this Aug 17, 2023
@Jolg42 Jolg42 deleted the joel/remove-BINARY branch August 17, 2023 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants