feat!(ruby): set LoadFlags::DEFAULT on Database.new and allow override in open

[amoeba]

The current way to load an ADBC driver installed via ADBC driver manifest is cumbersome because the user has to do it over multiple calls and can't use the block-based Database.open if they need to customize init. This is currently what it looks like to load the sqlite driver installed via manfiest:

database = ADBC::Database.new

begin
  database.set_option("driver", "sqlite")
  database.set_option("uri", "games.sqlite")
  database.set_load_flags(ADBC::LoadFlags::DEFAULT)
...

It would be better if we could write:

ADBC::Database.open(driver: "sqlite", uri: "games.sqlite") do |database|
  ...
end

This PR does two things,

1. Set the default value of LoadFlags to ADBC_LOAD_FLAG_DEFAULT. This matches Python.
2. Adds a load_flags keyword arg to Database.open so the user can override it without having to drop down to Database.new

This is a breaking change because existing direct and indirect callers of Database.new will get different driver loading behavior.

[lidavidm]

Perhaps @otegami would also be interested?

[kou]

Thanks!

[kou]

In general, we should not replace new class method in Ruby. We should replace initialize instance method in Ruby.

BTW, how about setting the default load flags in GLib not Ruby?

diff --git a/glib/adbc-glib/database.c b/glib/adbc-glib/database.c
index 886b3474b..1225b3116 100644
--- a/glib/adbc-glib/database.c
+++ b/glib/adbc-glib/database.c
@@ -75,6 +75,11 @@ GADBCDatabase* gadbc_database_new(GError** error) {
   AdbcStatusCode status_code = AdbcDatabaseNew(&(priv->adbc_database), &adbc_error);
   priv->initialized =
       gadbc_error_check(error, status_code, &adbc_error, "[adbc][database][new]");
+  if (priv->initialized) {
+    status_code =
+      AdbcDriverManagerDatabaseSetLoadFlags(&(priv->adbc_database), GADBC_LOAD_FLAGS_DEFAULT, &adbc_error);
+    priv->initialized = gadbc_error_check(error, status_code, &adbc_error, "[adbc][database][set-load-flags]");
+  }
   if (!priv->initialized) {
     g_object_unref(database);
     return NULL;
diff --git a/glib/adbc-glib/database.h b/glib/adbc-glib/database.h
index 5f510b346..1c0464987 100644
--- a/glib/adbc-glib/database.h
+++ b/glib/adbc-glib/database.h
@@ -48,7 +48,7 @@ typedef enum {
 } GADBCLoadFlags;
 
 /**
- * GADBC_LOAD_FLAGAS_DEFAULT:
+ * GADBC_LOAD_FLAGS_DEFAULT:
  *
  * The default GADBCLoadFlags.
  *

[amoeba]

ah, thanks. I originally override initialize like,

  module ADBC
    class Database
      def initialize
        super
        set_load_flags(LoadFlags::DEFAULT)
      end
    end
  end

and the set_load_flags was failing with "database already released" so I think that's not doing what I thought it was. The approach with new fixed it.

Setting load_flags in GLib looks better so I'll do that. Thanks.

[kou]

Ah, we need to call the original initialize instead of super because Database#initialize is already defined (and this is not inherited class):

module ADBC
  class Database
    alias_method :initialize_raw, :initialize
    def initialize
      initialize_raw
      set_load_flags(LoadFlags::DEFAULT)
    end
  end
end

[amoeba]

Thanks for the review @kou, I think I've addressed everything and this is ready for another review.

[kou]

+1