Improve GADBC API to be GObject Introspection friendly

[esodan]

The C API could be correct and useful as is, but when switch to Vala using void* doesn't feet Vala's automatic memory management, as we need to tell it if the pointer should or not be freed. In other words void* means manual memory management, and that push back the advantage of Vala for GObject.

For GADBC.Statement, currently there are this API:

gboolean gadbc_statement_bind(GADBCStatement* statement, gpointer c_abi_array,
                              gpointer c_abi_schema, GError** error);

gboolean gadbc_statement_bind_stream(GADBCStatement* statement,
                                     gpointer c_abi_array_stream, GError** error)

The C comments declares gpointer c_abi_array and gpointer c_abi_array_stream as out parameters, but the pointer in not a pointer to pointer (void**), so should not be declared as it.

But when looking to a better GObject API, the C API should be:

gboolean gadbc_statement_bind(GADBCStatement* statement, GArrow.Array *array,
                              GArrow.Schema *schema, GError** error);

gboolean gadbc_statement_bind_stream(GADBCStatement* statement,
                                     GArrow.InputStream *stream, GError** error)

Internally, the above statements should use *_get_raw() methods to get the native Arrow structures and fill out them with the provided data.

I'm not quite sure if c_abi_array_stream is a GArrow.InputStream or an GArrow.OutputStream, so please guide me here, because there is not a generic GArrow.Stream to use here.

This issue is affecting #1152 because the current API forces to manual memory management, that is not a good idea, both in C and in Vala.

[kou]

I don't want to add hard Apache Arrow GLib (that depends on Apache Arrow C++) dependency to ADBC GLib.
Because ADBC itself doesn't depend on Apache Arrow C++.

