Repository navigation
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request implements the GetChars method in SpannerDataReader to support reading string data into character arrays, along with corresponding unit tests. A review comment suggests simplifying and correcting the buffer range validation checks to prevent misleading exception messages and improve clarity.
| GaxPreconditions.CheckArgumentRange(bufferOffset, nameof(bufferOffset), 0, buffer.Length); | ||
| GaxPreconditions.CheckArgumentRange(length, nameof(length), 0, buffer.Length); | ||
| GaxPreconditions.CheckArgumentRange(bufferOffset + length, nameof(length), 0, buffer.Length); |
There was a problem hiding this comment.
The validation checks for length and bufferOffset + length can be simplified and made more precise. Currently, checking bufferOffset + length using nameof(length) as the parameter name can produce misleading exception messages (e.g., showing the sum of offset and length as the invalid 'length' value). By checking length against buffer.Length - bufferOffset, we can ensure that the requested length fits within the remaining buffer space. This mathematically guarantees that bufferOffset + length <= buffer.Length without needing a third check, and provides a much clearer error message if the validation fails.
GaxPreconditions.CheckArgumentRange(bufferOffset, nameof(bufferOffset), 0, buffer.Length);
GaxPreconditions.CheckArgumentRange(length, nameof(length), 0, buffer.Length - bufferOffset);1528406 to
f1545c7
Compare
Summary
Implement ranged character reads for Spanner
STRINGvalues, with length queries when the buffer is null. Reject null and non-STRING values withInvalidCastException, and add tests for ranged reads and these cases.Resolves #16042.
Microsoft contract
Microsoft documents
DbDataReader.GetCharsas reading a requested character range from a column into a buffer: https://learn.microsoft.com/dotnet/api/system.data.common.dbdatareader.getchars. ItsDataTableReaderimplementation documentsInvalidCastExceptionwhen the column does not contain character data and accepts nullable buffers: https://learn.microsoft.com/dotnet/api/system.data.datatablereader.getcharsCompatibility
This replaces
NotSupportedExceptionfor SpannerSTRINGvalues with the documented read behavior. Other value types and SQL NULL values reportInvalidCastException; callers that depended on the previous unsupported exception for strings will observe the new supported result.Validation
dotnet test apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/Google.Cloud.Spanner.Data.Tests.csproj --no-restore -f net8.0 -p:NoWarn=CS8981%3BxUnit2004%3BxUnit2013— 2,834 passed, 0 failed.Discovery
Found using pengdows.AdoNetApiTest.conformance.