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
|
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(); | ||
| } |
| 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(); | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4010 +/- ##
============================================
+ Coverage 55.69% 63.48% +7.78%
+ Complexity 6772 2498 -4274
============================================
Files 1125 536 -589
Lines 68437 30710 -37727
Branches 7719 3418 -4301
============================================
- Hits 38118 19497 -18621
+ Misses 27843 10200 -17643
+ Partials 2476 1013 -1463
🚀 New features to boost your workflow:
|
88454e5 to
5c85c66
Compare
No description provided.