fix(csharp/src/Apache.Arrow.Adbc): correct StandardSchemas.ColumnSchema data types #1731

birschick-bq · 2024-04-19T00:30:10Z

Corrects the two fields in StandardSchemas.ColumnSchema
Adds unit tests for the StandardSchemas
Adds extension methods to validate data using Schema or IReadOnlyList<Field>.
Add extension method to CreateNestedListArray

Resolves #1729

…ma data types

birschick-bq · 2024-04-19T00:37:06Z

csharp/src/Apache.Arrow.Adbc/StandardSchemas.cs

@@ -128,8 +128,8 @@ public static class StandardSchemas
                new Field("xdbc_scope_catalog", StringType.Default, true),
                new Field("xdbc_scope_schema", StringType.Default, true),
                new Field("xdbc_scope_table", StringType.Default, true),
-                new Field("xdbc_is_autoincrement", StringType.Default, true),
-                new Field("xdbc_is_generatedcolumn", StringType.Default, true)
+                new Field("xdbc_is_autoincrement", BooleanType.Default, true),


Corrected data types.

lidavidm · 2024-04-19T00:41:43Z

Oh hmm, the regex I'm using to validate the title is too strict - periods should probably be allowed. You can ignore that check for now, please use the correct path.

.pre-commit-config.yaml

…lumnschema-types

…/inconsistent-columnschema-types

CurtHagenlocher

On the whole, I'm not excited about exposing two public and non-ADBC-specific functions from this assembly -- particularly without a stronger specification for them. Can we make them internal and then use InternalsVisibleTo? This is already being used in the C# Arrow implementation, so the build processes should support it.

It would also be nice to eliminate the large number of nullability warnings by either removing the nullability annotations or by putting the code into a #nullable block.

CurtHagenlocher · 2024-04-22T20:19:18Z

csharp/src/Apache.Arrow.Adbc/Extensions/StandardSchemaExtensions.cs

+        /// <param name="schema">The schema to validate against.</param>
+        /// <param name="data">The data array to validate.</param>
+        /// <exception cref="ArgumentException">Throws an exception if the number of columns or type data types in the data array do not match the schema fields.</exception>
+        public static IReadOnlyList<IArrowArray> Validate(this Schema schema, IReadOnlyList<IArrowArray> data)


This does a very superficial comparison of types. For instance, it doesn't check the precision and scale of decimals, length of a fixed-size list, the units of dates, times or intervals, whether or not a union is dense or sparse, the name or nullability of a field, etc. That seems mostly okay for an internal method being used privately against a relatively-known schema, but problematic for a public function that's being exported for anyone to use.

How many of the gaps in the above list are features (for this particular use case) rather than flaws? Should this be standard functionality in the core Arrow library? It seems to overlap with what the (internal) class ArrayDataTypeComparer does.

I'd be happy to use the existing class, if it were exposed. I could also make this better and more rigorous, if you believe it might be used outside of its current context.

CurtHagenlocher · 2024-04-22T20:27:27Z

csharp/src/Apache.Arrow.Adbc/Extensions/ListArrayExtensions.cs

+        /// <param name="arrayList">The list of data.</param>
+        /// <param name="dataType">The data type of the contained data.</param>
+        /// <returns>A <see cref="ListArray"/> of the data.</returns>
+        public static ListArray CreateNestedListArray(this IReadOnlyList<IArrowArray?> arrayList, IArrowType dataType)


The summary seems underspecified for a public function. I can't tell from the text what it does or what I need to pass to it to make it work.

Is this functionality which should be in the core Arrow library?

Yes. I believe this should likely be included in the Arrow library. Not sure of it proper placement in the namespace, yet.

…ecific external projects.

…/inconsistent-columnschema-types

birschick-bq

Changed the classed in the new extensions to be internal and exposed them in the AssemblyInfo.cs properties.

birschick-bq · 2024-04-26T23:21:33Z

csharp/src/Apache.Arrow.Adbc/Extensions/ListArrayExtensions.cs

