Copilot commented on code in PR #4885:
URL: https://github.com/apache/arrow-adbc/pull/4885#discussion_r4261059157
##########
csharp/src/Apache.Arrow.Adbc/C/CAdbcDriverImporter.cs:
##########
@@ -606,21 +778,193 @@ public override IsolationLevel IsolationLevel
public override bool ReadOnly
{
- get => _readOnly ?? throw AdbcException.NotImplemented("no
value has been set for ReadOnly");
+ get => _readOnly ??
+ (TryGetNativeOption(AdbcOptions.Connection.ReadOnly,
AdbcOptions.GetEnabled, out bool value)
+ ? value
+ : throw AdbcException.NotImplemented("no value has
been set for ReadOnly"));
set
{
SetOption(AdbcOptions.Connection.ReadOnly,
AdbcOptions.GetEnabled(value));
_readOnly = value;
}
}
+ public override string? CurrentCatalog
+ {
+ get => GetOptionOrNull(AdbcOptions.Connection.CurrentCatalog);
+ }
+
+ public override string? CurrentDbSchema
+ {
+ get => GetOptionOrNull(AdbcOptions.Connection.CurrentDbSchema);
+ }
+
+ /// <summary>
+ /// Asks an ADBC 1.1 driver for a value that hasn't been set
through this object.
+ /// Any failure yields null so that callers keep their ADBC 1.0
behavior.
+ /// </summary>
+ private bool TryGetNativeOption<T>(string key, Func<string, T>
parse, out T value)
+ {
+ if (IsVersion1_1)
+ {
+ try
+ {
+ value = parse(GetOption(key));
+ return true;
+ }
+ catch (AdbcException)
+ {
+ }
+ catch (NotSupportedException)
+ {
+ // The driver reported a value that isn't one of the
standard ones
+ }
+ }
+
+ value = default!;
+ return false;
+ }
+
+ private string? GetOptionOrNull(string key)
+ {
+ // A 1.0 driver can't report the value. Don't call GetOption,
whose 1.0
+ // fallback reads these properties.
+ if (!IsVersion1_1)
+ {
+ return null;
+ }
+
+ try
+ {
+ return GetOption(key);
+ }
+ catch (AdbcException e) when (e.Status ==
AdbcStatusCode.NotFound || e.Status == AdbcStatusCode.NotImplemented)
+ {
+ return null;
+ }
+ }
+
+ private OptionFunctions Options
+ {
+ get
+ {
+ if (_disposed) { throw new
ObjectDisposedException(nameof(ImportedAdbcConnection)); }
+ return _driver.ConnectionOptions;
+ }
+ }
+
+ public unsafe override void SetOptionInt64(string key, long value)
+ {
+ if (!IsVersion1_1) { base.SetOptionInt64(key, value); return; }
+ fixed (CAdbcConnection* connection = &_nativeConnection)
+ {
+ CAdbcDriverImporter.SetOptionInt64(_driver,
Options.SetOptionInt, connection, key, value);
+ }
+ }
+
+ public unsafe override void SetOptionDouble(string key, double
value)
+ {
+ if (!IsVersion1_1) { base.SetOptionDouble(key, value); return;
}
+ fixed (CAdbcConnection* connection = &_nativeConnection)
+ {
+ CAdbcDriverImporter.SetOptionDouble(_driver,
Options.SetOptionDouble, connection, key, value);
+ }
+ }
+
+ public unsafe override void SetOptionBytes(string key,
ReadOnlySpan<byte> value)
+ {
+ fixed (CAdbcConnection* connection = &_nativeConnection)
+ {
+ CAdbcDriverImporter.SetOptionBytes(_driver,
Options.SetOptionBytes, connection, key, value);
+ }
+ }
+
+ public unsafe override string GetOption(string key)
+ {
+ if (_driver.DriverVersion < AdbcVersion.Version_1_1_0) {
return base.GetOption(key); }
+
+ fixed (CAdbcConnection* connection = &_nativeConnection)
+ {
+ return GetOptionString(_driver, Options.GetOption,
connection, key);
+ }
+ }
+
+ public unsafe override long GetOptionInt64(string key)
+ {
+ if (_driver.DriverVersion < AdbcVersion.Version_1_1_0) {
return base.GetOptionInt64(key); }
+
+ fixed (CAdbcConnection* connection = &_nativeConnection)
+ {
+ return CAdbcDriverImporter.GetOptionInt64(_driver,
Options.GetOptionInt, connection, key);
+ }
+ }
+
+ public unsafe override double GetOptionDouble(string key)
+ {
+ if (_driver.DriverVersion < AdbcVersion.Version_1_1_0) {
return base.GetOptionDouble(key); }
+
+ fixed (CAdbcConnection* connection = &_nativeConnection)
+ {
+ return CAdbcDriverImporter.GetOptionDouble(_driver,
Options.GetOptionDouble, connection, key);
+ }
+ }
+
+ public unsafe override byte[] GetOptionBytes(string key)
+ {
+ if (_driver.DriverVersion < AdbcVersion.Version_1_1_0) {
return base.GetOptionBytes(key); }
+
+ fixed (CAdbcConnection* connection = &_nativeConnection)
+ {
+ return GetOptionBuffer(_driver, Options.GetOptionBytes,
connection, key);
+ }
+ }
+
+ public unsafe override IArrowArrayStream GetStatistics(string?
catalogPattern, string? schemaPattern, string tableNamePattern, bool
approximate)
+ {
+ using (Utf8Helper utf8Catalog = new Utf8Helper(catalogPattern))
+ using (Utf8Helper utf8Schema = new Utf8Helper(schemaPattern))
+ using (Utf8Helper utf8Table = new Utf8Helper(tableNamePattern))
+ using (CallHelper caller = new CallHelper(_driver))
Review Comment:
This new override calls the 1.1 `ConnectionGetStatistics` callback
unconditionally. For a negotiated 1.0 driver, `ValidateDriver` returns before
those callbacks are populated (`CAdbcDriverImporter.cs:183-216`), so this
dispatches through a zero function pointer instead of the base class's
`NotImplemented` implementation. Add an `IsVersion1_1` guard and return
`base.GetStatistics(...)` for 1.0 drivers.
This issue also appears on line 943 of the same file.
##########
csharp/src/Apache.Arrow.Adbc/C/CAdbcDriverImporter.cs:
##########
@@ -502,12 +591,87 @@ public unsafe override AdbcConnection
Connect(IReadOnlyDictionary<string, string
public unsafe override void SetOption(string key, string value)
{
- using (CallHelper caller = new CallHelper())
+ using (CallHelper caller = new CallHelper(_driver))
{
caller.Call(Driver.DatabaseSetOption, ref _nativeDatabase,
key, value);
}
}
+ public unsafe override void SetOptionInt64(string key, long value)
+ {
+ if (_driver.DriverVersion < AdbcVersion.Version_1_1_0) {
base.SetOptionInt64(key, value); return; }
+ fixed (CAdbcDatabase* database = &_nativeDatabase)
+ {
+ CAdbcDriverImporter.SetOptionInt64(_driver,
Options.SetOptionInt, database, key, value);
+ }
+ }
+
+ public unsafe override void SetOptionDouble(string key, double
value)
+ {
+ if (_driver.DriverVersion < AdbcVersion.Version_1_1_0) {
base.SetOptionDouble(key, value); return; }
+ fixed (CAdbcDatabase* database = &_nativeDatabase)
+ {
+ CAdbcDriverImporter.SetOptionDouble(_driver,
Options.SetOptionDouble, database, key, value);
+ }
+ }
+
+ public unsafe override void SetOptionBytes(string key,
ReadOnlySpan<byte> value)
+ {
+ fixed (CAdbcDatabase* database = &_nativeDatabase)
+ {
+ CAdbcDriverImporter.SetOptionBytes(_driver,
Options.SetOptionBytes, database, key, value);
+ }
+ }
Review Comment:
This setter is callable on a driver negotiated as ADBC 1.0, but
`ValidateDriver` returns before populating the 1.1 function pointers
(`CAdbcDriverImporter.cs:183-216`). Unlike the integer and double setters
below, this path does not fall back to the base implementation, so it invokes a
zero `DatabaseSetOptionBytes` pointer instead of returning the documented
`NotImplemented` status (and can crash on native-call targets). Guard the call
by driver version and delegate to `base.SetOptionBytes` for 1.0 drivers.
This issue also appears in the following locations of the same file:
- line 874
- line 1526
##########
csharp/src/Apache.Arrow.Adbc/C/CAdbcDriverImporter.cs:
##########
@@ -1022,7 +1366,7 @@ public unsafe override QueryResult ExecuteQuery()
public override unsafe Schema ExecuteSchema()
{
- using (CallHelper caller = new CallHelper())
+ using (CallHelper caller = new CallHelper(_driver))
{
fixed (CAdbcStatement* statement = &_nativeStatement)
Review Comment:
A 1.0 imported driver has no `StatementExecuteSchema` pointer because
`ValidateDriver` skips the 1.1 fields. Guard on `DriverVersion` and call
`base.ExecuteSchema()` before this native call; otherwise invoking the newly
added override on a 1.0 driver jumps through null.
--
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]
