Support correct YYYY-MM-DD formatting for MySQL DATE to Spanner STRING migration#3895
Support correct YYYY-MM-DD formatting for MySQL DATE to Spanner STRING migration#3895sm745052 wants to merge 4 commits into
Conversation
- Update MysqlMappingProvider to use UnifiedMappingProvider.Type.DATE for DATE columns. - Update MysqlJdbcValueMappings to map java.sql.Date to Avro date logical type (days since epoch). - Re-enable date_to_string test case in MySQLDataTypesIT. - Update MysqlMappingProviderTest to reflect the schema change. Co-authored-by: sm745052 <79754058+sm745052@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
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 improves the migration process from MySQL to Spanner by correcting how DATE columns are handled. By switching the Avro logical type mapping, the system now correctly preserves the YYYY-MM-DD format, preventing the generation of unexpected ISO timestamp strings during the migration process. 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 MySQL mapping for the DATE type from TIMESTAMP (represented as microseconds since epoch in a long) to DATE (represented as days since epoch in an int). This change is reflected in the value mapper, the type mapping provider, and the corresponding unit and integration tests. A review comment points out that using value.toLocalDate() to calculate the epoch day is timezone-dependent and could cause a timezone shift depending on the JVM's default timezone. It suggests calculating the epoch day directly from the UTC milliseconds of the java.sql.Date object using TimeUnit.MILLISECONDS.toDays(value.getTime()).
| private static final ResultSetValueMapper<java.sql.Date> sqlDateToAvroDate = | ||
| (value, schema) -> (int) value.toLocalDate().toEpochDay(); |
There was a problem hiding this comment.
Using value.toLocalDate() is timezone-dependent because java.sql.Date.toLocalDate() uses the JVM's default system timezone to interpret the date. Since the date was extracted in UTC using utcDateExtractor, this can cause a timezone shift (e.g., shifting the date by -1 day if the JVM is running in a timezone behind UTC like EST/PST).
To ensure timezone-independent and correct conversion, we should calculate the epoch day directly from the UTC milliseconds of the java.sql.Date object using TimeUnit.MILLISECONDS.toDays(value.getTime()).
| private static final ResultSetValueMapper<java.sql.Date> sqlDateToAvroDate = | |
| (value, schema) -> (int) value.toLocalDate().toEpochDay(); | |
| private static final ResultSetValueMapper<java.sql.Date> sqlDateToAvroDate = | |
| (value, schema) -> (int) TimeUnit.MILLISECONDS.toDays(value.getTime()); |
|
@jules fix the failing tests |
I have implemented the fix for MySQL DATE formatting and re-enabled the |
- Update MysqlMappingProvider to use UnifiedMappingProvider.Type.DATE for DATE columns. - Update MysqlJdbcValueMappings to map java.sql.Date to Avro date logical type (days since epoch). - Re-enable date_to_string test case in MySQLDataTypesIT. - Update MysqlMappingProviderTest to reflect the schema change. Co-authored-by: sm745052 <79754058+sm745052@users.noreply.github.com>
|
@jules fix the spotless issue |
I have fixed the spotless formatting issues by running |
- Update MysqlMappingProvider to use UnifiedMappingProvider.Type.DATE for DATE columns. - Update MysqlJdbcValueMappings to map java.sql.Date to Avro date logical type (days since epoch). - Re-enable date_to_string test case in MySQLDataTypesIT. - Update MysqlMappingProviderTest to reflect the schema change. - Run spotless:apply to fix formatting issues. Co-authored-by: sm745052 <79754058+sm745052@users.noreply.github.com>
|
@jules now the unit tests are failing |
I've fixed the unit test failure in |
- Update MysqlMappingProvider to use UnifiedMappingProvider.Type.DATE for DATE columns. - Update MysqlJdbcValueMappings to map java.sql.Date to Avro date logical type (days since epoch). - Re-enable date_to_string test case in MySQLDataTypesIT. - Update MysqlMappingProviderTest and JdbcSourceRowMapperTest to reflect the schema change. - Fix formatting with spotless:apply. Co-authored-by: sm745052 <79754058+sm745052@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3895 +/- ##
============================================
+ Coverage 55.51% 63.28% +7.76%
+ Complexity 7026 2315 -4711
============================================
Files 1103 514 -589
Lines 67614 29949 -37665
Branches 7587 3300 -4287
============================================
- Hits 37537 18952 -18585
+ Misses 27662 10034 -17628
+ Partials 2415 963 -1452
🚀 New features to boost your workflow:
|
|
@manitgupta @VardhanThigle @darshan-sj could one of you please take a look? |
2 participants
This change updates the mapping of MySQL DATE columns to use the Avro date logical type (days since epoch) instead of timestamp-micros. This ensures that when migrating to Spanner GoogleSQL STRING columns, the values are correctly formatted as YYYY-MM-DD instead of full ISO timestamps. The date_to_string test case in MySQLDataTypesIT has also been re-enabled.
Fixes #3893
PR created automatically by Jules for task 15180501333780523455 started by @sm745052