Skip to content

Conversation

@kou
Copy link
Member

@kou kou commented Oct 18, 2017

No description provided.

*
* Since: 0.8.0
*/
GArrowTableBatchReader *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some functions like this (which have no novel methods) could instead return instances of the base RecordBatchReader, in case that helps make things simpler

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. I couldn't understand what you want to say.
You say that "is it better that the function returns GArrowRecordBatchReader (base class object) instead of GArrowTableBatchReader (specific class object)?", right?

Short answer: I don't think so.

In GTK+ case, it's true. GTK+ uses GtkWidget as a base class. Many methods accept GtkWidget as argument. So, returning base class (GtkWidget) object instead of specific class (GtkComboBox) object by specific class constructor such as gtk_combo_box_new() makes code simpler. Users can avoid many casts.

In Arrow GLib case, there are no class that are accepted many methods. So, there are only small merits. And GObject Introspection based bindings don't care about return type. They use real class of the object on run time because GObject (object system based on GLib) provides API to get class from object on run time.

I think that returning GArrowRecordBatchReader (base class object) instead of GArrowTableBatchReader (specific class object) isn't reasonable in Arrow GLib case. There are small merits for C API users (not language bindings users). Return type information is reduced in documentation side (returning GArrowTableBatchReader is more meaningful than returning GArrowRecordBatchReader).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thank you. I was mostly asking for my own understanding of how the GObject Introspection bindings work

On the C++ side in most cases we would want to use std::shared_ptr<RecordBatchReader> for generic handling of streams, where all you care about is the ReadNext method. In the Python bindings there would be not much benefit to creating a new wrapper type for the subclass unless it has some unique methods that you wished to expose.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining background. I understand.

In the Python bindings there would be not much benefit to creating a new wrapper type for the subclass unless it has some unique methods that you wished to expose.

It's reasonable. Arrow GLib can also choose the design.

In my experience, creating a new wrapper type for the subclass helps us to debug something. "Easy to debug" is very important for me. :-)

Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@asfgit asfgit closed this in a4813bd Oct 18, 2017
@kou kou deleted the glib-add-table-batch-reader branch October 19, 2017 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants