fix(spanner): ResultSet::merge panic for chunked nested ListValues#439
Merged
yoshidan merged 4 commits intoMay 15, 2026
Merged
Conversation
) `ResultSet::merge` previously recursed unconditionally on the inner-last/inner-first pair when merging two `ListValues`, causing `Internal: previous_last kind mismatch` when the chunk boundary fell between complete elements whose kinds are not chunkable (Null, Bool, Number). Mirrors the Go SDK (isMergeable/merge) and the official Rust SDK (merge_values): only recurse when both sides are StringValue or ListValue, otherwise push both elements and concatenate. Also adds short-circuit handling for empty inner lists. Closes yoshidan#438 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes ResultSet::merge behavior when Spanner streams chunked nested ListValues (e.g., ARRAY<STRUCT<...>>) across multiple PartialResultSets, avoiding erroneous kind-mismatch failures by only recursively merging truly chunk-split elements and correctly handling empty inner lists.
Changes:
- Add a mergeability gate for
ListValuemerges: recurse only for(StringValue, StringValue)and(ListValue, ListValue)boundaries; otherwise concatenate. - Add short-circuit handling when either side’s inner list is empty.
- Add regression tests covering non-mergeable boundaries and empty-list edge cases.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+216
to
+217
| let first_value_of_current = first.values.remove(0); | ||
| let merged = match last.values.pop() { | ||
| Some(last_value_of_previous) => { | ||
| ResultSet::merge(last_value_of_previous, first_value_of_current)? | ||
| } | ||
| // last record can be empty | ||
| None => first_value_of_current, | ||
| }; | ||
| last.values.push(merged); | ||
| let last_value_of_previous = last.values.pop().unwrap(); |
| // mismatch ...`. After the fix it must concatenate without merging. | ||
| #[test] | ||
| fn test_rs_merge_list_value_with_non_mergeable_tail() { | ||
| let null = || Value { kind: Some(Kind::NullValue(0)) }; |
- Replace `first.values.remove(0)` + `extend(first.values)` with `into_iter()` to avoid the O(n) shift and reduce to a single pass. - Use `prost_types::NullValue::NullValue.into()` instead of the magic literal `0` in the test helper, consistent with statement.rs:263. - Remove now-unnecessary `mut` binding on `first`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixes CI fmt.sh failures caused by formatting drift in the merge function and test helpers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
Hi @yoshidan, I have just pushed the two new commits where I considered and applied the Copilot suggestions. I also format the file to avoid failed CI. Can you enable the CI again and review my PR? Thanks a lot. |
Owner
|
LGTM! |
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.
Problem
When
ExecuteStreamingSqlstreams a large nestedARRAY<STRUCT<...>>value split across multiplePartialResultSets withchunked_value=true,ResultSet::mergefails with:The root cause is in
spanner/src/reader.rs: when merging twoListValues, the code unconditionally popped the last element of the previous chunk and recursed intoResultSet::mergewith the first element of the current chunk. If the boundary fell between two complete elements whose kinds are not chunkable (NullValue,BoolValue,NumberValue), the recursive call hit the catch-all arm and returned an error.Additionally, the code did not handle the case where either inner list was empty.
Fix
Mirrors the reference SDKs:
isMergeable+mergemerge_valuesChanges:
StringValueorListValue. Otherwise push both elements as-is and concatenate the remaining values.Impact
Any caller streaming
ARRAY<STRUCT<...>>columns large enough to trigger chunking can hit this. Spanner change-stream readers (SELECT ChangeRecord FROM READ_<stream>(...)) are particularly exposed because the column type mixes strings, lists, bools, nulls, and numerics.