Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues for sequences and identity columns #2171

Merged

Conversation

hengfengli
Copy link
Contributor

@hengfengli hengfengli commented Feb 10, 2025

Fix a few issues related to sequences and identity columns:

  • null strings appear in identity columns. If a parameter is null, it should not appear in the output file.
  • A conversion is required from BIT_REVERSED_POSITIVE_SEQUENCE to bit_reversed_positive because the identity_kind in the information_schema table has a value BIT_REVERSED_POSITIVE_SEQUENCE which is different than bit_reversed_positive.
  • sequence_kind="default" is not shown properly for the statement CREATE SEQUENCE myseq.

@hengfengli hengfengli requested a review from a team as a code owner February 10, 2025 02:35
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.06%. Comparing base (c0cadc4) to head (beb1571).
Report is 16 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2171      +/-   ##
============================================
+ Coverage     46.95%   47.06%   +0.10%     
- Complexity     4044     4053       +9     
============================================
  Files           876      876              
  Lines         52190    52226      +36     
  Branches       5501     5508       +7     
============================================
+ Hits          24507    24580      +73     
+ Misses        25924    25887      -37     
  Partials       1759     1759              
Components Coverage Δ
spanner-templates 68.92% <100.00%> (+0.04%) ⬆️
spanner-import-export 65.75% <100.00%> (+0.05%) ⬆️
spanner-live-forward-migration 76.48% <ø> (-0.03%) ⬇️
spanner-live-reverse-replication 78.80% <ø> (+0.12%) ⬆️
spanner-bulk-migration 87.94% <ø> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
...oud/teleport/spanner/DdlToAvroSchemaConverter.java 93.12% <100.00%> (+0.07%) ⬆️

... and 14 files with indirect coverage changes

manitgupta
manitgupta previously approved these changes Feb 10, 2025
Copy link
Member

@manitgupta manitgupta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@darshan-sj Do we also test this functionality in an IT? Do we need to update a corresponding IT as well?

Copy link
Contributor

@darshan-sj darshan-sj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check ExportPipelineIT and ImportPipelineIT and please add a test case if it is possible to add for this edgecase.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 12, 2025
Copy link
Contributor

@darshan-sj darshan-sj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@darshan-sj darshan-sj merged commit dba7112 into GoogleCloudPlatform:main Feb 20, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants