From 1b6af2dcefe291c8c5c76151a41b973cc2fbf999 Mon Sep 17 00:00:00 2001 From: tuantran0910 Date: Fri, 8 May 2026 13:46:10 +0700 Subject: [PATCH 1/4] fix(spanner): gate ListValue merging on mergeability check (#438) `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 #438 Co-Authored-By: Claude Sonnet 4.6 --- spanner/src/reader.rs | 128 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 120 insertions(+), 8 deletions(-) diff --git a/spanner/src/reader.rs b/spanner/src/reader.rs index 9694c475..db143b22 100644 --- a/spanner/src/reader.rs +++ b/spanner/src/reader.rs @@ -198,15 +198,38 @@ impl ResultSet { }, Kind::ListValue(mut last) => match current_first.kind.unwrap() { Kind::ListValue(mut first) => { + if first.values.is_empty() { + return Ok(Value { + kind: Some(Kind::ListValue(last)), + }); + } + if last.values.is_empty() { + return Ok(Value { + kind: Some(Kind::ListValue(first)), + }); + } + // Only recurse when the chunk boundary actually splits a + // single value: both the last element of `last` and the + // first element of `first` must be of a chunkable kind + // (StringValue or ListValue). Otherwise the boundary fell + // between complete elements and we just concatenate. + // Mirrors the Go SDK (read.go: isMergeable / merge) and + // the official Rust SDK (result_set.rs: merge_values). 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(); + let mergeable = matches!( + (&last_value_of_previous.kind, &first_value_of_current.kind), + (Some(Kind::StringValue(_)), Some(Kind::StringValue(_))) + | (Some(Kind::ListValue(_)), Some(Kind::ListValue(_))) + ); + if mergeable { + let merged = + ResultSet::merge(last_value_of_previous, first_value_of_current)?; + last.values.push(merged); + } else { + last.values.push(last_value_of_previous); + last.values.push(first_value_of_current); + } last.values.extend(first.values); Ok(Value { kind: Some(Kind::ListValue(last)), @@ -632,6 +655,95 @@ mod tests { } } + // Regression: chunk boundary falls between complete inner elements where + // the last element of the previous list is not a chunkable kind (here + // NullValue). Previously this returned `Internal: previous_last kind + // 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)) }; + let bool_v = |b: bool| Value { kind: Some(Kind::BoolValue(b)) }; + let num_v = |n: f64| Value { kind: Some(Kind::NumberValue(n)) }; + + // previous tail = NullValue, source first = StringValue: must NOT merge. + let previous_last = Value { + kind: Some(Kind::ListValue(prost_types::ListValue { + values: vec![value("a"), null()], + })), + }; + let current_first = Value { + kind: Some(Kind::ListValue(prost_types::ListValue { + values: vec![value("b"), bool_v(true)], + })), + }; + let merged = ResultSet::merge(previous_last, current_first).expect("must concatenate"); + match merged.kind.unwrap() { + Kind::ListValue(v) => { + assert_eq!(v.values.len(), 4); + assert!(matches!(v.values[0].kind, Some(Kind::StringValue(ref s)) if s == "a")); + assert!(matches!(v.values[1].kind, Some(Kind::NullValue(_)))); + assert!(matches!(v.values[2].kind, Some(Kind::StringValue(ref s)) if s == "b")); + assert!(matches!(v.values[3].kind, Some(Kind::BoolValue(true)))); + } + _ => unreachable!("must be list value"), + } + + // previous tail = BoolValue, source first = NumberValue: still ok. + let previous_last = Value { + kind: Some(Kind::ListValue(prost_types::ListValue { + values: vec![bool_v(false)], + })), + }; + let current_first = Value { + kind: Some(Kind::ListValue(prost_types::ListValue { + values: vec![num_v(1.5), value("tail")], + })), + }; + let merged = ResultSet::merge(previous_last, current_first).expect("must concatenate"); + match merged.kind.unwrap() { + Kind::ListValue(v) => { + assert_eq!(v.values.len(), 3); + assert!(matches!(v.values[0].kind, Some(Kind::BoolValue(false)))); + assert!(matches!(v.values[1].kind, Some(Kind::NumberValue(n)) if n == 1.5)); + assert!(matches!(v.values[2].kind, Some(Kind::StringValue(ref s)) if s == "tail")); + } + _ => unreachable!("must be list value"), + } + } + + // Empty list edge cases — a chunk may carry no inner values. + #[test] + fn test_rs_merge_list_value_empty_sides() { + let empty_list = || Value { + kind: Some(Kind::ListValue(prost_types::ListValue { values: vec![] })), + }; + let with_one = || Value { + kind: Some(Kind::ListValue(prost_types::ListValue { + values: vec![value("only")], + })), + }; + + // previous empty + current with one => result is current. + let merged = ResultSet::merge(empty_list(), with_one()).unwrap(); + match merged.kind.unwrap() { + Kind::ListValue(v) => { + assert_eq!(v.values.len(), 1); + assert!(matches!(v.values[0].kind, Some(Kind::StringValue(ref s)) if s == "only")); + } + _ => unreachable!(), + } + + // previous with one + current empty => result is previous. + let merged = ResultSet::merge(with_one(), empty_list()).unwrap(); + match merged.kind.unwrap() { + Kind::ListValue(v) => { + assert_eq!(v.values.len(), 1); + assert!(matches!(v.values[0].kind, Some(Kind::StringValue(ref s)) if s == "only")); + } + _ => unreachable!(), + } + } + #[test] fn test_rs_add_one_column_no_chunked_value() { let mut rs = empty_rs(); From 6810f69d7b04f5780b4980af289d437230731de6 Mon Sep 17 00:00:00 2001 From: tuantran0910 Date: Fri, 8 May 2026 13:51:00 +0700 Subject: [PATCH 2/4] refactor(spanner): remove SDK reference from inline comment Co-Authored-By: Claude Sonnet 4.6 --- spanner/src/reader.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/spanner/src/reader.rs b/spanner/src/reader.rs index db143b22..d542e9c3 100644 --- a/spanner/src/reader.rs +++ b/spanner/src/reader.rs @@ -213,8 +213,6 @@ impl ResultSet { // first element of `first` must be of a chunkable kind // (StringValue or ListValue). Otherwise the boundary fell // between complete elements and we just concatenate. - // Mirrors the Go SDK (read.go: isMergeable / merge) and - // the official Rust SDK (result_set.rs: merge_values). let first_value_of_current = first.values.remove(0); let last_value_of_previous = last.values.pop().unwrap(); let mergeable = matches!( From eea5260f2aaf38e1137b01858bf8f4d2b54145ce Mon Sep 17 00:00:00 2001 From: tuantran0910 Date: Sat, 9 May 2026 22:16:59 +0700 Subject: [PATCH 3/4] refactor(spanner): apply Copilot review suggestions - 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 --- spanner/src/reader.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/spanner/src/reader.rs b/spanner/src/reader.rs index d542e9c3..58a5b959 100644 --- a/spanner/src/reader.rs +++ b/spanner/src/reader.rs @@ -197,7 +197,7 @@ impl ResultSet { )), }, Kind::ListValue(mut last) => match current_first.kind.unwrap() { - Kind::ListValue(mut first) => { + Kind::ListValue(first) => { if first.values.is_empty() { return Ok(Value { kind: Some(Kind::ListValue(last)), @@ -213,7 +213,8 @@ impl ResultSet { // first element of `first` must be of a chunkable kind // (StringValue or ListValue). Otherwise the boundary fell // between complete elements and we just concatenate. - let first_value_of_current = first.values.remove(0); + let mut iter = first.values.into_iter(); + let first_value_of_current = iter.next().unwrap(); let last_value_of_previous = last.values.pop().unwrap(); let mergeable = matches!( (&last_value_of_previous.kind, &first_value_of_current.kind), @@ -228,7 +229,7 @@ impl ResultSet { last.values.push(last_value_of_previous); last.values.push(first_value_of_current); } - last.values.extend(first.values); + last.values.extend(iter); Ok(Value { kind: Some(Kind::ListValue(last)), }) @@ -659,7 +660,7 @@ mod tests { // 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)) }; + let null = || Value { kind: Some(Kind::NullValue(prost_types::NullValue::NullValue.into())) }; let bool_v = |b: bool| Value { kind: Some(Kind::BoolValue(b)) }; let num_v = |n: f64| Value { kind: Some(Kind::NumberValue(n)) }; From eba83476ba37caceeca205898599c09ed655c9e9 Mon Sep 17 00:00:00 2001 From: tuantran0910 Date: Sat, 9 May 2026 22:20:34 +0700 Subject: [PATCH 4/4] style(spanner): apply rustfmt to reader.rs Fixes CI fmt.sh failures caused by formatting drift in the merge function and test helpers. Co-Authored-By: Claude Sonnet 4.6 --- spanner/src/reader.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/spanner/src/reader.rs b/spanner/src/reader.rs index 58a5b959..2f26f940 100644 --- a/spanner/src/reader.rs +++ b/spanner/src/reader.rs @@ -222,8 +222,7 @@ impl ResultSet { | (Some(Kind::ListValue(_)), Some(Kind::ListValue(_))) ); if mergeable { - let merged = - ResultSet::merge(last_value_of_previous, first_value_of_current)?; + let merged = ResultSet::merge(last_value_of_previous, first_value_of_current)?; last.values.push(merged); } else { last.values.push(last_value_of_previous); @@ -660,9 +659,15 @@ mod tests { // 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(prost_types::NullValue::NullValue.into())) }; - let bool_v = |b: bool| Value { kind: Some(Kind::BoolValue(b)) }; - let num_v = |n: f64| Value { kind: Some(Kind::NumberValue(n)) }; + let null = || Value { + kind: Some(Kind::NullValue(prost_types::NullValue::NullValue.into())), + }; + let bool_v = |b: bool| Value { + kind: Some(Kind::BoolValue(b)), + }; + let num_v = |n: f64| Value { + kind: Some(Kind::NumberValue(n)), + }; // previous tail = NullValue, source first = StringValue: must NOT merge. let previous_last = Value {