Repository navigation
Conversation
Change-Id: I5c94a4448cc24ee80e283f8ca93f21b8b35ccccd
Contributor
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in the chunk_stream method of the ReadRows operation where current_key was incorrectly reset to None at the start of each ReadRowsResponse. By moving the initialization of current_key outside of the response stream loop, the code now correctly handles rows that span across multiple responses when continuation chunks omit the row key. The changes are reflected in both the asynchronous implementation and the autogenerated synchronous counterpart, accompanied by new unit tests to verify the fix. I have no further feedback to provide as the changes are clean and well-tested.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
current_keywas reset toNoneat the start of eachReadRowsResponseinsidechunk_stream. When a single row spans two response messages, the continuation chunk in the second response carries norow_key(protobuf bytes defaultb""), socurrent_keywas set tob""instead of the actual row key.commit_row,self._last_yielded_row_key = current_keywroteb"", corrupting the retry cursor. On any subsequent retryable error,_revise_request_rowsetusedb""aslast_seen_row_key, causing the already-committed row to be re-requested and yielded again.if current_key is None→if not current_key: protobuf bytes fields default tob"", notNone, so the old check never fired.Fix: move
current_key = Noneto before the outerasync for resploop so it persists across response boundaries. The sync autogen file is regenerated from the async source.Test plan
test_chunk_stream_cross_response_row_key(async + autogen sync): constructs a two-response stream where the first response opens a row and the second commits it, then asserts_last_yielded_row_keyequals the actual row key rather thanb"".pytest tests/unit/data/_async/test__read_rows.py tests/unit/data/_sync_autogen/test__read_rows.py tests/unit/data/_async/test_read_rows_acceptance.py tests/unit/data/_sync_autogen/test_read_rows_acceptance.py).