CurtHagenlocher commented on code in PR #2961:
URL: https://github.com/apache/arrow-adbc/pull/2961#discussion_r2157380014
##########
ci/configs/flightsql-spiceai.json:
##########
@@ -0,0 +1,28 @@
+{
+ "driverPath": "../../../../../go/adbc/pkg/libadbc_driver_flightsql.so",
+ "driverEntryPoint": "FlightSqlDriverInit",
+ "testEnvironments": [
+ "SpiceAI_Local"
Review Comment:
nit: this file is mostly tab-indented but this one line has spaces. It would
be nice to be consistent.
##########
csharp/test/Drivers/Interop/FlightSql/FlightSqlTestConfiguration.cs:
##########
@@ -40,7 +40,9 @@ internal enum FlightSqlTestEnvironmentType
Denodo,
Dremio,
DuckDB,
- SQLite
+ SQLite,
+
Review Comment:
nit: remove blank line
##########
csharp/src/Apache.Arrow.Adbc/Extensions/IArrowArrayExtensions.cs:
##########
@@ -103,6 +103,8 @@ public static class IArrowArrayExtensions
return ((Int64Array)arrowArray).GetValue(index);
case ArrowTypeId.String:
return ((StringArray)arrowArray).GetString(index);
+ case ArrowTypeId.LargeString:
+ return ((LargeStringArray)arrowArray).GetString(index);
Review Comment:
We should also add `LargeBinary`, `LargeList`, `StringView`, `BinaryView`
and `ListView`, though that doesn't need to be part of this PR. Does spice.ai
return any of those types?
We can't add `LargeStringView`, `LargeBinaryView` or `LargeListView` until
the C# Arrow library supports those types.
##########
csharp/test/Drivers/Interop/FlightSql/ClientTests.cs:
##########
@@ -159,12 +159,12 @@ public void CanClientExecuteQueryWithNoResults()
{
foreach (FlightSqlTestEnvironment environment in _environments)
{
- environment.Query = "SELECT * WHERE 0=1";
+ environment.Query = "SELECT 1 WHERE false"; // The query
simulates no results
Review Comment:
Can we change this to `SELECT 1 WHERE 0=1`? The vast majority of my deep SQL
experience is with MSSQL and SQL-92 compatible systems, and boolean types and
literals weren't added until SQL-99 (and still aren't supported by MSSQL). So
while my view may be overly colored by that experience, it does still suggest
that `0=1` will be more-widely supported than `FALSE`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
