feat: add support for DataReader.GetBytes #7395

olavloite · 2021-10-26T19:16:18Z

Adds support for DataReader.GetBytes to SpannerDataReader. Although Spanner does not support any
streaming of bytes from a result set, which means that there is no performance gain to be achieved
by implementing this method, it still makes sense to support it as some frameworks (NHibernate)
use the method by default for columns that store binary data.

jskeet

Generally fine, but a couple of questions.

jskeet · 2021-10-27T07:04:11Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerDataReader.cs

@@ -147,8 +148,33 @@ public T GetFieldValue<T>(string columnName)
        public override byte GetByte(int i) => GetFieldValue<byte>(i);

        /// <inheritdoc />
-        public override long GetBytes(int i, long fieldOffset, byte[] buffer, int bufferoffset, int length) => throw
-            new NotSupportedException("Spanner does not support conversion to byte arrays.");
+        public override long GetBytes(int i, long fieldOffset, byte[] buffer, int bufferoffset, int length)


It's annoying that the parameter names don't match the ones we're overriding (and they're unconventional) but fixing that now would be a breaking change, technically. I think we could reasonably argue that any call to this would previously have failed anyway, so now is a reasonable time to change the parameter names - the original declaration is:

public abstract long GetBytes (int ordinal, long dataOffset, byte[]? buffer, int bufferOffset, int length);

Thoughts, @amanda-tarafa?

Yes, I agree that we can make the change.

I've changed them to match the original declaration.

jskeet · 2021-10-27T07:05:56Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerDataReader.cs

+            GaxPreconditions.CheckArgumentRange(length, nameof(length), 0, buffer?.Length ?? int.MaxValue);
+            if (buffer != null)
+            {
+                GaxPreconditions.CheckArgumentRange(bufferoffset + length, nameof(bufferoffset) + "+" + nameof(length), 0, buffer.Length);


I'd prefer this to just use the name of a single parameter. I don't mind which it is, but ArgumentOutOfRangeException.ParamName really should be an actual parameter name IMO.

(I suspect length is a simpler option to use here.)

Changed to nameof(length)

jskeet · 2021-10-27T07:07:53Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerDataReader.cs

+            }
+
+            var bytes = IsDBNull(i) ? null : GetFieldValue<byte[]>(i);
+            if (buffer == null)


Is this behavior documented somewhere? https://docs.microsoft.com/en-us/dotnet/api/system.data.common.dbdatareader.getbytes doesn't specify it. (In general ADO.NET isn't very well specified unfortunately, from a provider perspective.)

Yep, it's here https://docs.microsoft.com/en-us/dotnet/api/system.data.idatarecord.getbytes?view=netstandard-2.1#remarks

I have not been able to find any documentation regarding this. From what I can tell from the source of other implementations:

SQL Server will throw an exception: https://github.com/dotnet/SqlClient/blob/27c56af560cd8049cf894a3ae6a4686c5b7f6481/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDataReader.cs#L1652

From what I can tell from the PostgreSQL source it will return -1: https://github.com/npgsql/npgsql/blob/925b6fc66cac0c593e66c6227e35c042d5771910/src/Npgsql/NpgsqlDataReader.cs#L2150 (The PostgreSQL wire protocol returns -1 as length for NULL values: https://www.postgresql.org/docs/current/protocol-message-formats.html)

MySQL returns 0: https://github.com/mysql-net/MySqlConnector/blob/2ea826b4788e3d1e2c2800de64593c57bb8bff7c/src/MySqlConnector/Core/Row.cs#L135

So AFAICT there's no real consensus on what the best option is.

Ah, sorry, I thought you meant for the case when the column value is NULL. There is nothing in the specs regarding what the method should do in that case.

The case when buffer is null is documented as the way of getting the length of the column, which is in our case unfortunately somewhat inefficient, as it means that in most cases we'll be decoding the BASE64 string twice; once for getting the length and once for getting the actual bytes.

amanda-tarafa

LGTM, just a tests request.
And a link to the spec where the bahviour of the null buffer is described.

amanda-tarafa · 2021-10-27T09:57:36Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerDataReader.cs

@@ -147,8 +148,33 @@ public T GetFieldValue<T>(string columnName)
        public override byte GetByte(int i) => GetFieldValue<byte>(i);

        /// <inheritdoc />
-        public override long GetBytes(int i, long fieldOffset, byte[] buffer, int bufferoffset, int length) => throw
-            new NotSupportedException("Spanner does not support conversion to byte arrays.");
+        public override long GetBytes(int i, long fieldOffset, byte[] buffer, int bufferoffset, int length)


Yes, I agree that we can make the change.

amanda-tarafa · 2021-10-27T10:06:16Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerDataReader.cs

+            }
+
+            var bytes = IsDBNull(i) ? null : GetFieldValue<byte[]>(i);
+            if (buffer == null)


Yep, it's here https://docs.microsoft.com/en-us/dotnet/api/system.data.idatarecord.getbytes?view=netstandard-2.1#remarks

amanda-tarafa · 2021-10-27T10:22:34Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerDataReader.cs

