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 4 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
| @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.
| if (!tableOptions().isEmpty()) { | ||
| appendable.append("\nWITH (").append(String.join(", ", tableOptions())).append(")"); | ||
| } |
There was a problem hiding this comment.
Is the relative positioning of "with" clause correct in this CREATE TABLE statement?
Also, please add a test case in DdlTest.java covering all the clauses here including Table options.
| if (!tableOptions().isEmpty()) { | ||
| appendable.append(",\nOPTIONS (").append(String.join(", ", tableOptions())).append(")"); | ||
| } |
There was a problem hiding this comment.
Same comment as PG. I hope the relative ordering of OPTIONS clause is correct here. And Please add test case in DdlTest.java covering all the clauses currently present here.
| public static final String SPANNER_PARENT = "spannerParent"; | ||
| public static final String SPANNER_INTERLEAVE_TYPE = "spannerInterleaveType"; | ||
| public static final String SPANNER_PRIMARY_KEY = "spannerPrimaryKey"; | ||
| public static final String SPANNER_TABLE_OPTIONS = "spannerTableOptions_"; |
There was a problem hiding this comment.
Other numbered properties are singular in this file.
Please change to Singular
SPANNER_TABLE_OPTION = "spannerTableOption_"
| + " ) 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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3552 +/- ##
============================================
+ Coverage 52.19% 52.22% +0.02%
+ Complexity 6048 5644 -404
============================================
Files 1040 1041 +1
Lines 62985 62995 +10
Branches 6901 6904 +3
============================================
+ Hits 32877 32897 +20
+ Misses 27882 27874 -8
+ Partials 2226 2224 -2
🚀 New features to boost your workflow:
|
This PR adds: