-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
return Adbc.Column
#70
Conversation
Currently it supports both primitive and complex data types in results. I'll send another PR for binding parameters with complex data types using |
lib/adbc_connection.ex
Outdated
|
||
{:error, reason} -> | ||
{:error, error_to_exception(reason)} | ||
end | ||
end | ||
|
||
defp merge_columns(columns) do | ||
Enum.reduce(columns, [], fn column, merged_columns -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The columns should always be in the same order, right? Maybe we can use zip_with instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wonder if maybe call stream_results
passing empty columns as a third argument. Then adbc_arrow_array_stream_next
returns only a list of lists, which we can concatenate, and put it back into the Adbc.Column only at the end. WDYT? This would be the most efficient way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we can return the empty columns/metadata with :end_of_series
.
So overall, it would be something like this: on each result set, we get a list of lists. We can either zip them together or collect them to flatten at the end. Then at the end we receive empty column structs from the metadata and put the actual data inside (by using zip_with)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe we can send paged/chunked results to users by default (the same way as arrow/adbc returns the results to us) and only concatenate the results if user explicitly requested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Two minor comments and we can ship it!
Thanks for the code review Jose! And I applied a minor change so the users can use seconds and milliseconds in |
This is a follow-up PR of #68.