+            GaxPreconditions.CheckArgumentRange(length, nameof(length), 0, buffer?.Length ?? int.MaxValue);
+            if (buffer != null)
+            {
+                GaxPreconditions.CheckArgumentRange(bufferoffset + length, nameof(bufferoffset) + "+" + nameof(length), 0, buffer.Length);


amanda-tarafa · 2021-10-27T10:23:36Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerDataReaderTests.cs

+
+namespace Google.Cloud.Spanner.Data.Tests
+{
+    public class SpannerDataReaderTests


I think a test for a type that's not supporte, would be fine.

I've now added a test case for that as well.

Adds support for DataReader.GetBytes to SpannerDataReader. Although Spanner does not support any streaming of bytes from a result set, which means that there is no performance gain to be achieved by implementing this method, it still makes sense to support it as some frameworks (NHibernate) use the method by default for columns that store binary data.

olavloite · 2021-10-28T07:57:18Z

@jskeet Do you want to take a second look at this? And when do we want to merge this, as it is technically a breaking change?

jskeet

I'm fine with this - and with merging it now. We'll only break users who are calling it with named arguments, and they'd be getting an exception before anyway...

jskeet · 2021-10-28T09:19:31Z

apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/SpannerDataReaderTests.cs

+            Assert.Equal(expectedBytes, buffer);
+        }
+
+        [Fact]


I suspect these ArgumentOutOfRangeException tests could all be done with one theory, but I'm fine leaving it as it is.

Changes in Google.Cloud.Spanner.Data version 3.13.0: - [Commit 02b16d7](googleapis@02b16d7): fix: clone each SpannerParameter when cloning a parameter collection - [Commit c653720](googleapis@c653720): feat: add support for DataReader.GetBytes ([issue 7395](googleapis#7395)) - [Commit 9bc116d](googleapis@9bc116d): fix: allow setting parameter size to the same as the current value ([issue 7401](googleapis#7401)) - [Commit 0666123](googleapis@0666123): feat: Spanner Read API Packages in this release: - Release Google.Cloud.Spanner.Admin.Database.V1 version 3.13.0 - Release Google.Cloud.Spanner.Admin.Instance.V1 version 3.13.0 - Release Google.Cloud.Spanner.Common.V1 version 3.13.0 - Release Google.Cloud.Spanner.Data version 3.13.0 - Release Google.Cloud.Spanner.V1 version 3.13.0

Changes in Google.Cloud.Spanner.Data version 3.13.0: - [Commit 02b16d7](02b16d7): fix: clone each SpannerParameter when cloning a parameter collection - [Commit c653720](c653720): feat: add support for DataReader.GetBytes ([issue 7395](#7395)) - [Commit 9bc116d](9bc116d): fix: allow setting parameter size to the same as the current value ([issue 7401](#7401)) - [Commit 0666123](0666123): feat: Spanner Read API Packages in this release: - Release Google.Cloud.Spanner.Admin.Database.V1 version 3.13.0 - Release Google.Cloud.Spanner.Admin.Instance.V1 version 3.13.0 - Release Google.Cloud.Spanner.Common.V1 version 3.13.0 - Release Google.Cloud.Spanner.Data version 3.13.0 - Release Google.Cloud.Spanner.V1 version 3.13.0

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 26, 2021

olavloite marked this pull request as ready for review October 26, 2021 19:28

olavloite requested a review from a team as a code owner October 26, 2021 19:28

olavloite requested review from skuruppu, amanda-tarafa and jskeet October 26, 2021 19:28

jskeet reviewed Oct 27, 2021

View reviewed changes

amanda-tarafa approved these changes Oct 27, 2021

View reviewed changes

amanda-tarafa added the allow breaking changes Allows breaking changes to be merged. Use with care! label Oct 27, 2021

olavloite force-pushed the spanner-get-bytes branch from 115c8d7 to c57251e Compare October 27, 2021 10:44

olavloite force-pushed the spanner-get-bytes branch from c57251e to e12d75a Compare October 27, 2021 10:50

jskeet approved these changes Oct 28, 2021

View reviewed changes

olavloite merged commit c653720 into master Oct 28, 2021

olavloite deleted the spanner-get-bytes branch October 28, 2021 09:52

jskeet mentioned this pull request Dec 1, 2021

Release Spanner libraries version 3.13.0 #7570

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for DataReader.GetBytes #7395

feat: add support for DataReader.GetBytes #7395

olavloite commented Oct 26, 2021

jskeet left a comment

jskeet Oct 27, 2021

amanda-tarafa Oct 27, 2021

olavloite Oct 27, 2021

jskeet Oct 27, 2021

amanda-tarafa Oct 27, 2021

olavloite Oct 27, 2021

jskeet Oct 27, 2021

amanda-tarafa Oct 27, 2021

olavloite Oct 27, 2021

olavloite Oct 27, 2021

amanda-tarafa left a comment

amanda-tarafa Oct 27, 2021

amanda-tarafa Oct 27, 2021

amanda-tarafa Oct 27, 2021

amanda-tarafa Oct 27, 2021

olavloite Oct 27, 2021

olavloite commented Oct 28, 2021

jskeet left a comment

jskeet Oct 28, 2021

feat: add support for DataReader.GetBytes #7395

feat: add support for DataReader.GetBytes #7395

Conversation

olavloite commented Oct 26, 2021

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olavloite commented Oct 28, 2021

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment