feat(csharp): adding C# functionality #697

davidhcoe · 2023-05-22T18:37:01Z

Introduces C# functionality for ADBC, including base classes for

AdbcDriver
AdbcDatabase
AdbcConnection
AdbcStatement

Also adds a FlightSql implementation

Currently tied to Arrow 13.0.0. Uses a submodule to pull in the Arrow library until the latest NuGet is released.

github-actions · 2023-05-22T18:37:20Z

⚠️ Please follow the Conventional Commits format in CONTRIBUTING.md for PR titles.

lidavidm · 2023-05-22T19:37:08Z

Cool!

It's been >15 years since I last touched C♯, so this will take me a while to review (or I will have to see if Weston or someone else can help).

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

csharp/src/Apache.Arrow.Adbc/Apache.Arrow.Adbc.csproj

csharp/src/Apache.Arrow.Adbc/Core/AdbcConnection.cs

csharp/src/Apache.Arrow.Adbc/Interop/NativePointer.cs

csharp/src/Apache.Arrow.Adbc/Core/Interop.cs

csharp/src/Apache.Arrow.Adbc/Core/AdbcException.cs

csharp/src/Apache.Arrow.Adbc/Core/AdbcConnection.cs

lidavidm

I took a general pass (I haven't looked at the native interop)

csharp/src/Apache.Arrow.Adbc/Core/AdbcStatement.cs

csharp/src/Apache.Arrow.Adbc/Extensions/MatchCollectionExtensions.netstandard.cs

csharp/src/Apache.Arrow.Adbc/Extensions/MarshalExtensions.netstandard.cs

csharp/test/Apache.Arrow.Adbc.FlightSql.Tests/DriverConnectionTests.cs

csharp/test/Apache.Arrow.Adbc.Tests/DriverConnectionTests.cs

csharp/src/Apache.Arrow.Adbc.FlightSql/FlightSqlParameters.cs

csharp/src/Apache.Arrow.Adbc.FlightSql/FlightSqlStatement.cs

csharp/Directory.Build.targets

csharp/src/Apache.Arrow.Adbc/Apache.Arrow.Adbc.csproj

csharp/src/Apache.Arrow.Adbc/Core/Interop.cs

lidavidm

Admittedly I'm only giving things a cursory look. Even if there's a few parts I'm not sure about, I'd be in favor of merging sooner rather than later and continuing to cycle on things in later PRs.

csharp/test/Apache.Arrow.Adbc.FlightSql.Tests/flightsql.parquet

csharp/src/Apache.Arrow.Adbc/Interop/LoadDriver.cs

CurtHagenlocher · 2023-06-06T15:44:10Z

Admittedly I'm only giving things a cursory look. Even if there's a few parts I'm not sure about, I'd be in favor of merging sooner rather than later and continuing to cycle on things in later PRs.

Is there a target date yet for a 0.5 release? I think there's a bunch of stuff that would need to have cleaned up before then.

lidavidm · 2023-06-06T15:46:22Z

Mid-late June, but we could omit this from the release (it wouldn't be part of it anyways unless/until we wire it into the release scripts)

davidhcoe · 2023-06-06T18:12:47Z

Is this far enough we can commit this iteration?

csharp/src/Apache.Arrow.Adbc.FlightSql/Apache.Arrow.Adbc.FlightSql.csproj

csharp/src/Apache.Arrow.Adbc.FlightSql/Apache - Backup.Arrow.Adbc.FlightSql.csproj

csharp/src/Apache.Arrow.Adbc.FlightSql/FlightSqlStatement.cs

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

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

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

lidavidm · 2023-06-06T19:47:32Z

Also note the pre-commit CI failures (mostly: trim trailing whitespace, and make sure the Apache license header is added to new files), then I think it's ready to merge (can we start filing follow-up issues?)

csharp/src/Apache.Arrow.Adbc.FlightSql/Apache - Backup.Arrow.Adbc.FlightSql.csproj

CurtHagenlocher · 2023-06-05T23:41:02Z

csharp/Directory.Build.props

@@ -0,0 +1,60 @@
+<!--


Should there be a .sln file in the root like there is for arrow/csharp?

ah, interesting. yes. It turns out that a root level .gitignore was preventing the one I had from being added

CurtHagenlocher · 2023-06-05T23:42:13Z

csharp/Directory.Build.props

+  <!-- AssemblyInfo properties -->
+  <PropertyGroup>
+    <Product>Apache Arrow ADBC library</Product>
+    <Copyright>Copyright 2016-2019 The Apache Software Foundation</Copyright>


2023? Or 2022-2023? (The root has a file which says just 2022.)

I copied over what was in the Arrow one, but changed to 2022-2023. unless @lidavidm has an opinion on it.

Hmm we should update the Arrow one but 2022-2023 is fine here

CurtHagenlocher · 2023-06-05T23:43:52Z

csharp/Directory.Build.props

+    <Product>Apache Arrow ADBC library</Product>
+    <Copyright>Copyright 2016-2019 The Apache Software Foundation</Copyright>
+    <Company>The Apache Software Foundation</Company>
+    <Version>0.1.0</Version>


Should this reflect the release version of ADBC (which will next be 0.5, I think?)

I only intended for it to be the iteration of this check-in; not necessarily where ADBC is. but, I will defer to @lidavidm on this one as well.

We can update it later (but it would be fine to put 0.5.0 here)

csharp/src/Apache.Arrow.Adbc.FlightSql/FlightSqlConnection.cs

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

CurtHagenlocher · 2023-06-06T00:17:05Z

csharp/src/Apache.Arrow.Adbc/Extensions/MarshalExtensions.netstandard.cs

+ * limitations under the License.
+ */
+
+#if NETSTANDARD


Consider removing via the project file like the Arrow project does.

I wasn't able to do that because I cant add an actual extension to Marshal since it's static

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

CurtHagenlocher · 2023-06-06T01:06:14Z

csharp/src/Apache.Arrow.Adbc/Interop/NativePointer.cs

+
+namespace Apache.Arrow.Adbc.Interop
+{
+    internal readonly struct NativeDelegate<T>


Consider renaming the file to match the class.

If you look at a recent in-progress change for Arrow (apache/arrow#35810) there's some pertinent work and discussion related to NativeDelegate:

It's not needed for .NET >= 5.0 due to the ability to declare a function as [UnmanagedCallersOnly]

The native delegates have been typed somewhat incorrectly, and in particular would produce the wrong outcome when building a 32-bit version for non-Windows platforms.

The proposed solutions are

Only use NativeDelegate when targeting older .NET

Make the actual delegate fields in the structs be private and give them different calling conventions depending on the version of .NET.

That said, the change is still in progress and not final, and there's no reason the same change can't be made here after the initial commit.

I renamed the file - TBD on the other items for now?

csharp/test/Apache.Arrow.Adbc.FlightSql.Tests/ConnectionTests.cs

lidavidm · 2023-06-07T12:21:52Z

It looks like there's still whitespace errors: https://github.com/apache/arrow-adbc/actions/runs/5196453814/jobs/9377170741?pr=697

if you set up pre-commit, you can run pre-commit in the repo root then commit the changes

lidavidm · 2023-06-07T14:38:23Z

There's still license header issues; if need be you can add paths to https://github.com/apache/arrow-adbc/blob/main/dev/release/rat_exclude_files.txt

lidavidm

I think we should merge here, but I'll let others take a look first

eerhardt · 2023-06-07T17:35:27Z

dev/release/rat_exclude_files.txt

+csharp/Apache.Arrow.Adbc.sln
+csharp/src/Apache.Arrow.Adbc.FlightSql/Apache.Arrow.Adbc.FlightSql.csproj
+csharp/src/Apache.Arrow.Adbc/Apache.Arrow.Adbc.csproj
+csharp/test/Apache.Arrow.Adbc.FlightSql.Tests/Apache.Arrow.Adbc.FlightSql.Tests.csproj
+csharp/test/Apache.Arrow.Adbc.Tests/Apache.Arrow.Adbc.Tests.csproj


I didn't know these rat_exclude_files support wildcards. Can we just have:

Suggested change

csharp/Apache.Arrow.Adbc.sln

csharp/src/Apache.Arrow.Adbc.FlightSql/Apache.Arrow.Adbc.FlightSql.csproj

csharp/src/Apache.Arrow.Adbc/Apache.Arrow.Adbc.csproj

csharp/test/Apache.Arrow.Adbc.FlightSql.Tests/Apache.Arrow.Adbc.FlightSql.Tests.csproj

csharp/test/Apache.Arrow.Adbc.Tests/Apache.Arrow.Adbc.Tests.csproj

*.sln

*.csproj

addressing this in #824

eerhardt · 2023-06-07T17:46:59Z

csharp/src/Apache.Arrow.Adbc/Interop/LoadDriver.cs

+        /// </param>
+        public static AdbcDriver Load(string file)
+        {
+            if (file[0] == '/')


What's the plan for Linux? Might be good to have a follow-up issue for supporting Linux.

eerhardt · 2023-06-07T17:47:29Z

csharp/src/Apache.Arrow.Adbc/Interop/LoadDriver.cs

+                AdbcDriverInit init = Marshal.GetDelegateForFunctionPointer<AdbcDriverInit>(symbol);
+                NativeAdbcDriver driver = new NativeAdbcDriver();
+                NativeAdbcError error = new NativeAdbcError();
+                byte result = init(1000000, ref driver, ref error);


Are we ignoring the error here? And above in the MacInterop.

eerhardt · 2023-06-07T17:52:33Z

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

+            public PinnedArray(IArrowArray array)
+            {
+                _array = array;
+                pinnedHandles = new MemoryHandle[GetHandleCount(array.Data)];


Would it make sense to make this a List<MemoryHandle> instead, so we don't have to count all the handles first, and then pin them?

davidhcoe · 2023-06-07T20:16:40Z

chatted with Eric on the side -- he is good to close this PR and incorporate his comments in to a different PR. I've noted them for tracking.

eerhardt · 2023-06-07T20:45:52Z

@lidavidm - It looks like I have merge permissions here since I'm a committer on apache/arrow. But I'm unsure on the process. Can I just click the green button in GitHub? In apache/arrow there is a python script I run: dev/merge_arrow_pr.py, but I'm not seeing it here.

If you want to merge this PR, please go ahead.

lidavidm · 2023-06-07T20:48:27Z

Yup, we just use the button here.

lidavidm · 2023-06-07T20:48:46Z

Thanks @davidhcoe @eerhardt @CurtHagenlocher!

lidavidm · 2023-06-07T20:49:17Z

If possible - can we create GitHub issues for followups? (I suppose you may have your own work queue but it'd be nice to make at least some of that public)

first check in

c024443

lidavidm changed the title ~~adding C# functionality~~ feat(csharp): adding C# functionality May 22, 2023

eerhardt self-requested a review May 22, 2023 20:09

eerhardt reviewed May 22, 2023

View reviewed changes

lidavidm reviewed May 22, 2023

View reviewed changes

csharp/src/Apache.Arrow.Adbc/Core/AdbcException.cs Outdated Show resolved Hide resolved

csharp/src/Apache.Arrow.Adbc/Core/AdbcConnection.cs Outdated Show resolved Hide resolved

lidavidm reviewed May 23, 2023

View reviewed changes

davidhcoe added 5 commits May 23, 2023 11:42

updating from PR feedback

b2fc612

update submodule

9690495

fixes from PR feedback

d676c75

add submodule info to readme

7696f8d

first set of IntPtr to void*; leverage Arrow NativeMemoryManager

42b1f7d

eerhardt reviewed May 25, 2023

View reviewed changes

davidhcoe added 7 commits May 25, 2023 13:34

updating interop and proj file based on feedback

c5e14db

interop code

53abb6e

PR feedback items

5f94376

moving tests to use parquet files for type verification

b44fd4e

adding tests

79868f9

move to byte*

f5e7bce

fix ignore

4c368ec

lidavidm reviewed May 31, 2023

View reviewed changes

csharp/test/Apache.Arrow.Adbc.FlightSql.Tests/flightsql.parquet Outdated Show resolved Hide resolved

davidhcoe added 3 commits May 31, 2023 11:44

cleaning up tests

e826da4

cleaning up tests

49d0efb

remove IntPtr from method definitions

abfbd15

davidhcoe marked this pull request as draft June 1, 2023 13:32

remove parquet in favor of arrow files

13b06f1

eerhardt reviewed Jun 1, 2023

View reviewed changes

csharp/src/Apache.Arrow.Adbc/Interop/LoadDriver.cs Outdated Show resolved Hide resolved

remove GetFunctionPointerForDelegate calls

805fe41

davidhcoe marked this pull request as ready for review June 2, 2023 16:54

re-arrange files; remove type conversions from ADBC layer

5d66ff4

Merge branch 'apache:main' into dev/davidhcoe/adbc-csharp

f3d646f

lidavidm reviewed Jun 6, 2023

View reviewed changes

CurtHagenlocher requested changes Jun 6, 2023

View reviewed changes

davidhcoe added 7 commits June 7, 2023 00:07

file cleanup; PR feedback

5a8c331

change Dictionary parameters to IReadOnlyDictionary

03db8a5

change copyright dates

e0db83f

PR feedback

7e2a02c

even more clean up

9e758d8

documentation formatting

8dbf9d7

editorconfig

381d078

davidhcoe added 2 commits June 7, 2023 10:02

whitespace formatting

3b9f742

header to git files

f16fb3f

add exclude files

9c3b8de

lidavidm approved these changes Jun 7, 2023

View reviewed changes

eerhardt approved these changes Jun 7, 2023

View reviewed changes

lidavidm merged commit ad07ba2 into apache:main Jun 7, 2023

lidavidm added this to the ADBC Libraries 0.5.0 milestone Jun 7, 2023

davidhcoe deleted the dev/davidhcoe/adbc-csharp branch June 15, 2023 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(csharp): adding C# functionality #697

feat(csharp): adding C# functionality #697

davidhcoe commented May 22, 2023 •

edited

Loading

github-actions bot commented May 22, 2023

lidavidm commented May 22, 2023

lidavidm left a comment

lidavidm left a comment

CurtHagenlocher commented Jun 6, 2023

lidavidm commented Jun 6, 2023

davidhcoe commented Jun 6, 2023

lidavidm commented Jun 6, 2023

CurtHagenlocher Jun 5, 2023

davidhcoe Jun 7, 2023

CurtHagenlocher Jun 5, 2023

davidhcoe Jun 7, 2023

lidavidm Jun 7, 2023

CurtHagenlocher Jun 5, 2023

davidhcoe Jun 7, 2023

lidavidm Jun 7, 2023

CurtHagenlocher Jun 6, 2023

davidhcoe Jun 7, 2023

CurtHagenlocher Jun 6, 2023

davidhcoe Jun 7, 2023

lidavidm commented Jun 7, 2023

lidavidm commented Jun 7, 2023

lidavidm left a comment

eerhardt Jun 7, 2023

davidhcoe Jun 20, 2023

eerhardt Jun 7, 2023

eerhardt Jun 7, 2023

eerhardt Jun 7, 2023

davidhcoe commented Jun 7, 2023

eerhardt commented Jun 7, 2023

lidavidm commented Jun 7, 2023

lidavidm commented Jun 7, 2023

lidavidm commented Jun 7, 2023

feat(csharp): adding C# functionality #697

feat(csharp): adding C# functionality #697

Conversation

davidhcoe commented May 22, 2023 • edited Loading

github-actions bot commented May 22, 2023

lidavidm commented May 22, 2023

lidavidm left a comment

Choose a reason for hiding this comment

lidavidm left a comment

Choose a reason for hiding this comment

CurtHagenlocher commented Jun 6, 2023

lidavidm commented Jun 6, 2023

davidhcoe commented Jun 6, 2023

lidavidm commented Jun 6, 2023

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

Choose a reason for hiding this comment

lidavidm commented Jun 7, 2023

lidavidm commented Jun 7, 2023

lidavidm 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

davidhcoe commented Jun 7, 2023

eerhardt commented Jun 7, 2023

lidavidm commented Jun 7, 2023

lidavidm commented Jun 7, 2023

lidavidm commented Jun 7, 2023

davidhcoe commented May 22, 2023 •

edited

Loading