[yaml] Adding Spanner IO Providers for Beam YAML by reeba212 · Pull Request #31987 · apache/beam

reeba212 · 2024-07-25T16:09:11Z

Added SpannerReadSchemaTransformProvider.java, a provider for reading from spanner.
Modified SpannerWriteSchemaTransformProvider.java to incorporate error handling and fixed other issues.
Modified MutationUtils.java to add a function "createRowFromMutation" which creates a Beam Row from a Mutation. This is used in the spanner write provider.
Modified ErrorHandling.java to make it serializable.
Modified standard_io.yaml to register the Spanner IO providers.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

github-actions · 2024-07-25T17:34:49Z

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

Polber

Thanks Reeba! I left some comments, but the overall structure looks really good

...io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/MutationUtils.java

...orm/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerReadSchemaTransformProvider.java

...rm/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerWriteSchemaTransformProvider.java

sdks/python/apache_beam/yaml/standard_io.yaml

Polber · 2024-07-25T19:01:34Z

R: @damccorm

github-actions · 2024-07-25T19:04:39Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

Polber

Added some comments related to newly added testing framework

.../java/core/src/main/java/org/apache/beam/sdk/schemas/transforms/providers/ErrorHandling.java

...io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/MutationUtils.java

...orm/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerReadSchemaTransformProvider.java

sdks/python/apache_beam/yaml/integration_tests.py

sdks/python/apache_beam/io/gcp/spanner_wrapper.py

Polber

Couple more small changes. Also, make sure to address the unresolved conversations. Thanks!

...orm/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerReadSchemaTransformProvider.java

sdks/python/apache_beam/yaml/integration_tests.py

Polber · 2024-08-27T16:18:51Z

@reeba212 #32008 was merged, so you should be able to rebase on main and make sure checks pass!

Polber

Lint errors:

************* Module apache_beam.io.gcp.spanner_wrapper	
apache_beam/io/gcp/spanner_wrapper.py:20:0: W0611: Unused apache_beam imported as beam (unused-import)	
************* Module apache_beam.yaml.integration_tests	
apache_beam/yaml/integration_tests.py:50:0: W0404: Reimport 'contextlib' (imported line 20) (reimported)	
apache_beam/yaml/integration_tests.py:50:0: C0413: Import "import contextlib" should be placed at the top of the module (wrong-import-position)	
apache_beam/yaml/integration_tests.py:51:0: W0404: Reimport 'uuid' (imported line 27) (reimported)	
apache_beam/yaml/integration_tests.py:51:0: C0413: Import "import uuid" should be placed at the top of the module (wrong-import-position)	
apache_beam/yaml/integration_tests.py:52:0: W0404: Reimport 'logging' (imported line 24) (reimported)	
apache_beam/yaml/integration_tests.py:52:0: C0413: Import "import logging" should be placed at the top of the module (wrong-import-position)	
apache_beam/yaml/integration_tests.py:53:0: C0413: Import "from google.cloud import spanner" should be placed at the top of the module (wrong-import-position)	
apache_beam/yaml/integration_tests.py:50:0: C0412: Imports from package contextlib are not grouped (ungrouped-imports)	
apache_beam/yaml/integration_tests.py:51:0: C0412: Imports from package uuid are not grouped (ungrouped-imports)	
apache_beam/yaml/integration_tests.py:52:0: C0412: Imports from package logging are not grouped (ungrouped-imports)	
apache_beam/yaml/integration_tests.py:53:0: W0611: Unused spanner imported from google.cloud (unused-import)

Make modification I added in comments then run:
yapf --in-place --parallel --recursive apache_beam

I also recommend following steps in https://cwiki.apache.org/confluence/display/BEAM/Python+Tips#PythonTips-LintandFormattingChecks

sdks/python/apache_beam/io/gcp/spanner_wrapper.py

sdks/python/apache_beam/yaml/integration_tests.py

codecov · 2024-08-30T19:32:05Z

Codecov Report

Attention: Patch coverage is 0% with 48 lines in your changes missing coverage. Please review.

