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.
  *

[kou]

We can simplify this:

Suggested change

	def setup
	@tmpdir = Dir.mktmpdir
	File.write(File.join(@tmpdir, "testdriver.toml"), <<~TOML)
	name = "test driver"
	version = "0.1.0"
	[Driver]
	shared = "adbc_driver_sqlite"
	TOML
	@original_driver_path = ENV["ADBC_DRIVER_PATH"]
	ENV["ADBC_DRIVER_PATH"] = @tmpdir
	end
	def teardown
	ENV["ADBC_DRIVER_PATH"] = @original_driver_path
	FileUtils.remove_entry(@tmpdir)
	end
	def setup
	Dir.mktmpdir do |tmpdir|
	File.write(File.join(tmpdir, "testdriver.toml"), <<~TOML)
	name = "test driver"
	version = "0.1.0"
	[Driver]
	shared = "adbc_driver_sqlite"
	TOML
	driver_path, ENV["ADBC_DRIVER_PATH"] = ENV["ADBC_DRIVER_PATH"], tmpdir
	begin
	yield
	ensure
	ENV["ADBC_DRIVER_PATH"] = driver_path
	end
	end
	end

[kou]

We don't need this because other test such as

arrow-adbc/ruby/test/test-connection.rb

Lines 24 to 30 in cd6e396

	ADBC::Database.open(**options) do |database|
	connect_options = {}
	database.connect(**connect_options) do |connection|
	@connection = connection
	yield
	end
	end

covers this.

[kou]

Do we need this .to_s?

[kou]

We can remove this because manifest test case covers this case.

[kou]

assert_nothing_raised is meaningless. Let's use a real assertion:

Suggested change

	assert_nothing_raised do
	ADBC::Database.open(driver: "testdriver",
	uri: ":memory:",
	load_flags: ADBC::LoadFlags::SEARCH_ENV) do |_database|
	end
	end
	ADBC::Database.open(driver: "testdriver",
	uri: ":memory:",
	load_flags: ADBC::LoadFlags::SEARCH_ENV) do |database|
	database.connect do |connection|
	assert_equal([
	Arrow::Table.new("1" => Arrow::Int64Array.new([1])),
	-1,
	],
	connection.query("SELECT 1"))
	end
	end

[kou]

We can simplify this:

Suggested change

	load_flags: ADBC::LoadFlags.new(0)) do |_database|
	load_flags: 0) do |_database|

[kou]

How about asserting more specific error class instead of error message?

Suggested change

	error = assert_raise_kind_of(ADBC::Error) do
	ADBC::Database.open(driver: "testdriver",
	uri: ":memory:",
	load_flags: ADBC::LoadFlags.new(0)) do |_database|
	end
	end
	assert_match(/not enabled at run time/, error.message)
	assert_raise(ADBC::Error::NotFound) do
	ADBC::Database.open(driver: "testdriver",
	uri: ":memory:",
	load_flags: ADBC::LoadFlags.new(0)) do |_database|
	end
	end