Add support for spanner table options in import/export#3552
Add support for spanner table options in import/export#3552n-d-joshi wants to merge 5 commits intoGoogleCloudPlatform:mainfrom
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 significantly enhances the Spanner import/export functionality by enabling the handling of table options. This allows for a more complete and accurate representation of Spanner table definitions during data migration and schema synchronization processes, ensuring that critical table-level configurations are preserved. 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 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 counter productive. 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
|
|
R: @darshan-sj |
darshan-sj
left a comment
There was a problem hiding this comment.
The changes look good overall, I have left a few comments.
Please address these as well:
- Please build the Import and export template with your code changes and run an export from Spanner database with your table option. Import the exported data into a new and empty Spanner database and make sure that the table option is created properly in the new database. Please document the results of the test here.
- Please add integration test cases in ExportPipelineIT and ImportPipelineIT.
| String quoteChar = | ||
| dialect == Dialect.POSTGRESQL ? POSTGRESQL_LITERAL_QUOTE : GSQL_LITERAL_QUOTE; | ||
| options.add( | ||
| optionName + "=" + quoteChar + OPTION_STRING_ESCAPER.escape(optionValue) + quoteChar); |
There was a problem hiding this comment.
Why is NameUtils' quoteIdentifier or quoteLiteral not used here? - https://github.com/GoogleCloudPlatform/DataflowTemplates/blob/main/v1/src/main/java/com/google/cloud/teleport/spanner/common/NameUtils.java
There was a problem hiding this comment.
Done. Changed it to NameUtils.
| @VisibleForTesting | ||
| Statement listTableOptionsSQL() { | ||
| switch (dialect) { | ||
| case GOOGLE_STANDARD_SQL: |
There was a problem hiding this comment.
Are all these columns and tables available in every production databases? Please confirm.
Please get these 2 queries reviewed and LGTMed by one of your team members.
There was a problem hiding this comment.
Yes this should be available in all database as long as we expose the table_options table.
| + " ) PRIMARY KEY (`id` ASC)")); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
I see some escaping is being done for the Table options in InformationSchemaScanner class. Please add test cases where escaping is exercised and escaping is not exercised.
There was a problem hiding this comment.
I am following the same convention as the other tests in creating the table:
"CREATE TABLE `CustomDictionary` ("
+ " `Key` STRING(MAX) NOT NULL,"
+ " `Value` ARRAY<STRING(MAX)> NOT NULL,"
+ " ) PRIMARY KEY (`Key` ASC),\nOPTIONS (fulltext_dictionary_table=true)"));
This is not escaped?
Did I misunderstand the comment?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3552 +/- ##
============================================
- Coverage 52.19% 52.18% -0.02%
+ Complexity 6048 5649 -399
============================================
Files 1040 1041 +1
Lines 62985 63077 +92
Branches 6901 6917 +16
============================================
+ Hits 32877 32917 +40
- Misses 27882 27927 +45
- Partials 2226 2233 +7
🚀 New features to boost your workflow:
|
| options.add( | ||
| optionName + "=" + quoteChar + OPTION_STRING_ESCAPER.escape(optionValue) + quoteChar); | ||
| } else if (optionType.equalsIgnoreCase("character varying")) { | ||
| options.add(optionName + "='" + OPTION_STRING_ESCAPER.escape(optionValue) + "'"); |
There was a problem hiding this comment.
I'm referring to this option_string_escaper string in the comment in AvroSchemaToDdlConverterTest.java . Does the test case added there test different edge cases around this escaping?
This PR adds: