4 Byte PK support#4010
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces support for 4-byte character sets (such as emojis) in primary keys during source database to Spanner migrations. The changes involve refining the collation order logic for both MySQL and PostgreSQL to ensure these characters are correctly processed and validated, alongside new integration tests to validate the end-to-end migration flow. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the collation order queries for MySQL and PostgreSQL to support 4-byte UTF-8 characters and adds corresponding integration tests. Feedback highlights a significant performance concern in the MySQL query due to the massive increase in rows (to over 1.1 million) processed during 4-byte codepoint generation, which could cause high CPU/IO load and disk sorting. Additionally, the new integration tests should be refactored to use instance fields instead of static fields for resource managers and to declare the Dataflow job info as a local variable.
| 'WHERE t1.h BETWEEN ''00'' AND ''7f''' | ||
| ); | ||
|
|
||
| SET @all_utf8_hex = CONCAT(@four_byte_codepoints, ' UNION ALL ', @three_byte_codepoints, ' UNION ALL ', @two_byte_codepoints, ' UNION ALL ', @one_byte_codepoints); |
There was a problem hiding this comment.
Performance Concern: Scale of 4-Byte Codepoints
Including 4-byte codepoints increases the number of rows processed by this query from ~65,000 (for 1, 2, and 3-byte characters) to over 1.1 million (a ~17x increase).
Since the query performs 6 FIRST_VALUE and 2 DENSE_RANK window functions, MySQL will have to perform multiple large-scale sorts. With 1.1 million rows, this is highly likely to exceed the default MySQL sort_buffer_size (typically 256KB) and tmp_table_size, forcing MySQL to write large temporary files to disk.
This can cause:
- Significant CPU and I/O load on the source database during pipeline initialization.
- Query execution time to increase from < 60 seconds to several minutes, potentially causing timeouts.
Recommendations:
- Benchmark this query on a standard MySQL instance with default configuration to measure the performance impact.
- Consider if we can optimize the 4-byte range or if we can increase the session-level
sort_buffer_sizeandjoin_buffer_sizebefore running this query (e.g.,SET SESSION sort_buffer_size = ...).
| import org.apache.beam.it.gcp.spanner.SpannerResourceManager; | ||
| import org.apache.beam.it.gcp.spanner.matchers.SpannerAsserts; | ||
| import org.apache.beam.it.jdbc.JDBCResourceManager; | ||
| import org.apache.beam.it.jdbc.MySQLResourceManager; |
There was a problem hiding this comment.
The jobInfo field is only used within the testMySqlToSpanner method and does not need to be a class field. Additionally, the resource managers do not need to be static since they are initialized in the non-static @Before method. Making them instance fields prevents potential issues if tests are run in parallel or if multiple test methods are added in the future.
| import org.apache.beam.it.gcp.spanner.SpannerResourceManager; | |
| import org.apache.beam.it.gcp.spanner.matchers.SpannerAsserts; | |
| import org.apache.beam.it.jdbc.JDBCResourceManager; | |
| import org.apache.beam.it.jdbc.MySQLResourceManager; | |
| private MySQLResourceManager mySQLResourceManager; | |
| private SpannerResourceManager spannerResourceManager; |
| spannerResourceManager = setUpSpannerResourceManager(); | ||
| } |
There was a problem hiding this comment.
Declare jobInfo as a local variable since it is only used within this test method.
| spannerResourceManager = setUpSpannerResourceManager(); | |
| } | |
| PipelineLauncher.LaunchInfo jobInfo = | |
| launchDataflowJob( |
| import org.apache.beam.it.gcp.spanner.SpannerResourceManager; | ||
| import org.apache.beam.it.gcp.spanner.matchers.SpannerAsserts; | ||
| import org.apache.beam.it.jdbc.JDBCResourceManager; | ||
| import org.apache.beam.it.jdbc.PostgresResourceManager; |
There was a problem hiding this comment.
The jobInfo field is only used within the testPostgreSQLToSpanner method and does not need to be a class field. Additionally, the resource managers do not need to be static since they are initialized in the non-static @Before method. Making them instance fields prevents potential issues if tests are run in parallel or if multiple test methods are added in the future.
| import org.apache.beam.it.gcp.spanner.SpannerResourceManager; | |
| import org.apache.beam.it.gcp.spanner.matchers.SpannerAsserts; | |
| import org.apache.beam.it.jdbc.JDBCResourceManager; | |
| import org.apache.beam.it.jdbc.PostgresResourceManager; | |
| private PostgresResourceManager postgreSQLResourceManager; | |
| private SpannerResourceManager spannerResourceManager; |
| spannerResourceManager = setUpSpannerResourceManager(); | ||
| } |
There was a problem hiding this comment.
Declare jobInfo as a local variable since it is only used within this test method.
| spannerResourceManager = setUpSpannerResourceManager(); | |
| } | |
| PipelineLauncher.LaunchInfo jobInfo = | |
| launchDataflowJob( |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4010 +/- ##
============================================
+ Coverage 55.74% 63.49% +7.74%
+ Complexity 7241 2507 -4734
============================================
Files 1124 535 -589
Lines 68476 30719 -37757
Branches 7726 3418 -4308
============================================
- Hits 38173 19505 -18668
+ Misses 27826 10202 -17624
+ Partials 2477 1012 -1465
🚀 New features to boost your workflow:
|
88454e5 to
5c85c66
Compare
818d1f6 to
64bdb18
Compare
bce030a to
1b0a869
Compare
There was a problem hiding this comment.
Code Review
This pull request updates the collation mapping classes to use String instead of Character to support multi-byte characters (such as 4-byte UTF-8 emojis) and adds corresponding integration tests for MySQL and PostgreSQL. The review feedback highlights a critical bug where reference equality (== and !=) is used with BigInteger.ZERO instead of .equals(), which can lead to infinite loops during deserialization. Additionally, suggestions are provided to simplify padding calculations and use Java 8 streams for more idiomatic code point extraction.
| String c = getCharacterFromPosition(element.longValue(), true); | ||
| return c; |
There was a problem hiding this comment.
Using reference equality (== or !=) with BigInteger.ZERO is highly dangerous in Java, especially in a distributed environment like Apache Beam where objects are serialized and deserialized across workers. Deserialized BigInteger instances representing 0 will not share the same reference as BigInteger.ZERO, which can lead to infinite loops in the while (element != BigInteger.ZERO) block or incorrect execution of the if (element == BigInteger.ZERO) block.
Please update the comparisons to use .equals(BigInteger.ZERO) instead:
if (element.equals(BigInteger.ZERO)) {
...
}
while (!element.equals(BigInteger.ZERO)) {
...
}
| java.util.List<String> codePoints = new java.util.ArrayList<>(); | ||
| for (int i = 0; i < element.length(); ) { | ||
| int cp = element.codePointAt(i); | ||
| codePoints.add(new String(Character.toChars(cp))); | ||
| i += Character.charCount(cp); | ||
| } |
There was a problem hiding this comment.
Using Java 8+ Streams to iterate over code points is more idiomatic, concise, and less error-prone than manual index manipulation with codePointAt and charCount.
java.util.List<String> codePoints =
element
.codePoints()
.mapToObj(cp -> new String(Character.toChars(cp)))
.collect(Collectors.toList());| for (int index = codePoints.size(); index < lengthToPad; index++) { | ||
| ret = ret.multiply(BigInteger.valueOf(getCharsetSize(index == (codePoints.size() - 1)))); | ||
| } |
There was a problem hiding this comment.
The expression index == (codePoints.size() - 1) is always false because index starts at codePoints.size() and only increases. Simplifying this to false improves readability and avoids confusion for future maintainers.
| for (int index = codePoints.size(); index < lengthToPad; index++) { | |
| ret = ret.multiply(BigInteger.valueOf(getCharsetSize(index == (codePoints.size() - 1)))); | |
| } | |
| for (int index = codePoints.size(); index < lengthToPad; index++) { | |
| ret = ret.multiply(BigInteger.valueOf(getCharsetSize(false))); | |
| } |
1 participant
This PR updates the uniform splitter to correctly handle 4-byte string characters (like emojis) in primary keys during SourceDB to Spanner migrations.
Previously, the string mapper used Java
chartypes. Sincecharis limited to 2 bytes, it cannot represent 4-byte characters without surrogate pairs. RefactoredCollationIndex,CollationMapper, andCollationOrderRowto useStringrepresentations instead. This removes the byte length constraints.I also updated the MySQL and PostgreSQL collation order SQL queries to accurately handle these characters in range calculations.
To verify the fixes, I added two new integration tests that copy tables with 4-byte primary keys:
MySQLSourceDbToSpanner4ByteStringPKITPostgreSQLSourceDbToSpanner4ByteStringPKITLogs showing all characters were captured:
MySQL
PG
I increased the DirectRunner test timeout to 15 minutes in
SourceDbToSpannerITBasealthough the tests use DataflowRunner in github actions so that we can test these tests locally using direct runner (the default timeout there was 4 min)