Skip to content

Fix SpannerIO PerformanceTests workflows - #40683

Merged
Amar3tto merged 1 commit into
masterfrom
fix-spanner-flatmap
Oct 21, 2026
Merged

Amar3tto merged 1 commit into
masterfrom
fix-spanner-flatmap

Conversation

@Amar3tto

Copy link
Copy Markdown
Collaborator

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📋 Review Summary

This PR fixes type checking errors in the SpannerIO Python performance tests (spannerio_read_perf_test.py and spannerio_write_perf_test.py) by explicitly specifying .with_output_types(_Mutator) on the FlatMap(make_insert_mutations) transform.

🔍 General Feedback

  • The changes are clean, minimal, and directly resolve the type inference mismatch between FlatMap and _MakeMutationGroupsFn (which expects Union[MutationGroup, _Mutator]).
  • Both read and write performance test workflows have been verified with passing runs.

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

| 'Format' >> Map(format_record)
| 'Make mutations' >> FlatMap(make_insert_mutations)
| 'Make mutations' >> FlatMap(
make_insert_mutations).with_output_types(_Mutator)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we know what change caused this to start failing? Did we make a breaking change to something?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I found a possible trigger: #40413, merged on October 8, switched Python load/performance tests from 3.10 to 3.11. This may have exposed the existing FlatMap type inference issue. I haven't confirmed it yet, but I didn't find any changes to the SpannerIO API or type checking itself.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok - could we try to run before/after that change and see if it shows the issue? If yes, then I am fine with the change. If no, we should really understand why we need this first

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@github-actions

Copy link
Copy Markdown
Contributor

Assigning reviewers:

R: @tvalentyn for label python.
Backup reviewer: shunping (if the main reviewer is unresponsive, you can escalate to them by tagging their username)

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@Amar3tto
Amar3tto merged commit 5b6aa70 into master Oct 21, 2026
90 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.

The PerformanceTests SpannerIO Read 2GB Python job is flaky The PerformanceTests SpannerIO Write 2GB Python Batch job is flaky

4 participants