Apache Arrow C data/stream interfaces ( https://arrow.apache.org/docs/format/CDataInterface.html / https://arrow.apache.org/docs/format/CStreamInterface.html ) are interfaces to avoid the hard dependency.

Can we provide a convenient wrapper in Vala layer instead like we did for the Ruby bindings?
https://github.com/apache/arrow-adbc/blob/main/ruby/lib/adbc/statement.rb#L36-L60

[esodan]

Vala produce C code, so if we wraps but use symbols from other libraries, they depends on them; in this case Ruby use Arrow::RecordBatchReader.import so in C this symbol should be linked against the library providing it. GADBC avoids this by using void* but that is hard to bind because there is not confidence about its type and how handle it for memory management.

Then GADBC requires to add objects to handle data returned by queries, streams and data structures, but that is the job of Arrow I think; so, make depends GADBC to Arrow should be a natural step.

I see GADBC as the bridge between Data Sources and Data Structures and Streams to handle them, where the data types and streams are provided by Apache Arrow, while the data source driver translate his own format to Arrow.

[esodan]

I'm writting a library called libvda in Vala, it will use GADBC to access Databases using its drivers. This could be the wrapper we are talking about. Internally libvda could wrap GADBC API and Arrow to access the data in the way more convenient for Vala programmers.

Please help me to merge my PR #1152 so any one can use Vala API as is currently, including libvda.

[kou]

It's OK that the Ruby bindings and the Vala API depends on Apache Arrow GLib because Apache Arrow GLib is the de fact library to process Apache Arrow data in Ruby and Vala. But I want to avoid that ADBC GLib depends on Apache Arrow GLib because there are many options to process Apache Arrow data for C users.

Can I push some changes to your branch directly to merge #1152 as soon as possible?
I think that we need more fixes to merge #1152. For example, there are wrong indentations you wasn't aware ( https://github.com/apache/arrow-adbc/pull/1152/files#diff-5a7fbf2bb50697b9dd6dbdceabd575237c146f31c737823f74f4a4d9ac1a39d5R107-R109 and https://github.com/apache/arrow-adbc/pull/1152/files#diff-0cb672ece56343cb6a81db58840a92428447da01eaf28c8725ca0e88a1d42780R44 ).

[esodan]

It's OK that the Ruby bindings and the Vala API depends on Apache Arrow GLib because Apache Arrow GLib is the de fact library to process Apache Arrow data in Ruby and Vala. But I want to avoid that ADBC GLib depends on Apache Arrow GLib because there are many options to process Apache Arrow data for C users.

Maybe I can use Metadata to generate the required fixes on Vala API, but if VAPI is generated is possible that the development package will depend on Arrow development package, to provide de GIR/VAPI required by this bindings. Is this acceptable to you?

Can I push some changes to your branch directly to merge #1152 as soon as possible? I think that we need more fixes to merge #1152. For example, there are wrong indentations you wasn't aware ( https://github.com/apache/arrow-adbc/pull/1152/files#diff-5a7fbf2bb50697b9dd6dbdceabd575237c146f31c737823f74f4a4d9ac1a39d5R107-R109 and https://github.com/apache/arrow-adbc/pull/1152/files#diff-0cb672ece56343cb6a81db58840a92428447da01eaf28c8725ca0e88a1d42780R44 ).

Yes please! Feel free to do any thing you consider is better for GADBC, thanks in advance.

[esodan]

So as for other languages, having its own directory, I can add a folder to create a pure GObject/Vala bindings library, with interfaces and classes to make easy to use ADBC with GObject (C)/Vala and any other language using GObject-Introspection, is it that good for you? @kou

The new library could be called arrow-adbc with a namespace GArrowAdbc; this will help to develop software using Arrow and ADBC but with a GObject/OOP API.

[kou]

Does this mean that you want to implement a new GObject based library with Apache Arrow GLib dependency from scratch?

[esodan]

Does this mean that you want to implement a new GObject based library with Apache Arrow GLib dependency from scratch?

No really. It means I will reuse the existing API and wrap it to one providing GObject one but GObject Introspection friendly. It will depend on ADBC and Arrow to provide Database access and data handling. I don’t plan to add new, for example, Connection object but a derived one providing the above API, just for the ones that use void* or gpointer, because they are not GObject Introspection friendly.

I think that your current API, if is GLib/GObject and depending on GObject Introspection, should be GObject Introspection friendly.

[esodan]

Does this mean that you want to implement a new GObject based library with Apache Arrow GLib dependency from scratch?

No really. It means I will reuse the existing API and wrap it to one providing GObject one but GObject Introspection friendly. It will depend on ADBC and Arrow to provide Database access and data handling. I don’t plan to add new, for example, Connection object but a derived one providing the above API, just for the ones that use void* or gpointer, because they are not GObject Introspection friendly.

I think that your current API, if is GLib/GObject and depending on GObject Introspection, should be GObject Introspection friendly.

Also, if some generic is required, may ADBC API should use a GObject * and use macros to know the type of the object before use it.

[kou]

I see. Then we don't need to create a new top-level directory. We can just create a new sub-directory under glib/:

diff --git a/glib/example/meson.build b/glib/example/meson.build
index ed34fd02..41c9ab23 100644
--- a/glib/example/meson.build
+++ b/glib/example/meson.build
@@ -18,7 +18,6 @@
 # under the License.
 
 if build_example
-  arrow_glib = dependency('arrow-glib')
   executable('sqlite', 'sqlite.c',
              dependencies: [adbc_glib, arrow_glib],
              link_language: 'c')
diff --git a/glib/meson.build b/glib/meson.build
index 0deee035..a7b716c5 100644
--- a/glib/meson.build
+++ b/glib/meson.build
@@ -78,6 +78,13 @@ if generate_vapi
 endif
 
 subdir('adbc-glib')
+build_arrow_integration = get_option('arrow')
+if build_arrow_integration or build_example
+  arrow_glib = dependency('arrow-glib')
+endif
+if build_arrow_integration
+  subdir('adbc-arrow-glib')
+endif
 subdir('example')
 
 install_data('../LICENSE.txt',
diff --git a/glib/meson_options.txt b/glib/meson_options.txt
index 8603b783..39a318c5 100644
--- a/glib/meson_options.txt
+++ b/glib/meson_options.txt
@@ -22,6 +22,11 @@ option('adbc_build_dir',
        value: '',
        description: 'Use this option to build with not installed ADBC')
 
+option('arrow',
+       type: 'boolean',
+       value: false,
+       description: 'Enable Apache Arrow GLib integration')
+
 option('example',
        type: 'boolean',
        value: false,

If we provide a Apache Arrow GLib integration library, we should use GArrowRecordBatch *, GArrowSchema * and GArrowRecordBatchReader * instead of gpointer c_abi_array, gpointer c_abi_schema and gpointer c_abi_array_stream. (We should call garrow_record_batch_import(), garrow_schema_import() and garrow_record_batch_reader_import() internally.)

[esodan]

Checking out glib/test/statement.rb

I see. Then we don't need to create a new top-level directory. We can just create a new sub-directory under glib/:

diff --git a/glib/example/meson.build b/glib/example/meson.build
index ed34fd02..41c9ab23 100644
--- a/glib/example/meson.build
+++ b/glib/example/meson.build
@@ -18,7 +18,6 @@
 # under the License.
 
 if build_example
-  arrow_glib = dependency('arrow-glib')
   executable('sqlite', 'sqlite.c',
              dependencies: [adbc_glib, arrow_glib],
              link_language: 'c')
diff --git a/glib/meson.build b/glib/meson.build
index 0deee035..a7b716c5 100644
--- a/glib/meson.build
+++ b/glib/meson.build
@@ -78,6 +78,13 @@ if generate_vapi
 endif
 
 subdir('adbc-glib')
+build_arrow_integration = get_option('arrow')
+if build_arrow_integration or build_example
+  arrow_glib = dependency('arrow-glib')
+endif
+if build_arrow_integration
+  subdir('adbc-arrow-glib')
+endif
 subdir('example')
 
 install_data('../LICENSE.txt',
diff --git a/glib/meson_options.txt b/glib/meson_options.txt
index 8603b783..39a318c5 100644
--- a/glib/meson_options.txt
+++ b/glib/meson_options.txt
@@ -22,6 +22,11 @@ option('adbc_build_dir',
        value: '',
        description: 'Use this option to build with not installed ADBC')
 
+option('arrow',
+       type: 'boolean',
+       value: false,
+       description: 'Enable Apache Arrow GLib integration')
+
 option('example',
        type: 'boolean',
        value: false,

If we provide a Apache Arrow GLib integration library, we should use GArrowRecordBatch *, GArrowSchema * and GArrowRecordBatchReader * instead of gpointer c_abi_array, gpointer c_abi_schema and gpointer c_abi_array_stream. (We should call garrow_record_batch_import(), garrow_schema_import() and garrow_record_batch_reader_import() internally.)

Great! Now which should be the name space of this new library?

• GADBC - Keep the same namespace and just add new GObject Introspectable methods to existing objects
• GADB - GLib Arrow Data Base
• GADA - GLib Arrow Data Access
• GADC - GLib Arrow Data Connectivity

Just extend the API by adding new GObject Introspectable methods to the existing objects, means different libraries, with the same API and same namespace, but using GArrow and different name. Current is called libadbc-glib; new one will have same namespace but libadbc-arrow-glib as its name.

We can use #if directives to create the new library using the same source code, so when creating libadbc-arrow-glib, we can remove the no-introspectable API from the symbols and just use the new introspectable API. This can remove the adbc-arrow-glib directory and produce a VAPI with the same namespace but different package's name adbc-arrow-glib.

Above method, will allow to reuse the same code and create a new library, but just adding a MACRO definition.

Is this good for you?

[kou]

How about GADBCArrow?

I don't want to use the #if approach. libadbc-arrow-glib should depend on libadbc-glib and libarrow-glib. If we use the #if approach, users use libadbc-arrow-glib as an alternative of libadbc-glib, right? I don't want to put same symbols to both of libadbc-glib and libadbc-arrow-glib.

[esodan]

How about GADBCArrow?

I don't want to use the #if approach. libadbc-arrow-glib should depend on libadbc-glib and libarrow-glib. If we use the #if approach, users use libadbc-arrow-glib as an alternative of libadbc-glib, right? I don't want to put same symbols to both of libadbc-glib and libadbc-arrow-glib.

I see.

On using #if, yes if users want a fully introspectable API they should use libadbc-arrow-glib, with the same namespace and objects as for libadbc-glib. Lets consider the following Code (in Vala for easy reading):

This is how libadbc-arrow-glib should be used:

var statement = new GADBC.Statement (connection);
string sql = "SELECT sqlite_version() AS version";
statement.set_sql_query (sql);
int64 n_rows_affected = 0;
GArrow.RecordBatchReader reader = statement.execute_stream (out n_rows_affected);
var table = reader.read_all ();
stdout.printf ("Result:\n%s", table.to_string ());

Above code is introspectable, but libadbc-arrow-glib have same symbols and namespace than libadbc-glib, they share same source code and #if is used to add code when compiling for libadbc-arrow-glib to add GArrow API symbols.

This is how libadbc-glib should be used:

var statement = new GADBC.Statement (connection);
string sql = "SELECT sqlite_version() AS version";
statement.set_sql_query (sql);
void *c_abi_array_stream = null;
int64 n_rows_affected;
statement.execute (true, out c_abi_array_stream, out n_rows_affected);
var reader = GArrow.RecordBatchReader.import (c_abi_array_stream);
var table = reader.read_all ();
stdout.printf ("Result:\n%s", table.to_string ());

Above code works currently, but is not completely introspectable.

This is how libadbc-glib and libadbc-arrow-glib (dependant) should be used:

var statement = new GADBC.Statement (connection);
string sql = "SELECT sqlite_version() AS version";
statement.set_sql_query (sql);
var stm = GADBCArrow.Statement (statement);
GArrow.RecordBatchReader reader = stm.execute (true, out n_rows_affected);
var table = reader.read_all ();
stdout.printf ("Result:\n%s", table.to_string ());

In above code we use GADBCArrow name space, creating a new set of objects that use internally objects from GADBC, so var stm = GADBCArrow.Statement (statement) is required to add new API. I think, this approach is harder to use.

There is a way, where we wrap all API from GADBC into new objects definitions in GADBCArrow, so when calling a method in GADBCArrow for an object, internally calls the method from GADBC library. But means a lot of work and when GADBC is added the corresponding object should be added to GADBCArrow too.

This is how libadbc-glib and libadbc-arrow-glib (dependant and wrapping) should be used:

var statement = new GADBCArrow.Statement (connection);
string sql = "SELECT sqlite_version() AS version";
statement.set_sql_query (sql);
GArrow.RecordBatchReader reader = statement.execute (true, out n_rows_affected);
var table = reader.read_all ();
stdout.printf ("Result:\n%s", table.to_string ());

Above code all objects including connection are objects in GADBCArrow that wraps objects from GADBC and their API.

I consider the wrapping solution is harder to implement and maintain; while the use of #if solution allows to share source code and add just the API using GArrow symbols when creating the libadbc-arrow-glib library. libadbc-arrow-glib doesn't depend on libadbc-glib they are alternatives each other is up to the user to select one or the other depending on what programming language is using.

Consider that libadbc-arrow-glib will be the facto library for languages using GObject Introspection to access ADBC, because that library will be fully introspectable and easy to use on Python, JS and other languages.

Hope all above convince you to create a C API far from GLIb/GObject and allow that GADBC is modified to provide a fully introspectable API for the future. Yes, that is an API break, but may be necessary in order to make ADBC available for more languages using GObject Introspection.

[kou]

How about just providing GADBCArrowStatment that is a sub class of GADBCStatment in libadbc-arrow-glib?

#define GADBC_ARROW_TYPE_STATEMENT (gadbc_arrow_statement_get_type())
G_DECLARE_DERIVABLE_TYPE(GADBCArrowStatement, gadbc_arrow_statement, GADBCArrow, STATEMENT, GADBCStatement)
struct _GADBCArrowStatementClass {
  GADBCStatementClass parent_class;
};

GADBCArrowStatement* gadbc_arrow_statement_new(GADBCConnection* connection, GError** error);
gboolean
gadbc_arrow_statement_bind(GADBCArrowStatement* statement, GArrowArray *array, GArrowSchema *schema, GError ** error);
gboolean gadbc_arrow_statement_bind_stream(GADBCArrowStatement* statement,
                                     GArrowRecordBatchReader *reader, GError** error);
GArrowRecordBatchReader *gadbc_arrow_statement_execute(GADBCArrowStatement* statement, gboolean need_result,
                                 gint64* n_rows_affected,
                                 GError** error);

Why do we need to wrap all APIs in libadbc-glib?

[esodan]

How about just providing GADBCArrowStatment that is a sub class of GADBCStatment in libadbc-arrow-glib?

#define GADBC_ARROW_TYPE_STATEMENT (gadbc_arrow_statement_get_type())
G_DECLARE_DERIVABLE_TYPE(GADBCArrowStatement, gadbc_arrow_statement, GADBCArrow, STATEMENT, GADBCStatement)
struct _GADBCArrowStatementClass {
  GADBCStatementClass parent_class;
};

GADBCArrowStatement* gadbc_arrow_statement_new(GADBCConnection* connection, GError** error);
gboolean
gadbc_arrow_statement_bind(GADBCArrowStatement* statement, GArrowArray *array, GArrowSchema *schema, GError ** error);
gboolean gadbc_arrow_statement_bind_stream(GADBCArrowStatement* statement,
                                     GArrowRecordBatchReader *reader, GError** error);
GArrowRecordBatchReader *gadbc_arrow_statement_execute(GADBCArrowStatement* statement, gboolean need_result,
                                 gint64* n_rows_affected,
                                 GError** error);

Why do we need to wrap all APIs in libadbc-glib?

Because you can use just one namespace to access all objects and its API. If leave just parts of the API , you have to always add the namespace in order to distinguish between types, when using on Vala, Python or other languages, making weir to use this API.

Also, above code requires a new method with a Connection parameter to initialize the GADBCArrowStatement