Skip to content

Added support for Tokenlist columns#2144

Merged
darshan-sj merged 1 commit intoGoogleCloudPlatform:mainfrom
n-d-joshi:tokenlist-changes
Jan 27, 2025
Merged

Added support for Tokenlist columns#2144
darshan-sj merged 1 commit intoGoogleCloudPlatform:mainfrom
n-d-joshi:tokenlist-changes

Conversation

@n-d-joshi
Copy link
Contributor

This PR supports the following for PG dialect:

  1. Tokenlist type for columns.
  2. Hidden attribute for columns.

Did manual testing by creating a test database and running the Cloud_Spanner_to_GCS_Avro template to export and then imported successfully into a new database using a locally built GCS_Avro_to_Cloud_Spanner. The imported database had the TOKENLIST columns along with Hidden attributes.

@n-d-joshi n-d-joshi requested a review from a team as a code owner January 23, 2025 03:01
@n-d-joshi
Copy link
Contributor Author

R: @atask-g

@codecov
Copy link

codecov bot commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 46.75%. Comparing base (5cda88d) to head (800a0ad).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
...oogle/cloud/teleport/spanner/common/SizedType.java 66.66% 0 Missing and 1 partial ⚠️
.../apache/beam/sdk/io/gcp/spanner/SpannerSchema.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2144      +/-   ##
============================================
+ Coverage     46.72%   46.75%   +0.03%     
- Complexity     3978     3980       +2     
============================================
  Files           872      873       +1     
  Lines         51687    51879     +192     
  Branches       5402     5430      +28     
============================================
+ Hits          24151    24257     +106     
- Misses        25818    25897      +79     
- Partials       1718     1725       +7     
Components Coverage Δ
spanner-templates 68.65% <87.50%> (-0.14%) ⬇️
spanner-import-export 65.65% <87.50%> (+0.05%) ⬆️
spanner-live-forward-migration 76.49% <ø> (-1.02%) ⬇️
spanner-live-reverse-replication 78.39% <ø> (ø)
spanner-bulk-migration 87.87% <ø> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
...oud/teleport/spanner/DdlToAvroSchemaConverter.java 93.03% <ø> (ø)
...com/google/cloud/teleport/spanner/common/Type.java 90.32% <100.00%> (+0.13%) ⬆️
.../com/google/cloud/teleport/spanner/ddl/Column.java 84.11% <100.00%> (+0.93%) ⬆️
...oogle/cloud/teleport/spanner/common/SizedType.java 91.84% <66.66%> (+0.13%) ⬆️
.../apache/beam/sdk/io/gcp/spanner/SpannerSchema.java 66.05% <50.00%> (+0.63%) ⬆️

... and 17 files with indirect coverage changes

@darshan-sj darshan-sj added the improvement Making existing code better label Jan 23, 2025
@atask-g
Copy link
Contributor

atask-g commented Jan 23, 2025

Looks great! Can you also also add a pg specific test similar to searchIndexes() to InformationSchemaScannerIT.java? Also, consider adding a test to DdlTest.java similar to testSearchIndex() which should improve the code coverage stats.

@n-d-joshi
Copy link
Contributor Author

@atask-g, as discussed offline - will follow up with these tests in the next PR once the rest of the changes are also done so it will help to get comprehensive coverage. Thanks for the review.

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.

Looks good.

You will need to raise a PR in apache/beam also similar to this apache/beam#32038

@darshan-sj darshan-sj merged commit d3074fc into GoogleCloudPlatform:main Jan 27, 2025
@Abacn
Copy link
Contributor

Abacn commented Feb 14, 2025

Please sync the change to Beam's SpannerIO : https://github.com/apache/beam/blob/master/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerSchema.java

LocalSpannerIO does not admit changes that not already in Beam's SpannerIO, and is going to be teardown

#1429 will affect part of the functionality added in this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Making existing code better size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants