feat(csharp/src/Drivers): introduce Interop.FlightSql driver #2214
Conversation
davidhcoe
commented
Oct 2, 2024
•
davidhcoe
commented
- Introduces the Interop.FlightSql driver to use the FlightSQL Go driver from C#
- Removes the Apache.Arrow.Adbc.Drivers.FlightSql.csproj and Apache.Arrow.Adbc.Tests.Drivers.FlightSql.csproj projects from the solution to start the deprecation process (I am not sure if these are really being used anywhere anyhow. They have their own limitations because of Windows funkiness).
- Adds support for testing against multiple FlightSQL environments simultaneously in the configuration (something we may want to carry forward to other projects)
- Has a first set of test sample data specific to DuckDB and SQLite using https://github.com/voltrondata/SQLFlite as the server(s).
- Adds support for the IntervalType in a generic capacity
…st as possible on Dremio
There was a problem hiding this comment.
Is there any way to make the process of creating something to load the driver and wrap it more generic so that potential future drivers don't require as much boiler plate as this appears to?
Otherwise, this looks pretty fine to me but I'm not very experienced with C#
| /// <param name="schemaName"></param> | ||
| /// <returns></returns> | ||
| public static List<AdbcCatalog> ParseCatalog(RecordBatch recordBatch, string? databaseName, string? schemaName) | ||
| public static List<AdbcCatalog> ParseCatalog(RecordBatch recordBatch, string? schemaName) |
There was a problem hiding this comment.
Why remove the databaseName param?
There was a problem hiding this comment.
It isn’t used in the method.
Co-authored-by: Matt Topol <[email protected]>
Like a base class that https://github.com/apache/arrow-adbc/blob/7fc08cce7404b000c400e00f208ded6dc80f6bb8/csharp/src/Drivers/Interop/FlightSql/FlightSqlDriverLoader.cs could implement? |
|
I do agree there's some boilerplate that might be good to factor out eventually, if we're going to have multiple packages like this |
| @@ -10,12 +10,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Tests", "Tests", "{5BD04C26 | |||
| EndProject | |||
| Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Drivers", "Drivers", "{FEB257A0-4FD3-495E-9A47-9E1649755445}" | |||
| EndProject | |||
| Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Apache.Arrow.Adbc.Drivers.FlightSql", "src\Drivers\FlightSql\Apache.Arrow.Adbc.Drivers.FlightSql.csproj", "{19AA450A-2F87-49BD-9122-8AD07D4C6DCE}" | |||
There was a problem hiding this comment.
We don't seem to be deleting these projects -- and I think we probably shouldn't -- so why remove them from the solution?
There was a problem hiding this comment.
I removed them so they would no longer be built with the solution and new versions of the NuGet packages wouldn't continue to generate.
There was a problem hiding this comment.
But don't you think that someone using .NET 8 would prefer to use these over the interop version?
There was a problem hiding this comment.
I thought it would just cause confusion. My intent was to show the native .NET one hasn't been updated in a while to then deprecate it, but we can keep it for now.
| -- limitations under the License. | ||
|
|
||
| CREATE OR REPLACE TRANSIENT TABLE {ADBC_CATALOG}.{ADBC_SCHEMA}.{ADBC_TABLE} ( | ||
| NUMBERTYPE NUMBER(38,0), |
There was a problem hiding this comment.
nit: This file has a mix of tab-based and space-based indentation
There was a problem hiding this comment.
I decided to just remove this, since it's based on the wrong types, and we would need different samples for the different Flight SQL environments.
Exactly |
