Add native ADBC FlightSQL driver support for osx-arm64, fix ADBC resources disposal - #16
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds native ADBC FlightSQL driver support for all platforms, removing the dependency on the Go-based driver interop. The changes enable direct loading of native drivers and include special handling for macOS ARM64 which downloads the driver at build time if not available in the upstream NuGet package.
Key changes:
- Direct native driver loading using
AdbcDriverLoaderwith platform-specific path resolution - Build-time download mechanism for osx-arm64 native driver via MSBuild targets
- Updated URI scheme handling to convert http/https to grpc/grpc+tls schemes
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| Spice/src/Adbc/SpiceAdbcClient.cs | Replaced Go-based FlightSQL driver with native driver loading, added platform-specific path resolution methods, and updated URI scheme conversion logic |
| Spice/build/SpiceAI.targets | New MSBuild targets file that downloads and extracts osx-arm64 driver from Python wheel at build time |
| Spice/Spice.csproj | Added SpiceAI.targets to package and imported it for local development |
| .github/workflows/pr.yaml | Removed continue-on-error flag for macOS tests, indicating native driver support is now working |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Include platform-specific driver loading logic and update tests to use explicit FlightSQL endpoints.
249cbe7 to
1571493
Compare
- Wrap query result streams to ensure the underlying ADBC statement remains alive for the stream's lifetime and is disposed correctly. - Update integration tests to use 'using' for result streams. - Add StatementBoundArrowArrayStream to manage disposal order.
8920139 to
f723f01
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
🗣 Description
This pull request updates the Spice ADBC FlightSQL client to support direct loading of platform-specific native drivers, improves resource management for query execution, and enhances test coverage and correctness. The most significant changes are the introduction of dynamic native driver resolution, improved statement disposal, and test updates for more robust integration.
Native driver loading and platform support:
SpiceAdbcClient.csto resolve and load the correct native ADBC FlightSQL driver at runtime based on the current OS and architecture, supporting platforms such as osx-arm64 that are not covered by the upstream NuGet package. This includes new helper methods for runtime identifier and driver filename resolution, and a corresponding update toSpice.csprojto include native binaries in the NuGet package. [1] [2]Resource management improvements:
StatementBoundArrowArrayStreamclass to ensure that ADBC statements are kept alive for the lifetime of the result stream and disposed in the correct order, preventing premature resource release and potential errors during streaming. [1] [2] [3]Connection URI handling:
SpiceAdbcClient.csto automatically translatehttp/httpsURIs to the appropriategrpc/grpc+tlsschemes expected by the native driver, improving compatibility and usability.Test improvements:
ParameterizedQueryIntegrationTest.csto use fully qualified table names (e.g.,spice.tpch.customer) and to properly dispose of result streams withusing, ensuring correctness and resource cleanup. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]Minor cleanup:
SpiceAdbcClient.cs. [1] [2]