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 introduces case-insensitive fallback logic for retrieving column ordinals in SpannerDataReader, along with comprehensive unit tests. The review feedback recommends optimizing the fallback search to prevent potential KeyNotFoundException errors and redundant dictionary lookups, while also adding a null check for the column name.
7d361f2 to
5e2fb19
Compare
|
@amanda-tarafa Thanks. I agree compatibility matters, and I don’t expect every conformance finding to justify a behavior change. You’re right that a lot of ADO.NET is underspecified, so I’d like to separate my PRs along that line. #16041 / #16044 (GetOrdinal) is documented at the interface level, not on SqlClient. The remarks on IDataRecord.GetOrdinal describe the lookup: A case-sensitive match first. This works purely against result-set metadata the reader already has, so it doesn’t depend on any Spanner capability. Other mature providers have accepted fixes like this, including Microsoft’s own. I’ve sent the same kind of contract fixes, each citing the learn.microsoft.com sentence involved, to a number of providers: Microsoft merged 7 across Microsoft.Data.Sqlite and Microsoft.Data.SqlClient. That includes a GetOrdinal fix that changed the exception type callers catch (dotnet/efcore#39353), which is a more breaking change than this PR. None has been declined on the grounds that the documented behavior isn’t the contract. On compatibility risk for #16044: Every exact-case lookup that succeeds today behaves the same after this change. I wrote the conformance suite specifically because the contract is clear, and so many providers do NOT implement it correctly. DuckDB.NET and npgsql are the 2 ado.net providers that are the closest, other than my own pengdows.flatfile. |
Summary
Implement the documented
IDataRecord.GetOrdinallookup behavior inSpannerDataReader: exact-case match first, then case-insensitive fallback, andIndexOutOfRangeExceptionwhen no field matches. Add sync and async regression coverage, including exact-match precedence.Resolves #16041.
Microsoft contract
Microsoft documents the case-sensitive then case-insensitive lookup sequence and
IndexOutOfRangeExceptionfor missing names: https://learn.microsoft.com/dotnet/api/system.data.idatarecord.getordinalCompatibility
This allows case variants that currently fail to resolve and leaves exact matches unchanged. The only observable change is that callers relying on an exception for a valid case-insensitive match will now receive its ordinal; exact-case precedence preserves deterministic behavior for names that differ only by case.
Validation
dotnet test apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/Google.Cloud.Spanner.Data.Tests.csproj -f net8.0 -p:NoWarn=CS8981%3BxUnit2004%3BxUnit2013— 2,835 passed, 0 failed.Discovery
Found using pengdows.AdoNetApiTest.conformance.