fix(c/driver_manager): test and fix bugs in profiles (#4080)

- Fix substitution of undefined env var erasing the entire value instead
of substituting in a blank string.
- Improve error messages.
- Add end-to-end tests with Python.
- Add end-to-end tests in Conda/virtualenv.
- Add end-to-end tests with the user path (`~/.config`, etc.)

This is not complete; see #4082, #4085, #4086, #4087 for things that
also need to be fixed, but this establishes a baseline of tests.

Closes #4024.

Author: lidavidm

.github/workflows/native-unix.yml

@@ -589,6 +589,7 @@ jobs:
           ADBC_USE_UBSAN: "0"
         run: |
           export PATH=$RUNNER_TOOL_CACHE/go/${GO_VERSION}/x64/bin:$PATH
+          export _ADBC_IS_CONDA=1
           ./ci/scripts/python_build.sh "$(pwd)" "$(pwd)/build" "$HOME/local"
       - name: Build Panic Dummy
         run: |
@@ -603,6 +604,8 @@ jobs:
         run: |
           cargo build -padbc_dummy
       - name: Test Python Driver Manager
+        env:
+          PYTEST_ADDOPTS: "--run-system"
         run: |
           if [[ $(uname) = "Darwin" ]]; then
             export PANICDUMMY_LIBRARY_PATH=$(pwd)/go/adbc/pkg/libadbc_driver_panicdummy.dylib

.github/workflows/native-windows.yml

@@ -337,21 +337,15 @@ jobs:
         env:
           BUILD_ALL: "0"
           BUILD_DRIVER_MANAGER: "1"
-        run: .\ci\scripts\python_build.ps1 $pwd $pwd\build
-      - name: Build Python Driver PostgreSQL
-        env:
-          BUILD_ALL: "0"
           BUILD_DRIVER_POSTGRESQL: "1"
-        run: .\ci\scripts\python_build.ps1 $pwd $pwd\build
-      - name: Build Python Driver SQLite
-        env:
-          BUILD_ALL: "0"
           BUILD_DRIVER_SQLITE: "1"
+          _ADBC_IS_CONDA: "1"
         run: .\ci\scripts\python_build.ps1 $pwd $pwd\build
       - name: Test Python Driver Manager
         env:
           BUILD_ALL: "0"
           BUILD_DRIVER_MANAGER: "1"
+          PYTEST_ADDOPTS: "--run-system"
         run: .\ci\scripts\python_test.ps1 $pwd $pwd\build
       - name: Test Python Driver PostgreSQL
         env:

c/driver/framework/connection.h

@@ -165,7 +165,8 @@ class Connection : public ObjectBase {
       }
       return driver::Option();
     }
-    return Base::GetOption(key);
+    return status::NotImplemented(Derived::kErrorPrefix, " Unknown connection option ",
+                                  key);
   }
 
   /// \internal

c/driver/sqlite/sqlite.cc

@@ -52,6 +52,7 @@ constexpr std::string_view kConnectionOptionLoadExtensionEntrypoint =
 /// The batch size for query results (and for initial type inference)
 constexpr std::string_view kStatementOptionBatchRows = "adbc.sqlite.query.batch_rows";
 constexpr std::string_view kStatementOptionBindByName = "adbc.statement.bind_by_name";
