CurtHagenlocher commented on code in PR #4075:
URL: https://github.com/apache/arrow-adbc/pull/4075#discussion_r3249482111
##########
csharp/test/Apache.Arrow.Adbc.Tests/Apache.Arrow.Adbc.Testing.csproj:
##########
@@ -1,12 +1,19 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
- <TargetFrameworks
Condition="'$(IsWindows)'=='true'">net8.0;net472</TargetFrameworks>
- <TargetFrameworks
Condition="'$(TargetFrameworks)'==''">net8.0</TargetFrameworks>
+ <TargetFrameworks
Condition="'$(IsWindows)'=='true'">net8.0;net10.0;net472</TargetFrameworks>
+ <TargetFrameworks
Condition="'$(TargetFrameworks)'==''">net8.0;net10.0</TargetFrameworks>
<IsPackable>true</IsPackable>
<IsTestProject>true</IsTestProject>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<ProcessArchitecture>$([System.Runtime.InteropServices.RuntimeInformation]::ProcessArchitecture.ToString().ToLowerInvariant())</ProcessArchitecture>
+ <!--
+ net10's NuGet audit elevates known vulnerabilities of transitive
packages to errors.
+ The OpenTelemetry.Api 1.12.0 advisory (GHSA-g94r-2vxg-569j) is pulled in
transitively
Review Comment:
o.0
##########
csharp/src/Apache.Arrow.Adbc/DriverManager/ManagedDriverLoader.cs:
##########
@@ -0,0 +1,172 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Collections.Concurrent;
+using System.IO;
+using System.Reflection;
+using System.Threading;
+#if NET6_0_OR_GREATER
+using System.Diagnostics.CodeAnalysis;
+using System.Runtime.Loader;
+#endif
+
+namespace Apache.Arrow.Adbc.DriverManager
+{
+ /// <summary>
+ /// Provides cross-platform assembly loading functionality for managed
ADBC drivers.
+ /// </summary>
+ /// <remarks>
+ /// <para>
+ /// This type abstracts the differences in assembly loading between .NET
Framework
+ /// (which uses Fusion / <c>LoadFrom</c> probing) and modern .NET (which
uses
+ /// <c>AssemblyLoadContext</c> and <c>.deps.json</c>-driven resolution).
+ /// </para>
+ /// <para>
+ /// <b>Dependency resolution on .NET Framework (net472, netstandard2.0
build):</b>
+ /// <see cref="Assembly.LoadFrom(string)"/> probes the assembly's
directory for
+ /// dependencies. Drop the driver assembly and all of its dependencies in
the same
+ /// directory and they will be found.
+ /// </para>
+ /// <para>
+ /// <b>Dependency resolution on .NET 6 / .NET 8 / .NET 10:</b>
+ /// </para>
+ /// <list type="bullet">
+ /// <item>
+ /// <description>
+ /// The driver assembly is loaded with <see
cref="Assembly.LoadFrom(string)"/>
+ /// into <see cref="AssemblyLoadContext.Default"/>. This is
intentional: it keeps
+ /// a single identity for <c>Apache.Arrow.Adbc.AdbcDriver</c> across
the host
+ /// and the driver so that <c>IsAssignableFrom</c> checks work
correctly.
+ /// </description>
+ /// </item>
+ /// <item>
+ /// <description>
+ /// Loading into the default context means the <b>host's</b>
<c>.deps.json</c>
+ /// drives resolution, not the driver's. Dependencies that are not
already known
+ /// to the host will not be found by default. To bridge that gap,
this loader
+ /// registers an <see cref="AssemblyDependencyResolver"/> for each
driver
+ /// assembly and attaches a single
+ /// <see cref="AssemblyLoadContext.Resolving"/> handler to
+ /// <see cref="AssemblyLoadContext.Default"/>. When the runtime
fails to resolve
+ /// a dependency, the handler consults each registered driver's
resolver, which
+ /// reads the driver's <c>.deps.json</c> next to it on disk.
+ /// </description>
+ /// </item>
+ /// <item>
+ /// <description>
+ /// Drivers should be published with their <c>.deps.json</c> file
alongside the
+ /// assembly (the default when building with the .NET SDK). Native
dependencies
+ /// should follow the standard <c>runtimes/<rid>/native</c>
layout.
+ /// </description>
+ /// </item>
+ /// <item>
+ /// <description>
+ /// If a driver does not ship a <c>.deps.json</c>, the loader still
works but
Review Comment:
In my experience, loading a driver without a `.deps.json` file did not
generally work. Has it worked for you?
##########
csharp/src/Apache.Arrow.Adbc/DriverManager/AdbcDriverManager.cs:
##########
@@ -0,0 +1,787 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Collections.Generic;
+using System.Globalization;
+using System.IO;
+using System.Reflection;
+using System.Runtime.InteropServices;
+#if NET6_0_OR_GREATER
+using System.Diagnostics.CodeAnalysis;
+#endif
+using Apache.Arrow.Adbc.C;
+
+namespace Apache.Arrow.Adbc.DriverManager
+{
+ /// <summary>
+ /// Provides methods to locate and load ADBC drivers, optionally using
TOML manifest
+ /// files and connection profiles.
+ /// Mirrors the free functions declared in <c>adbc_driver_manager.h</c>:
+ /// <c>AdbcLoadDriver</c> and <c>AdbcFindLoadDriver</c>.
+ /// </summary>
+ public static class AdbcDriverManager
+ {
+ /// <summary>
+ /// The environment variable that specifies additional driver search
paths.
+ /// </summary>
+ public const string DriverPathEnvVar = "ADBC_DRIVER_PATH";
+
+ private static readonly string[] s_nativeExtensions = { ".so", ".dll",
".dylib" };
+
+ //
-----------------------------------------------------------------------
+ // AdbcLoadDriver – load a driver directly from an absolute or
relative path
+ //
-----------------------------------------------------------------------
+
+ /// <summary>
+ /// Loads an ADBC driver from an explicit file path.
+ /// Mirrors <c>AdbcLoadDriver</c> in <c>adbc_driver_manager.h</c>.
+ /// </summary>
+ /// <remarks>
+ /// <para>
+ /// If a TOML manifest file with the same base name exists in the same
directory
+ /// as the driver file, it will be automatically detected and used.
For example,
+ /// loading <c>libadbc_driver_snowflake.dll</c> will automatically use
+ /// <c>libadbc_driver_snowflake.toml</c> if present.
+ /// </para>
+ /// <para>
+ /// The co-located manifest can specify default options, override the
driver path,
+ /// or provide additional metadata. The <paramref name="entrypoint"/>
parameter
+ /// always takes precedence over any entrypoint derived from the
manifest or filename.
+ /// </para>
+ /// </remarks>
+ /// <param name="driverPath">
+ /// The absolute or relative path to the driver shared library.
+ /// </param>
+ /// <param name="entrypoint">
+ /// The symbol name of the driver initialisation function. When
<c>null</c> the
Review Comment:
total nit: we have one instance of the American spelling `initialization`
and two instances of the British spelling `initialisation`. My desire for
consistency is warring with my desire for cultural inclusiveness, and my spell
checker definitely has feelings.
##########
csharp/src/Apache.Arrow.Adbc/DriverManager/TomlConnectionProfile.cs:
##########
@@ -0,0 +1,269 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Collections.Generic;
+using System.Globalization;
+
+namespace Apache.Arrow.Adbc.DriverManager
+{
+ /// <summary>
+ /// An <see cref="IConnectionProfile"/> loaded from a TOML file.
+ /// </summary>
+ /// <remarks>
+ /// <para>
+ /// The expected file format for profiles is:
+ /// </para>
+ /// <code>
+ /// profile_version = 1
+ /// driver = "driver_name"
+ ///
+ /// [Options]
+ /// option1 = "value1"
+ /// option2 = 42
+ /// option3 = 3.14
+ /// </code>
+ /// <para>
+ /// For backward compatibility, the following legacy field names are also
supported:
+ /// </para>
+ /// <list type="bullet">
+ /// <item><c>version</c> (use <c>profile_version</c> instead)</item>
+ /// <item><c>[options]</c> section (use <c>[Options]</c> instead)</item>
+ /// </list>
+ /// <para>
+ /// Boolean option values are converted to the string equivalents
<c>"true"</c> or
+ /// <c>"false"</c> and placed in <see cref="StringOptions"/>.
+ /// Integer values are placed in <see cref="IntOptions"/> and double
values in
+ /// <see cref="DoubleOptions"/>.
+ /// Values of the form <c>env_var(ENV_VAR_NAME)</c> are expanded from the
named
+ /// environment variable when <see cref="ResolveEnvVars"/> is called.
+ /// </para>
+ /// </remarks>
+ public sealed class TomlConnectionProfile : IConnectionProfile
Review Comment:
To the point of my other comment, this is basically a data transfer object
plus some static helpers which could live on the public
`IConnectionProfileProvider` implementation.
##########
csharp/src/Apache.Arrow.Adbc/DriverManager/ManagedDriverLoader.cs:
##########
@@ -0,0 +1,172 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Collections.Concurrent;
+using System.IO;
+using System.Reflection;
+using System.Threading;
+#if NET6_0_OR_GREATER
+using System.Diagnostics.CodeAnalysis;
+using System.Runtime.Loader;
+#endif
+
+namespace Apache.Arrow.Adbc.DriverManager
+{
+ /// <summary>
+ /// Provides cross-platform assembly loading functionality for managed
ADBC drivers.
+ /// </summary>
+ /// <remarks>
+ /// <para>
+ /// This type abstracts the differences in assembly loading between .NET
Framework
+ /// (which uses Fusion / <c>LoadFrom</c> probing) and modern .NET (which
uses
+ /// <c>AssemblyLoadContext</c> and <c>.deps.json</c>-driven resolution).
+ /// </para>
+ /// <para>
+ /// <b>Dependency resolution on .NET Framework (net472, netstandard2.0
build):</b>
+ /// <see cref="Assembly.LoadFrom(string)"/> probes the assembly's
directory for
+ /// dependencies. Drop the driver assembly and all of its dependencies in
the same
+ /// directory and they will be found.
+ /// </para>
+ /// <para>
+ /// <b>Dependency resolution on .NET 6 / .NET 8 / .NET 10:</b>
+ /// </para>
+ /// <list type="bullet">
+ /// <item>
+ /// <description>
+ /// The driver assembly is loaded with <see
cref="Assembly.LoadFrom(string)"/>
+ /// into <see cref="AssemblyLoadContext.Default"/>. This is
intentional: it keeps
+ /// a single identity for <c>Apache.Arrow.Adbc.AdbcDriver</c> across
the host
+ /// and the driver so that <c>IsAssignableFrom</c> checks work
correctly.
+ /// </description>
+ /// </item>
+ /// <item>
+ /// <description>
+ /// Loading into the default context means the <b>host's</b>
<c>.deps.json</c>
+ /// drives resolution, not the driver's. Dependencies that are not
already known
+ /// to the host will not be found by default. To bridge that gap,
this loader
+ /// registers an <see cref="AssemblyDependencyResolver"/> for each
driver
+ /// assembly and attaches a single
+ /// <see cref="AssemblyLoadContext.Resolving"/> handler to
+ /// <see cref="AssemblyLoadContext.Default"/>. When the runtime
fails to resolve
+ /// a dependency, the handler consults each registered driver's
resolver, which
+ /// reads the driver's <c>.deps.json</c> next to it on disk.
+ /// </description>
+ /// </item>
+ /// <item>
+ /// <description>
+ /// Drivers should be published with their <c>.deps.json</c> file
alongside the
+ /// assembly (the default when building with the .NET SDK). Native
dependencies
+ /// should follow the standard <c>runtimes/<rid>/native</c>
layout.
+ /// </description>
+ /// </item>
+ /// <item>
+ /// <description>
+ /// If a driver does not ship a <c>.deps.json</c>, the loader still
works but
+ /// falls back to whatever the host can resolve plus the driver's
directory.
+ /// Callers needing strong isolation should load the driver into a
custom
+ /// <see cref="AssemblyLoadContext"/> themselves.
+ /// </description>
+ /// </item>
+ /// </list>
+ /// </remarks>
+ internal static class ManagedDriverLoader
+ {
+#if NET6_0_OR_GREATER
+ private static readonly ConcurrentDictionary<string,
AssemblyDependencyResolver> resolvers
+ = new ConcurrentDictionary<string,
AssemblyDependencyResolver>(StringComparer.OrdinalIgnoreCase);
+
+ private static int s_resolveHookInstalled;
+
+ private static void EnsureResolveHookInstalled()
+ {
+ if (Interlocked.CompareExchange(ref s_resolveHookInstalled, 1, 0)
== 0)
+ {
+ AssemblyLoadContext.Default.Resolving +=
OnDefaultContextResolving;
+ }
+ }
+
+ [UnconditionalSuppressMessage("Trimming",
"IL2026:RequiresUnreferencedCode",
+ Justification = "Dynamic driver loading is inherently incompatible
with full trimming. " +
+ "Drivers must opt out of trimming or be preserved
by the host.")]
+ private static Assembly? OnDefaultContextResolving(AssemblyLoadContext
context, AssemblyName assemblyName)
+ {
+ foreach (AssemblyDependencyResolver resolver in resolvers.Values)
+ {
+ string? path = resolver.ResolveAssemblyToPath(assemblyName);
+ if (!string.IsNullOrEmpty(path) && File.Exists(path))
+ {
+ return context.LoadFromAssemblyPath(path!);
+ }
+ }
+ return null;
+ }
+#endif
+
+ /// <summary>
+ /// Loads a managed driver assembly from the specified path.
+ /// </summary>
+ /// <param name="assemblyPath">The path to the assembly to
load.</param>
+ /// <returns>The loaded assembly.</returns>
+ /// <exception cref="FileNotFoundException">The assembly file was not
found.</exception>
+ /// <exception cref="BadImageFormatException">The file is not a valid
managed assembly.</exception>
+ /// <remarks>
+ /// See the documentation on <see cref="ManagedDriverLoader"/> for
dependency
+ /// resolution semantics on .NET Framework versus modern .NET.
+ /// </remarks>
+#if NET6_0_OR_GREATER
+ [UnconditionalSuppressMessage("Trimming",
"IL2026:RequiresUnreferencedCode",
+ Justification = "Dynamic driver loading is inherently incompatible
with full trimming. " +
+ "Drivers must opt out of trimming or be preserved
by the host.")]
+#endif
+ internal static Assembly LoadAssembly(string assemblyPath)
+ {
+ string fullPath = Path.GetFullPath(assemblyPath);
+
+ if (!File.Exists(fullPath))
+ {
+ throw new FileNotFoundException($"Assembly not found:
{fullPath}", fullPath);
+ }
+
+#if NET6_0_OR_GREATER
+ // Register an AssemblyDependencyResolver for this driver so that
its own
+ // .deps.json is consulted when the default AssemblyLoadContext
fails to
+ // resolve a dependency. AssemblyDependencyResolver throws if no
.deps.json
+ // is present next to the assembly; in that case we silently fall
back to
+ // the host's normal probing behavior.
Review Comment:
I found that it was necessary to *always* reference the host's version of
the following three assemblies regardless of the driver's dependencies:
`Apache.Arrow`, `Apache.Arrow.Adbc` and `System.Diagnostics.DiagnosticSource`.
That's because these three assemblies contain types that are in the public ADBC
contracts. If the driver resolves to a different version of these assemblies
than the host, an attempt to use an API with one of these types will produce a
`MethodMissingException`.
A test which covers this scenario would need to have the driver built
against a different version of one or more of these than the test code is built
against.
##########
csharp/src/Apache.Arrow.Adbc/DriverManager/AdbcDriverManager.cs:
##########
@@ -0,0 +1,787 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Collections.Generic;
+using System.Globalization;
+using System.IO;
+using System.Reflection;
+using System.Runtime.InteropServices;
+#if NET6_0_OR_GREATER
+using System.Diagnostics.CodeAnalysis;
+#endif
+using Apache.Arrow.Adbc.C;
+
+namespace Apache.Arrow.Adbc.DriverManager
+{
+ /// <summary>
+ /// Provides methods to locate and load ADBC drivers, optionally using
TOML manifest
+ /// files and connection profiles.
+ /// Mirrors the free functions declared in <c>adbc_driver_manager.h</c>:
+ /// <c>AdbcLoadDriver</c> and <c>AdbcFindLoadDriver</c>.
+ /// </summary>
+ public static class AdbcDriverManager
+ {
+ /// <summary>
+ /// The environment variable that specifies additional driver search
paths.
+ /// </summary>
+ public const string DriverPathEnvVar = "ADBC_DRIVER_PATH";
+
+ private static readonly string[] s_nativeExtensions = { ".so", ".dll",
".dylib" };
+
+ //
-----------------------------------------------------------------------
+ // AdbcLoadDriver – load a driver directly from an absolute or
relative path
Review Comment:
Relative paths can be dangerous from a security perspective. What is the
path relative to? At first glance it appears that (at least sometimes) it's
relative to the current process directory, which I think is a very bad idea. If
we really want support for relative loading, then I think the best combination
of safety and usefulness is to make it relative to the location of the
executing process. But I'd rather force it to be absolute here and leave the
path resolution explicitly up to the host program.
I'd also be in favor of preventing a relative path from being applied to a
search path if the relative path contains any directory separators --
especially if it also contains `".."` segments.
##########
csharp/src/Apache.Arrow.Adbc/DriverManager/AdbcDriverManager.cs:
##########
@@ -0,0 +1,787 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Collections.Generic;
+using System.Globalization;
+using System.IO;
+using System.Reflection;
+using System.Runtime.InteropServices;
+#if NET6_0_OR_GREATER
+using System.Diagnostics.CodeAnalysis;
+#endif
+using Apache.Arrow.Adbc.C;
+
+namespace Apache.Arrow.Adbc.DriverManager
+{
+ /// <summary>
+ /// Provides methods to locate and load ADBC drivers, optionally using
TOML manifest
+ /// files and connection profiles.
+ /// Mirrors the free functions declared in <c>adbc_driver_manager.h</c>:
+ /// <c>AdbcLoadDriver</c> and <c>AdbcFindLoadDriver</c>.
+ /// </summary>
+ public static class AdbcDriverManager
+ {
+ /// <summary>
+ /// The environment variable that specifies additional driver search
paths.
+ /// </summary>
+ public const string DriverPathEnvVar = "ADBC_DRIVER_PATH";
+
+ private static readonly string[] s_nativeExtensions = { ".so", ".dll",
".dylib" };
Review Comment:
This appears to be unused.
##########
csharp/src/Apache.Arrow.Adbc/DriverManager/AdbcLoadFlags.cs:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+
+namespace Apache.Arrow.Adbc.DriverManager
+{
+ /// <summary>
+ /// Flags to control how the ADBC driver manager searches for and loads
drivers.
+ /// Mirrors the <c>AdbcLoadFlags</c> type and <c>ADBC_LOAD_FLAG_*</c>
constants
+ /// defined in <c>adbc_driver_manager.h</c>.
+ /// </summary>
+ [Flags]
+ public enum AdbcLoadFlags : uint
Review Comment:
Is there any specific reason to make this a `uint` enum rather than the
default?
##########
csharp/src/Apache.Arrow.Adbc/Apache.Arrow.Adbc.csproj:
##########
@@ -20,6 +20,11 @@
<Compile Remove="C\NativeLibrary.cs" />
</ItemGroup>
<ItemGroup>
+ <Content Include="DriverManager\readme.md">
+ <PackagePath>\</PackagePath>
Review Comment:
Does this preserve the subdirectory `DriverManager` or might it override the
other `readme.md`?
##########
csharp/src/Apache.Arrow.Adbc/DriverManager/TomlParser.cs:
##########
@@ -0,0 +1,178 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Collections.Generic;
+using System.Globalization;
+
+namespace Apache.Arrow.Adbc.DriverManager
+{
+ /// <summary>
+ /// A minimal TOML parser that handles the subset of TOML used by ADBC
driver
+ /// manifests and connection profiles:
+ /// <list type="bullet">
+ /// <item><description>Root-level key = value
assignments</description></item>
+ /// <item><description>Table section headers:
<c>[section]</c></description></item>
+ /// <item><description>String values (double-quoted), integer values,
floating-point
+ /// values, and boolean values
(<c>true</c>/<c>false</c>)</description></item>
+ /// <item><description>Line comments beginning with
<c>#</c></description></item>
+ /// </list>
+ /// This parser intentionally does not support the full TOML specification.
+ /// A full-featured TOML library (e.g. Tomlyn) was considered but cannot
be used here
+ /// because the assembly is strongly-named and Tomlyn does not publish a
strongly-named
+ /// package that is compatible with the project's pinned dependency
versions.
+ /// </summary>
+ internal static class TomlParser
+ {
+ private const string RootSection = "";
+
+ /// <summary>
+ /// Parses <paramref name="content"/> and returns a dictionary keyed
by section name.
+ /// Root-level keys are stored under the empty string key.
+ /// Values are typed as <see cref="string"/>, <see cref="long"/>, <see
cref="double"/>,
+ /// or <see cref="bool"/>.
+ /// </summary>
+ internal static Dictionary<string, Dictionary<string, object>>
Parse(string content)
+ {
+ if (content == null)
+ {
+ throw new ArgumentNullException(nameof(content));
+ }
+
+ Dictionary<string, Dictionary<string, object>> result = new
Dictionary<string, Dictionary<string, object>>(StringComparer.OrdinalIgnoreCase)
+ {
+ [RootSection] = new Dictionary<string,
object>(StringComparer.OrdinalIgnoreCase),
+ };
+
+ string currentSection = RootSection;
+
+ foreach (string rawLine in content.Split('\n'))
+ {
+ string line = StripComment(rawLine).Trim();
+
+ if (line.Length == 0)
+ {
+ continue;
+ }
+
+ if (line.StartsWith("[", StringComparison.Ordinal) &&
line.EndsWith("]", StringComparison.Ordinal))
Review Comment:
Should this throw an exception if the section name contains anything other
than `A-Za-z0-9_-`? I know we don't want or need a full-featured parser, but
perhaps we should try to be overly strict in what we accept rather than reading
something incorrectly.
##########
csharp/src/Apache.Arrow.Adbc/DriverManager/IConnectionProfile.cs:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Collections.Generic;
+
+namespace Apache.Arrow.Adbc.DriverManager
+{
+ /// <summary>
+ /// Represents an ADBC connection profile, which specifies a driver and a
set of
+ /// options to apply when initializing a database connection.
+ /// Mirrors the <c>AdbcConnectionProfile</c> struct defined in
<c>adbc_driver_manager.h</c>.
+ /// </summary>
+ /// <remarks>
+ /// The profile specifies a driver (optionally) and key/value options of
three types:
+ /// string, 64-bit integer, and double. String option values of the form
+ /// <c>env_var(ENV_VAR_NAME)</c> are expanded from the named environment
variable
+ /// before being applied to the database.
+ /// </remarks>
+ public interface IConnectionProfile : IDisposable
Review Comment:
Is there any real benefit to this being an interface versus a simple data
transfer object? It feels like the `IConnectionProfileProvider` ought to be
sufficient as an extensibility point without needing to also implement this
interface.
##########
csharp/src/Apache.Arrow.Adbc/DriverManager/AdbcDriverManager.cs:
##########
@@ -0,0 +1,787 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Collections.Generic;
+using System.Globalization;
+using System.IO;
+using System.Reflection;
+using System.Runtime.InteropServices;
+#if NET6_0_OR_GREATER
+using System.Diagnostics.CodeAnalysis;
+#endif
+using Apache.Arrow.Adbc.C;
+
+namespace Apache.Arrow.Adbc.DriverManager
+{
+ /// <summary>
+ /// Provides methods to locate and load ADBC drivers, optionally using
TOML manifest
+ /// files and connection profiles.
+ /// Mirrors the free functions declared in <c>adbc_driver_manager.h</c>:
+ /// <c>AdbcLoadDriver</c> and <c>AdbcFindLoadDriver</c>.
+ /// </summary>
+ public static class AdbcDriverManager
+ {
+ /// <summary>
+ /// The environment variable that specifies additional driver search
paths.
+ /// </summary>
+ public const string DriverPathEnvVar = "ADBC_DRIVER_PATH";
+
+ private static readonly string[] s_nativeExtensions = { ".so", ".dll",
".dylib" };
+
+ //
-----------------------------------------------------------------------
+ // AdbcLoadDriver – load a driver directly from an absolute or
relative path
+ //
-----------------------------------------------------------------------
+
+ /// <summary>
+ /// Loads an ADBC driver from an explicit file path.
+ /// Mirrors <c>AdbcLoadDriver</c> in <c>adbc_driver_manager.h</c>.
Review Comment:
It might be worth calling out that loading a managed driver always requires
a manifest.
--
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]