Project coverage is 57.15%. Comparing base (64b4aa6) to head (8f9178e).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
sdks/python/apache_beam/io/gcp/spanner_wrapper.py	0.00%	34 Missing ⚠️
sdks/python/apache_beam/yaml/integration_tests.py	0.00%	14 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #31987      +/-   ##
============================================
- Coverage     57.34%   57.15%   -0.19%     
  Complexity     1474     1474              
============================================
  Files           964      965       +1     
  Lines        153341   153435      +94     
  Branches       1076     1076              
============================================
- Hits          87933    87699     -234     
- Misses        63204    63537     +333     
+ Partials       2204     2199       -5

Flag	Coverage Δ
python	`81.15% <0.00%> (-0.45%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Polber

Try seeing if making these changes fixes test errors

...orm/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerReadSchemaTransformProvider.java

...rm/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerWriteSchemaTransformProvider.java

sdks/python/apache_beam/io/gcp/spanner_wrapper_test.py

1. Removed serialiazability from ErrorHandling.java 2. Removed double map definitions from MutationUtils.java 3. Added checkNotNull in spanner write provider 4. Modified some variables in spanner wrapper 5. Change instance id in integration tests

import retry

1. replace checknotnull with checkargument in spanner read provider 2. use the correct table name (tmp_table) in integration test

1. Added serializable to error handling 2. Corrected validation methods in spanner read 3. Added retry import and removed default project name in spanner wrapper 4. Corrected instance and database names in spanner integration test 5. Corrected table name in query

damccorm

Reran the failing jobs to see if we can get them green, but they do seem unrelated to this change

damccorm · 2024-09-05T16:11:07Z

sdks/python/setup.py

              'google-cloud-core>=2.0.0,<3',
              'google-cloud-bigtable>=2.19.0,<3',
-              'google-cloud-spanner>=3.0.0,<4',
+              'google-cloud-spanner>=3.0.0,<3.48',


@Polber @reeba212 catching up on this, which suites were failing such that we needed to add this? I'd like to follow up with the spanner folks to make sure they're aware of the breaking change

There were two checks that were failing and both had the same underlying error:
ModuleNotFoundError: No module named 'grpc_interceptor'

Polber

Thanks!

damccorm

Thanks!

chamikaramj · 2024-09-12T17:20:19Z

Seems like the google-cloud-spanner version specified here conflicts with the version installed in the containers.

https://github.com/apache/beam/blob/master/sdks/python/setup.py#L444
https://github.com/apache/beam/blob/master/sdks/python/container/py38/base_image_requirements.txt#L72

At least one test suite is failing due to this.
https://ge.apache.org/s/2eh6tfikgiwb2/console-log/task/:sdks:python:expansion-service-container:docker?anchor=258&page=1

cc: @tvalentyn

liferoad · 2024-09-12T17:33:27Z

cc @Polber

Polber · 2024-09-12T17:39:11Z

@damccorm was working on rebuilding those - not sure the status

Polber · 2024-09-12T17:39:56Z

Looks like it is under review still: #32411

damccorm · 2024-09-12T17:41:39Z

Yep, was just waiting on approval - just merged and it should be back to green

* Add Spanner IO providers to YAML SDK * add handling logic for more datatypes * delete examples * minor changes * minor change * add integration test * add docs * minor change * minor changes 1. Removed serialiazability from ErrorHandling.java 2. Removed double map definitions from MutationUtils.java 3. Added checkNotNull in spanner write provider 4. Modified some variables in spanner wrapper 5. Change instance id in integration tests * Update spanner_wrapper.py import retry * minor changes 1. replace checknotnull with checkargument in spanner read provider 2. use the correct table name (tmp_table) in integration test * minor changes 1. Added serializable to error handling 2. Corrected validation methods in spanner read 3. Added retry import and removed default project name in spanner wrapper 4. Corrected instance and database names in spanner integration test 5. Corrected table name in query * formatting * Update SpannerWriteSchemaTransformProvider.java * correct lint failures * correct lint failures * correct lint failures * Update SpannerReadSchemaTransformProvider.java * correct lint failures * Update SpannerWriteSchemaTransformProvider.java * spanner version update

github-actions bot added python java io gcp spanner yaml labels Jul 25, 2024

Polber reviewed Jul 25, 2024

View reviewed changes

Polber mentioned this pull request Jul 31, 2024

Add schema to SpannerIO read #32008

Merged

3 tasks

reeba212 changed the title ~~Adding Spanner IO Providers for Beam YAML~~ [yaml] Adding Spanner IO Providers for Beam YAML Aug 22, 2024

Polber suggested changes Aug 23, 2024

View reviewed changes

Polber suggested changes Aug 26, 2024

View reviewed changes

...orm/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerReadSchemaTransformProvider.java Outdated Show resolved Hide resolved

sdks/python/apache_beam/yaml/integration_tests.py Outdated Show resolved Hide resolved

Polber reviewed Aug 30, 2024

View reviewed changes

sdks/python/apache_beam/io/gcp/spanner_wrapper.py Outdated Show resolved Hide resolved

sdks/python/apache_beam/yaml/integration_tests.py Outdated Show resolved Hide resolved

Polber suggested changes Aug 30, 2024

View reviewed changes

...orm/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerReadSchemaTransformProvider.java Outdated Show resolved Hide resolved

...orm/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerReadSchemaTransformProvider.java Outdated Show resolved Hide resolved

Polber suggested changes Aug 31, 2024

View reviewed changes

...rm/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerWriteSchemaTransformProvider.java Outdated Show resolved Hide resolved

Polber reviewed Sep 3, 2024

View reviewed changes

sdks/python/apache_beam/io/gcp/spanner_wrapper_test.py Outdated Show resolved Hide resolved

reeba212 added 10 commits September 5, 2024 02:27

Add Spanner IO providers to YAML SDK

d41dfbd

add handling logic for more datatypes

fc3dc1d

delete examples

fafc028

minor changes

4ed10c5

minor change

62fedad

add integration test

fcb061e

add docs

04c3fac

minor change

329870a

minor changes

20c9b57

1. Removed serialiazability from ErrorHandling.java 2. Removed double map definitions from MutationUtils.java 3. Added checkNotNull in spanner write provider 4. Modified some variables in spanner wrapper 5. Change instance id in integration tests

Update spanner_wrapper.py

bd57fbd

import retry

reeba212 added 10 commits September 5, 2024 02:27

minor changes

22fc0b7

1. replace checknotnull with checkargument in spanner read provider 2. use the correct table name (tmp_table) in integration test

minor changes

7937c89

1. Added serializable to error handling 2. Corrected validation methods in spanner read 3. Added retry import and removed default project name in spanner wrapper 4. Corrected instance and database names in spanner integration test 5. Corrected table name in query

formatting

34e3831

Update SpannerWriteSchemaTransformProvider.java

aa5255e

correct lint failures

6cfc1b3

correct lint failures

adfdcde

correct lint failures

a451185

Update SpannerReadSchemaTransformProvider.java

575c5e5

correct lint failures

66ff2de

Update SpannerWriteSchemaTransformProvider.java

8f9178e

reeba212 force-pushed the spanner-io branch from 4566ee1 to 8f9178e Compare September 4, 2024 21:07

spanner version update

1599af1

damccorm reviewed Sep 5, 2024

View reviewed changes

Polber approved these changes Sep 5, 2024

View reviewed changes

damccorm approved these changes Sep 9, 2024

View reviewed changes

damccorm merged commit 173dd48 into apache:master Sep 9, 2024

Conversation

reeba212 commented Jul 25, 2024

GitHub Actions Tests Status (on master branch)

Uh oh!

github-actions bot commented Jul 25, 2024

Uh oh!

Polber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Polber commented Jul 25, 2024

Uh oh!

github-actions bot commented Jul 25, 2024

Uh oh!

Polber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Polber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Polber commented Aug 27, 2024

Uh oh!

Polber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Polber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

damccorm left a comment

Choose a reason for hiding this comment

Uh oh!

damccorm Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

reeba212 Sep 5, 2024

Choose a reason for hiding this comment

Uh oh!

Polber left a comment

Choose a reason for hiding this comment

Uh oh!

damccorm left a comment

Choose a reason for hiding this comment

Uh oh!

chamikaramj commented Sep 12, 2024

Uh oh!

liferoad commented Sep 12, 2024

Uh oh!

Polber commented Sep 12, 2024

Uh oh!

codecov bot commented Aug 30, 2024 •

edited

Loading