fix some TODOs

c/cmake_modules/AdbcDefines.cmake

@@ -85,11 +85,13 @@ if(MSVC)
 elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang"
        OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang"
        OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
+  # maybe-uninitialized is flaky
   set(ADBC_C_CXX_FLAGS_CHECKIN
       -Wall
       -Wextra
       -Wpedantic
       -Werror
+      -Wno-maybe-uninitialized
       -Wno-unused-parameter)
   set(ADBC_C_CXX_FLAGS_PRODUCTION -Wall)
 

c/driver/framework/base_connection.h

@@ -21,6 +21,7 @@
 #include <memory>
 #include <string>
 #include <string_view>
+#include <utility>
 #include <vector>
 
 #include <adbc.h>
@@ -34,29 +35,39 @@
 #include "driver/framework/objects.h"
 
 namespace adbc::driver {
-
+/// \brief The CRTP base implementation of an AdbcConnection.
+///
+/// Derived should override and implement the Impl methods, but not others.
+/// Overridden methods should defer to the superclass version at the end.
+/// (The Base typedef is provided to make this easier.)  Derived should also
+/// define a constexpr static symbol called kErrorPrefix that is used to
+/// construct error messages.
 template <typename Derived>
 class ConnectionBase : public ObjectBase {
  public:
   using Base = ConnectionBase<Derived>;
 
+  /// \brief Whether autocommit is enabled or not (by default: enabled).
   enum class AutocommitState {
     kAutocommit,
     kTransaction,
   };
 
-  ConnectionBase() : ObjectBase(), lifecycle_state_(LifecycleState::kUninitialized) {}
+  ConnectionBase() : ObjectBase() {}
+  ~ConnectionBase() = default;
 
+  /// \internal
   AdbcStatusCode Init(void* parent, AdbcError* error) override {
-    lifecycle_state_ = LifecycleState::kInitialized;
     if (auto status = impl().InitImpl(parent); !status.ok()) {
       return status.ToAdbc(error);
     }
     return ObjectBase::Init(parent, error);
   }
 
+  /// \internal
   AdbcStatusCode Cancel(AdbcError* error) { return ADBC_STATUS_NOT_IMPLEMENTED; }
 
+  /// \internal
   AdbcStatusCode Commit(AdbcError* error) {
     switch (autocommit_) {
       case AutocommitState::kAutocommit:
@@ -70,6 +81,7 @@ class ConnectionBase : public ObjectBase {
     return ADBC_STATUS_INTERNAL;
   }
 
+  /// \internal
   AdbcStatusCode GetInfo(const uint32_t* info_codes, size_t info_codes_length,
                          ArrowArrayStream* out, AdbcError* error) {
     std::vector<uint32_t> codes(info_codes, info_codes + info_codes_length);
@@ -92,7 +104,7 @@ class ConnectionBase : public ObjectBase {
                 } else {
                   static_assert(!sizeof(T), "info value type not implemented");
                 }
-                return status::kOk;
+                return status::Ok();
               },
               info.value));
       CHECK_NA(INTERNAL, ArrowArrayFinishElement(array.get()), error);
@@ -104,6 +116,7 @@ class ConnectionBase : public ObjectBase {
     return BatchToArrayStream(array.get(), schema.get(), out, error);
   }
 
+  /// \internal
   AdbcStatusCode GetObjects(int depth, const char* catalog, const char* db_schema,
                             const char* table_name, const char** table_type,
                             const char* column_name, ArrowArrayStream* out,
@@ -136,7 +149,8 @@ class ConnectionBase : public ObjectBase {
     return BatchToArrayStream(array.get(), schema.get(), out, error);
   }
 
-  Result<std::optional<Option>> GetOption(std::string_view key) const override {
+  /// \internal
+  Result<Option> GetOption(std::string_view key) override {
     if (key == ADBC_CONNECTION_OPTION_AUTOCOMMIT) {
       switch (autocommit_) {
         case AutocommitState::kAutocommit:
@@ -145,24 +159,34 @@ class ConnectionBase : public ObjectBase {
           return driver::Option(ADBC_OPTION_VALUE_DISABLED);
       }
     } else if (key == ADBC_CONNECTION_OPTION_CURRENT_CATALOG) {
-      // TODO: defer to impl
-      return driver::Option("main");
+      UNWRAP_RESULT(auto catalog, impl().GetCurrentCatalogImpl());
+      if (catalog) {
+        return driver::Option(std::move(*catalog));
+      }
+      return driver::Option();
     } else if (key == ADBC_CONNECTION_OPTION_CURRENT_DB_SCHEMA) {
+      UNWRAP_RESULT(auto schema, impl().GetCurrentSchemaImpl());
+      if (schema) {
+        return driver::Option(std::move(*schema));
+      }
       return driver::Option();
     }
-    return std::nullopt;
+    return Base::GetOption(key);
   }
 
+  /// \internal
   AdbcStatusCode GetStatistics(const char* catalog, const char* db_schema,
                                const char* table_name, char approximate,
                                ArrowArrayStream* out, AdbcError* error) {
     return ADBC_STATUS_NOT_IMPLEMENTED;
   }
 
+  /// \internal
   AdbcStatusCode GetStatisticNames(ArrowArrayStream* out, AdbcError* error) {
     return ADBC_STATUS_NOT_IMPLEMENTED;
   }
 
+  /// \internal
   AdbcStatusCode GetTableSchema(const char* catalog, const char* db_schema,
                                 const char* table_name, ArrowSchema* schema,
                                 AdbcError* error) {
@@ -183,6 +207,7 @@ class ConnectionBase : public ObjectBase {
         .ToAdbc(error);
   }
 
+  /// \internal
   AdbcStatusCode GetTableTypes(ArrowArrayStream* out, AdbcError* error) {
     RAISE_RESULT(error, std::vector<std::string> table_types, impl().GetTableTypesImpl());
 
@@ -216,16 +241,19 @@ class ConnectionBase : public ObjectBase {
     return BatchToArrayStream(array.get(), schema.get(), out, error);
   }
 
+  /// \internal
   AdbcStatusCode ReadPartition(const uint8_t* serialized_partition,
                                size_t serialized_length, ArrowArrayStream* out,
                                AdbcError* error) {
     return ADBC_STATUS_NOT_IMPLEMENTED;
   }
 
+  /// \internal
   AdbcStatusCode Release(AdbcError* error) override {
     return impl().ReleaseImpl().ToAdbc(error);
   }
 
+  /// \internal
   AdbcStatusCode Rollback(AdbcError* error) {
     switch (autocommit_) {
       case AutocommitState::kAutocommit:
@@ -239,13 +267,27 @@ class ConnectionBase : public ObjectBase {
     return ADBC_STATUS_INTERNAL;
   }
 
+  /// \internal
   AdbcStatusCode SetOption(std::string_view key, Option value,
                            AdbcError* error) override {
     return impl().SetOptionImpl(key, value).ToAdbc(error);
   }
 
+  /// \brief Commit the current transaction and begin a new transaction.
+  ///
+  /// Only called when autocommit is disabled.
   Status CommitImpl() { return status::NotImplemented("Commit"); }
 
+  Result<std::optional<std::string>> GetCurrentCatalogImpl() { return std::nullopt; }
+
+  Result<std::optional<std::string>> GetCurrentSchemaImpl() { return std::nullopt; }
+
+  /// \brief Query the database catalog.
+  ///
+  /// The default implementation assumes the underlying database allows
+  /// querying the catalog in a certain manner, embodied in the helper class
+  /// returned here.  If the database can directly implement GetObjects, then
+  /// directly override GetObjects instead of using this helper.
   Result<std::unique_ptr<GetObjectsHelper>> GetObjectsImpl() {
     return std::make_unique<GetObjectsHelper>();
   }
@@ -264,9 +306,9 @@ class ConnectionBase : public ObjectBase {
     return std::vector<InfoValue>{};
   }
 
-  Status InitImpl(void* parent) { return status::kOk; }
+  Status InitImpl(void* parent) { return status::Ok(); }
 
-  Status ReleaseImpl() { return status::kOk; }
+  Status ReleaseImpl() { return status::Ok(); }
 
   Status RollbackImpl() { return status::NotImplemented("Rollback"); }
 
@@ -289,7 +331,7 @@ class ConnectionBase : public ObjectBase {
           break;
         }
       }
-      return status::kOk;
+      return status::Ok();
     }
     return status::NotImplemented("{} Unknown connection option {}={}",
                                   Derived::kErrorPrefix, key, value);
@@ -301,7 +343,6 @@ class ConnectionBase : public ObjectBase {
 
  protected:
   AutocommitState autocommit_ = AutocommitState::kAutocommit;
-  LifecycleState lifecycle_state_;
 
  private:
   Derived& impl() { return static_cast<Derived&>(*this); }

c/driver/framework/base_database.h

@@ -38,11 +38,11 @@ class DatabaseBase : public ObjectBase {
  public:
   using Base = DatabaseBase<Derived>;
 
-  DatabaseBase() : ObjectBase(), lifecycle_state_(LifecycleState::kUninitialized) {}
+  DatabaseBase() : ObjectBase() {}
+  ~DatabaseBase() = default;
 
   /// \internal
   AdbcStatusCode Init(void* parent, AdbcError* error) override {
-    lifecycle_state_ = LifecycleState::kInitialized;
     if (auto status = impl().InitImpl(); !status.ok()) {
       return status.ToAdbc(error);
     }
@@ -61,20 +61,17 @@ class DatabaseBase : public ObjectBase {
   }
 
   /// \brief Initialize the database.
-  virtual Status InitImpl() { return status::kOk; }
+  virtual Status InitImpl() { return status::Ok(); }
 
   /// \brief Release the database.
-  virtual Status ReleaseImpl() { return status::kOk; }
+  virtual Status ReleaseImpl() { return status::Ok(); }
 
   /// \brief Set an option.  May be called prior to InitImpl.
   virtual Status SetOptionImpl(std::string_view key, Option value) {
     return status::NotImplemented("{} Unknown database option {}={}",
                                   Derived::kErrorPrefix, key, value);
   }
 
- protected:
-  LifecycleState lifecycle_state_;
-
  private:
   Derived& impl() { return static_cast<Derived&>(*this); }
 };

c/driver/framework/base_statement.h

@@ -103,9 +103,10 @@ class StatementBase : public ObjectBase {
   /// \brief Statement state: one of the above.
   using State = std::variant<EmptyState, IngestState, PreparedState, QueryState>;
 
-  StatementBase() : ObjectBase(), lifecycle_state_(LifecycleState::kUninitialized) {
+  StatementBase() : ObjectBase() {
     std::memset(&bind_parameters_, 0, sizeof(bind_parameters_));
   }
+  ~StatementBase() = default;
 
   AdbcStatusCode Bind(ArrowArray* values, ArrowSchema* schema, AdbcError* error) {
     if (!values || !values->release) {
@@ -245,11 +246,11 @@ class StatementBase : public ObjectBase {
                                     Derived::kErrorPrefix);
                               } else if constexpr (std::is_same_v<T, PreparedState>) {
                                 // No-op
-                                return status::kOk;
+                                return status::Ok();
                               } else if constexpr (std::is_same_v<T, QueryState>) {
                                 UNWRAP_STATUS(impl().PrepareImpl(state));
                                 state_ = PreparedState{std::move(state.query)};
-                                return status::kOk;
+                                return status::Ok();
                               } else {
                                 static_assert(!sizeof(T), "case not implemented");
                               }
@@ -298,12 +299,20 @@ class StatementBase : public ObjectBase {
       }
       return ADBC_STATUS_OK;
     } else if (key == ADBC_INGEST_OPTION_TARGET_CATALOG) {
-      RAISE_RESULT(error, auto catalog, value.AsString());
-      ensure_ingest().target_catalog = catalog;
+      if (value.has_value()) {
+        RAISE_RESULT(error, auto catalog, value.AsString());
+        ensure_ingest().target_catalog = catalog;
+      } else {
+        ensure_ingest().target_catalog = std::nullopt;
+      }
       return ADBC_STATUS_OK;
     } else if (key == ADBC_INGEST_OPTION_TARGET_DB_SCHEMA) {
-      RAISE_RESULT(error, auto schema, value.AsString());
-      ensure_ingest().target_schema = schema;
+      if (value.has_value()) {
+        RAISE_RESULT(error, auto schema, value.AsString());
+        ensure_ingest().target_schema = schema;
+      } else {
+        ensure_ingest().target_schema = std::nullopt;
+      }
       return ADBC_STATUS_OK;
     } else if (key == ADBC_INGEST_OPTION_TARGET_TABLE) {
       RAISE_RESULT(error, auto table, value.AsString());
@@ -325,20 +334,20 @@ class StatementBase : public ObjectBase {
                                 state_ = QueryState{
                                     std::string(query),
                                 };
-                                return status::kOk;
+                                return status::Ok();
                               } else if constexpr (std::is_same_v<T, IngestState>) {
                                 state_ = QueryState{
                                     std::string(query),
                                 };
-                                return status::kOk;
+                                return status::Ok();
                               } else if constexpr (std::is_same_v<T, PreparedState>) {
                                 state_ = QueryState{
                                     std::string(query),
                                 };
-                                return status::kOk;
+                                return status::Ok();
                               } else if constexpr (std::is_same_v<T, QueryState>) {
                                 state.query = std::string(query);
-                                return status::kOk;
+                                return status::Ok();
                               } else {
                                 static_assert(!sizeof(T),
                                               "info value type not implemented");
@@ -382,21 +391,20 @@ class StatementBase : public ObjectBase {
                                   Derived::kErrorPrefix);
   }
 
-  Status InitImpl(void* parent) { return status::kOk; }
+  Status InitImpl(void* parent) { return status::Ok(); }
 
   Status PrepareImpl(QueryState& state) {
     return status::NotImplemented("{} Prepare is not implemented", Derived::kErrorPrefix);
   }
 
-  Status ReleaseImpl() { return status::kOk; }
+  Status ReleaseImpl() { return status::Ok(); }
 
   Status SetOptionImpl(std::string_view key, Option value) {
     return status::NotImplemented("{} Unknown statement option {}={}",
                                   Derived::kErrorPrefix, key, value);
   }
 
  protected:
-  LifecycleState lifecycle_state_;
   ArrowArrayStream bind_parameters_;
 
  private:

c/driver/framework/catalog.cc

@@ -73,7 +73,7 @@ Status AdbcInitConnectionGetInfoSchema(struct ArrowSchema* schema,
                    ArrowArrayInitFromSchema(array, schema, &na_error));
   UNWRAP_ERRNO(Internal, ArrowArrayStartAppending(array));
 
-  return status::kOk;
+  return status::Ok();
 }
 
 Status AdbcConnectionGetInfoAppendString(struct ArrowArray* array, uint32_t info_code,
@@ -86,7 +86,7 @@ Status AdbcConnectionGetInfoAppendString(struct ArrowArray* array, uint32_t info
   UNWRAP_ERRNO(Internal, ArrowArrayAppendString(array->children[1]->children[0], value));
   // Append type code/offset
   UNWRAP_ERRNO(Internal, ArrowArrayFinishUnionElement(array->children[1], /*type_id=*/0));
-  return status::kOk;
+  return status::Ok();
 }
 
 Status AdbcConnectionGetInfoAppendInt(struct ArrowArray* array, uint32_t info_code,
@@ -97,7 +97,7 @@ Status AdbcConnectionGetInfoAppendInt(struct ArrowArray* array, uint32_t info_co
                ArrowArrayAppendInt(array->children[1]->children[2], info_value));
   // Append type code/offset
   UNWRAP_ERRNO(Internal, ArrowArrayFinishUnionElement(array->children[1], /*type_id=*/2));
-  return status::kOk;
+  return status::Ok();
 }
 
 Status AdbcInitConnectionObjectsSchema(struct ArrowSchema* schema) {
@@ -258,6 +258,6 @@ Status AdbcInitConnectionObjectsSchema(struct ArrowSchema* schema) {
   UNWRAP_ERRNO(Internal, ArrowSchemaSetName(usage_schema->children[3], "fk_column_name"));
   usage_schema->children[3]->flags &= ~ARROW_FLAG_NULLABLE;
 
-  return status::kOk;
+  return status::Ok();
 }
 }  // namespace adbc::driver