expose more APIs

[cocoa-xu]

Based on the design of ADBC, we probably have to expose more APIs, in particular these {get,set}_option functions. This PR added the following functions

Adbc.Database.get_option/2
Adbc.Database.set_option/3
Adbc.Connection.get_option/2
Adbc.Connection.set_option/3
Adbc.Connection.query_with_options/4
Adbc.Connection.query_with_options!/4

Adbc.Connection.query_with_options/4 will call Adbc.Nif.adbc_statement_set_option/3 in create_statement. Users can now send query with statement options using:

db = start_supervised!({Adbc.Database, driver: :some_driver})
conn = start_supervised!({Connection, database: db})

# with parameters
query_result = Connection.query_with_options(conn, 
  "SOME SQL STATEMENT BIND WITH ?", 
  ["PARAMETERS"],
  statement_option_a: "value for a",
  statement_option_b: "value for b",
)

# without parameters
query_result = Connection.query_with_options(conn, 
  "SOME SQL STATEMENT",
  statement_option_a: "value for a",
  statement_option_b: "value for b",
)

[josevalim]

Looks great to me I would just nitpick on the function names:

get_string_option
get_float_option
get_integer_option
get_binary_option

Similar for set. Those would align with Elixir naming and conventions. :) Thank you!

[cocoa-xu]

Looks great to me I would just nitpick on the function names:

get_string_option
get_float_option
get_integer_option
get_binary_option

Similar for set. Those would align with Elixir naming and conventions. :) Thank you!

No problem, I'll update these names and fix the test in test/adbc_connection_test.exs:96, and we should be ready to merge it!

[cocoa-xu]

I've done renaming these functions, but I'm not sure if we should leave Adbc.{Database,Connection}.set_option/3 because we can pattern match the value in Adbc.Helper.set_option/4.

It could be convenient maybe... What do you think?

[josevalim]

I've done renaming these functions, but I'm not sure if we should leave Adbc.{Database,Connection}.set_option/3 because we can pattern match the value in Adbc.Helper.set_option/4.

Actually, please ignore both of my comments above (I have deleted them).

I think keeping only set_option would be nice, and we use {:binary ...} (similar to what you did) to handle binaries. This keeps the API surface smaller and also works nicely with our existing keyword API that we already have today (when starting the connection/database).

However, I don't think we can apply this to get_option, can we? In that case, my suggestion for now would be:

• Remove all set_*_option and rely on set_option

• Streamline the get_*_option implementation. The call to the GenServer could be done like this:

  GenServer.call({:get, :adbc_connection_get_string_option, to_string(key)})
  

  And then in the GenServer you call the NIF

  def handle_call({:get, fun, key}, _from, state) do
    case apply(Adbc.NIF, fun, [state.ref, key]) do
      ...
    end
  end
  

  This way you can skip all of the helpers in adbc_helper? It would also be nice to make the NIF name consistent too: adbc_connection_get_string_option and so forth. :)

[cocoa-xu]

Got it :) I also thought that this would be the better way (although the user may have to explicitly use {:binary, data} if they want to set a bytes type option).

[cocoa-xu]

I refactored it a bit so we now have less repetition in both Elixir and C++.

[josevalim]

It looks glorious, I have dropped just one last comment. :)

[josevalim]

I think we can delete the clause above now, no?

Also, should we allow statement_options as optional argument to prepare?

[cocoa-xu]

Ah I finally found the above one which was used in query_pointer, and I've added statement_options to query_pointer.

[cocoa-xu]

I tested it with explorer, and it seems that we're okay! (Although we will need to add statement options to Explorer.DataFrame.from_query)