+
+namespace Apache.Arrow.Adbc.Extensions
+{
+    internal static class ListArrayExtensions


This extensions is marked as internal.

birschick-bq · 2024-04-26T23:22:06Z

csharp/src/Apache.Arrow.Adbc/Extensions/StandardSchemaExtensions.cs

+
+namespace Apache.Arrow.Adbc.Extensions
+{
+    internal static class StandardSchemaExtensions


This extension is marked as internal.

birschick-bq · 2024-04-26T23:22:51Z

csharp/src/Apache.Arrow.Adbc/Properties/AssemblyInfo.cs

+
+using System.Runtime.CompilerServices;
+
+[assembly: InternalsVisibleTo("Apache.Arrow.Adbc.Drivers.Apache, PublicKey=0024000004800000940000000602000000240000525341310004000001000100e504183f6d470d6b67b6d19212be3e1f598f70c246a120194bc38130101d0c1853e4a0f2232cb12e37a7a90e707aabd38511dac4f25fcb0d691b2aa265900bf42de7f70468fc997551a40e1e0679b605aa2088a4a69e07c117e988f5b1738c570ee66997fba02485e7856a49eca5fd0706d09899b8312577cbb9034599fc92d4")]


Expose internal interfaces to three libraries, assuming the current signing key.

birschick-bq · 2024-04-26T23:24:21Z

csharp/src/Apache.Arrow.Adbc/Extensions/StandardSchemaExtensions.cs

+        /// <param name="schema">The schema to validate against.</param>
+        /// <param name="data">The data array to validate.</param>
+        /// <exception cref="ArgumentException">Throws an exception if the number of columns or type data types in the data array do not match the schema fields.</exception>
+        public static IReadOnlyList<IArrowArray> Validate(this Schema schema, IReadOnlyList<IArrowArray> data)


I'd be happy to use the existing class, if it were exposed. I could also make this better and more rigorous, if you believe it might be used outside of its current context.

birschick-bq · 2024-04-26T23:25:35Z

csharp/src/Apache.Arrow.Adbc/Extensions/ListArrayExtensions.cs

+        /// <param name="arrayList">The list of data.</param>
+        /// <param name="dataType">The data type of the contained data.</param>
+        /// <returns>A <see cref="ListArray"/> of the data.</returns>
+        public static ListArray CreateNestedListArray(this IReadOnlyList<IArrowArray?> arrayList, IArrowType dataType)


Yes. I believe this should likely be included in the Arrow library. Not sure of it proper placement in the namespace, yet.

birschick-bq · 2024-04-26T23:56:08Z

On the whole, I'm not excited about exposing two public and non-ADBC-specific functions from this assembly -- particularly without a stronger specification for them. Can we make them internal and then use InternalsVisibleTo? This is already being used in the C# Arrow implementation, so the build processes should support it.

It would also be nice to eliminate the large number of nullability warnings by either removing the nullability annotations or by putting the code into a #nullable block.

RE: nullability annotations

I believe we need a separate ticket for this. It will require hundreds of small changes to mark current non-nullable types as nullable. I see you have already created a new issue for this.

…ma data types (apache#1731) 1. Corrects the two fields in `StandardSchemas.ColumnSchema` 2. Adds unit tests for the `StandardSchemas` 3. Adds extension methods to validate data using `Schema` or `IReadOnlyList<Field>`. 4. Add extension method to `CreateNestedListArray` Resolves apache#1729

bug(csharp/src/Apache.Arrow.Adbc): correct StandardSchemas.ColumnSche…

567aa6c

…ma data types

birschick-bq requested review from lidavidm and CurtHagenlocher as code owners April 19, 2024 00:30

github-actions bot added this to the ADBC Libraries 1.0.0 milestone Apr 19, 2024

lidavidm changed the title ~~bug(csharp/src/Apache.Arrow.Adbc): correct StandardSchemas.ColumnSchema data types~~ fix(csharp/src/Apache.Arrow.Adbc): correct StandardSchemas.ColumnSchema data types Apr 19, 2024

birschick-bq commented Apr 19, 2024

View reviewed changes

birschick-bq changed the title ~~fix(csharp/src/Apache.Arrow.Adbc): correct StandardSchemas.ColumnSchema data types~~ fix(csharp/src/Apache_Arrow_Adbc): correct StandardSchemas.ColumnSchema data types Apr 19, 2024

birschick-bq changed the title ~~fix(csharp/src/Apache_Arrow_Adbc): correct StandardSchemas.ColumnSchema data types~~ fix(csharp/src/Apache.Arrow.Adbc): correct StandardSchemas.ColumnSchema data types Apr 19, 2024

lidavidm mentioned this pull request Apr 19, 2024

ci: allow periods in PR comments, properly unit test this thing #1732

Closed

lidavidm reviewed Apr 19, 2024

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

birschick-bq added 3 commits April 19, 2024 09:44

Revert changes to .pre-commit-config file.

844df7f

Merge branch 'apache:main' into dev/birschick-bq/1729/inconsistent-co…

88491e7

…lumnschema-types

Revert accidental add of folder.

fd0fac5

birschick-bq marked this pull request as draft April 19, 2024 17:27

birschick-bq added 2 commits April 19, 2024 13:57

Add further coverage of Union and other schemas.

caca299

Corrected bugs.

d09c8fa

birschick-bq marked this pull request as ready for review April 19, 2024 21:03

Merge remote-tracking branch 'origin/main' into dev/birschick-bq/1729…

8c88186

…/inconsistent-columnschema-types

CurtHagenlocher requested changes Apr 22, 2024

View reviewed changes

birschick-bq added 3 commits April 26, 2024 15:47

Mark new extensions as internal and expose the internal methods to sp…

6b891c0

…ecific external projects.

Merge remote-tracking branch 'origin/main' into dev/birschick-bq/1729…

a4bdf6d

…/inconsistent-columnschema-types

Correct IReadOnlyList type def.

d46e982

birschick-bq commented Apr 26, 2024

View reviewed changes

birschick-bq requested a review from CurtHagenlocher April 26, 2024 23:27

birschick-bq added 2 commits April 26, 2024 16:47

Change name of extension method to build list array for type.

13e7b73

Minor update to documentation of BuildListArrayForType

28d2a84

CurtHagenlocher approved these changes Apr 27, 2024

View reviewed changes

CurtHagenlocher merged commit 3173a1d into apache:main Apr 27, 2024
6 checks passed

birschick-bq deleted the dev/birschick-bq/1729/inconsistent-columnschema-types branch May 9, 2024 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(csharp/src/Apache.Arrow.Adbc): correct StandardSchemas.ColumnSchema data types #1731

fix(csharp/src/Apache.Arrow.Adbc): correct StandardSchemas.ColumnSchema data types #1731

birschick-bq commented Apr 19, 2024 •

edited

Loading

birschick-bq Apr 19, 2024

lidavidm commented Apr 19, 2024

CurtHagenlocher left a comment

CurtHagenlocher Apr 22, 2024

birschick-bq Apr 26, 2024 •

edited

Loading

CurtHagenlocher Apr 22, 2024

birschick-bq Apr 26, 2024 •

edited

Loading

birschick-bq left a comment

birschick-bq Apr 26, 2024

birschick-bq Apr 26, 2024

birschick-bq Apr 26, 2024

birschick-bq Apr 26, 2024 •

edited

Loading

birschick-bq Apr 26, 2024 •

edited

Loading

birschick-bq commented Apr 26, 2024 •

edited

Loading


		using System.Runtime.CompilerServices;

		[assembly: InternalsVisibleTo("Apache.Arrow.Adbc.Drivers.Apache, PublicKey=0024000004800000940000000602000000240000525341310004000001000100e504183f6d470d6b67b6d19212be3e1f598f70c246a120194bc38130101d0c1853e4a0f2232cb12e37a7a90e707aabd38511dac4f25fcb0d691b2aa265900bf42de7f70468fc997551a40e1e0679b605aa2088a4a69e07c117e988f5b1738c570ee66997fba02485e7856a49eca5fd0706d09899b8312577cbb9034599fc92d4")]

fix(csharp/src/Apache.Arrow.Adbc): correct StandardSchemas.ColumnSchema data types #1731

fix(csharp/src/Apache.Arrow.Adbc): correct StandardSchemas.ColumnSchema data types #1731

Conversation

birschick-bq commented Apr 19, 2024 • edited Loading

birschick-bq Apr 19, 2024

Choose a reason for hiding this comment

lidavidm commented Apr 19, 2024

CurtHagenlocher left a comment

Choose a reason for hiding this comment

CurtHagenlocher Apr 22, 2024

Choose a reason for hiding this comment

birschick-bq Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

CurtHagenlocher Apr 22, 2024

Choose a reason for hiding this comment

birschick-bq Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

birschick-bq left a comment

Choose a reason for hiding this comment

birschick-bq Apr 26, 2024

Choose a reason for hiding this comment

birschick-bq Apr 26, 2024

Choose a reason for hiding this comment

birschick-bq Apr 26, 2024

Choose a reason for hiding this comment

birschick-bq Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

birschick-bq Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

birschick-bq commented Apr 26, 2024 • edited Loading

birschick-bq commented Apr 19, 2024 •

edited

Loading

birschick-bq Apr 26, 2024 •

edited

Loading

birschick-bq Apr 26, 2024 •

edited

Loading

birschick-bq Apr 26, 2024 •

edited

Loading

birschick-bq Apr 26, 2024 •

edited

Loading

birschick-bq commented Apr 26, 2024 •

edited

Loading