+constexpr int kDefaultBatchSize = 1024;
 
 std::string_view GetColumnText(sqlite3_stmt* stmt, int index) {
   return {
@@ -547,6 +548,15 @@ class SqliteDatabase : public driver::Database<SqliteDatabase> {
     return Base::ReleaseImpl();
   }
 
+  Result<driver::Option> GetOption(std::string_view key) override {
+    if (key == "uri") {
+      return driver::Option(uri_);
+    } else if (key == kStatementOptionBatchRows) {
+      return driver::Option(static_cast<int64_t>(batch_size_));
+    }
+    return Base::GetOption(key);
+  }
+
   Status SetOptionImpl(std::string_view key, driver::Option value) override {
     if (key == "uri") {
       if (lifecycle_state_ != driver::LifecycleState::kUninitialized) {
@@ -556,13 +566,32 @@ class SqliteDatabase : public driver::Database<SqliteDatabase> {
       UNWRAP_RESULT(uri, value.AsString());
       uri_ = std::move(uri);
       return status::Ok();
+    } else if (key == kStatementOptionBatchRows) {
+      if (lifecycle_state_ != driver::LifecycleState::kUninitialized) {
+        return status::fmt::InvalidState(
+            "{} cannot set {} after AdbcDatabaseInit, set it directly on the statement "
+            "instead",
+            kErrorPrefix, key);
+      }
+      int64_t batch_size;
+      UNWRAP_RESULT(batch_size, value.AsInt());
+      if (batch_size <= 0 || batch_size > std::numeric_limits<int>::max()) {
+        return status::fmt::InvalidArgument(
+            "{} Invalid statement option value {}={} (value is non-positive or out of "
+            "range of int)",
+            kErrorPrefix, key, value.Format());
+      }
+      batch_size_ = static_cast<int>(batch_size);
+      return status::Ok();
     }
     return Base::SetOptionImpl(key, value);
   }
 
  private:
+  friend class SqliteConnection;
   std::string uri_{kDefaultUri};
   sqlite3* conn_ = nullptr;
+  int batch_size_ = kDefaultBatchSize;
 };
 
 class SqliteConnection : public driver::Connection<SqliteConnection> {
@@ -672,6 +701,7 @@ class SqliteConnection : public driver::Connection<SqliteConnection> {
   Status InitImpl(void* parent) {
     auto& db = *reinterpret_cast<SqliteDatabase*>(parent);
     UNWRAP_RESULT(conn_, db.OpenConnection());
+    batch_size_ = db.batch_size_;
     return status::Ok();
   }
 
@@ -693,6 +723,13 @@ class SqliteConnection : public driver::Connection<SqliteConnection> {
     return SqliteQuery::Execute(conn_, "BEGIN");
   }
 
+  Result<driver::Option> GetOption(std::string_view key) override {
+    if (key == kStatementOptionBatchRows) {
+      return driver::Option(static_cast<int64_t>(batch_size_));
+    }
+    return Base::GetOption(key);
+  }
+
   Status SetOptionImpl(std::string_view key, driver::Option value) {
     if (key == kConnectionOptionEnableLoadExtension) {
       if (!conn_ || lifecycle_state_ != driver::LifecycleState::kInitialized) {
@@ -761,6 +798,8 @@ class SqliteConnection : public driver::Connection<SqliteConnection> {
   }
 
  private:
+  friend class SqliteStatement;
+
   Status CheckOpen() const {
     if (!conn_) {
       return status::InvalidState("connection is not open");
@@ -772,6 +811,7 @@ class SqliteConnection : public driver::Connection<SqliteConnection> {
   // Temporarily hold the extension path (since the path and entrypoint need
   // to be set separately)
   std::string extension_path_;
+  int batch_size_ = kDefaultBatchSize;
 };
 
 class SqliteStatement : public driver::Statement<SqliteStatement> {
@@ -1111,7 +1151,9 @@ class SqliteStatement : public driver::Statement<SqliteStatement> {
   }
 
   Status InitImpl(void* parent) {
-    conn_ = reinterpret_cast<SqliteConnection*>(parent)->conn();
+    auto& conn = *reinterpret_cast<SqliteConnection*>(parent);
+    conn_ = conn.conn();
+    batch_size_ = conn.batch_size_;
     return Statement::InitImpl(parent);
   }
 
@@ -1151,7 +1193,16 @@ class SqliteStatement : public driver::Statement<SqliteStatement> {
     return Statement::ReleaseImpl();
   }
 
-  Status SetOptionImpl(std::string_view key, driver::Option value) {
+  Result<driver::Option> GetOption(std::string_view key) override {
+    if (key == kStatementOptionBatchRows) {
+      return driver::Option(static_cast<int64_t>(batch_size_));
+    } else if (key == kStatementOptionBindByName) {
+      return driver::Option(bind_by_name_ ? "true" : "false");
+    }
+    return Base::GetOption(key);
+  }
+
+  Status SetOptionImpl(std::string_view key, driver::Option value) override {
     if (key == kStatementOptionBatchRows) {
       int64_t batch_size;
       UNWRAP_RESULT(batch_size, value.AsInt());
@@ -1170,7 +1221,7 @@ class SqliteStatement : public driver::Statement<SqliteStatement> {
     return Base::SetOptionImpl(key, std::move(value));
   }
 
-  int batch_size_ = 1024;
+  int batch_size_ = kDefaultBatchSize;
   bool bind_by_name_ = false;
   AdbcSqliteBinder binder_;
   sqlite3* conn_ = nullptr;

c/driver_manager/adbc_driver_manager.cc

@@ -471,7 +471,7 @@ AdbcStatusCode InternalInitializeProfile(TempDatabase* args,
     // use try_emplace so we only add the option if there isn't
     // already an option with the same name
     std::string processed;
-    CHECK_STATUS(ProcessProfileValue(values[i], processed, error));
+    CHECK_STATUS(ProcessProfileValue(keys[i], values[i], processed, error));
     args->options.try_emplace(keys[i], processed);
   }
 

c/driver_manager/adbc_driver_manager_internal.h

@@ -189,8 +189,8 @@ AdbcStatusCode LoadDriverFromRegistry(HKEY root, const std::wstring& driver_name
 #endif
 
 // Profile loading
-AdbcStatusCode ProcessProfileValue(std::string_view value, std::string& out,
-                                   struct AdbcError* error);
+AdbcStatusCode ProcessProfileValue(std::string_view key, std::string_view value,
+                                   std::string& out, struct AdbcError* error);
 
 // Initialization
 /// Temporary state while the database is being configured.