Skip to content

Correcting Partitioning Logic for VarBinary Primary Key Type.#2064

Merged
VardhanThigle merged 2 commits intoGoogleCloudPlatform:mainfrom
VardhanThigle:varbinary
Dec 19, 2024
Merged

Correcting Partitioning Logic for VarBinary Primary Key Type.#2064
VardhanThigle merged 2 commits intoGoogleCloudPlatform:mainfrom
VardhanThigle:varbinary

Conversation

@VardhanThigle
Copy link
Copy Markdown
Contributor

@VardhanThigle VardhanThigle commented Dec 16, 2024

Correcting Collation Mapping Query for Varbinary. Adding Integration test for all supported Pkey types on MySql.

Overview

For VarBinary, the collation used is binary which also happens to be a reserved keyword. We need to escape it.
With Support for mapping to bigIntegers in this PR, we are directly mapping binary columns to bigIntegers as the binary collation is always ordered by the value of the byte.
Also for future proofing, all the collations and character sets on MySql will be escaped with a backtick.

Please refer to https://dev.mysql.com/doc/refman/8.4/en/charset-binary-collations.html for more details.

Testing

  1. End to End manual run for Big Int Unsigned and VarBinary succeeds.
  2. Integration tests for Supported PK types

TODO (For Followup PR):

  1. Similar Integration Testing on PG
  2. List of Supported Primary types in README.md

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.18%. Comparing base (d9243b5) to head (10d43b7).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2064      +/-   ##
============================================
+ Coverage     46.31%   54.18%   +7.87%     
+ Complexity     3919     1542    -2377     
============================================
  Files           857      389     -468     
  Lines         51215    21342   -29873     
  Branches       5389     2141    -3248     
============================================
- Hits          23719    11565   -12154     
+ Misses        25783     9085   -16698     
+ Partials       1713      692    -1021     
Components Coverage Δ
spanner-templates 69.40% <100.00%> (+1.31%) ⬆️
spanner-import-export ∅ <ø> (∅)
spanner-live-forward-migration 76.42% <ø> (ø)
spanner-live-reverse-replication 77.63% <ø> (ø)
spanner-bulk-migration 87.22% <100.00%> (+0.05%) ⬆️
Files with missing lines Coverage Δ
...jdbc/dialectadapter/mysql/MysqlDialectAdapter.java 99.61% <100.00%> (-0.01%) ⬇️
...source/reader/io/jdbc/iowrapper/JdbcIoWrapper.java 93.92% <100.00%> (ø)
...niformsplitter/range/BoundaryExtractorFactory.java 100.00% <100.00%> (ø)
...uniformsplitter/range/BoundarySplitterFactory.java 100.00% <100.00%> (ø)
...source/reader/io/schema/SourceColumnIndexInfo.java 66.66% <100.00%> (+1.44%) ⬆️

... and 485 files with indirect coverage changes

@VardhanThigle VardhanThigle changed the title Correcting Collation Mapping Query for Varbinary. [DRAFT] Correcting Collation Mapping Query for Varbinary. Dec 16, 2024
@VardhanThigle VardhanThigle force-pushed the varbinary branch 10 times, most recently from f6500c7 to 621df32 Compare December 17, 2024 08:43
@VardhanThigle VardhanThigle marked this pull request as ready for review December 17, 2024 08:51
@VardhanThigle VardhanThigle requested a review from a team as a code owner December 17, 2024 08:51
@VardhanThigle VardhanThigle changed the title [DRAFT] Correcting Collation Mapping Query for Varbinary. Correcting Partitioning Logic for VarBinary. Dec 17, 2024
Copy link
Copy Markdown
Contributor

@Deep1998 Deep1998 left a comment

Choose a reason for hiding this comment

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

LGTM

@VardhanThigle VardhanThigle merged commit 5d6e63c into GoogleCloudPlatform:main Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants