C-API: support streaming arrow query #8642
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Virgiel
changed the title
Tishj
reviewed
Aug 21, 2023
Tishj
reviewed
Aug 21, 2023
| @@ -83,10 +83,21 @@ duckdb_state duckdb_query_arrow_array(duckdb_arrow result, duckdb_arrow_array *o | |||
| if (!wrapper->current_chunk || wrapper->current_chunk->size() == 0) { | |||
| return DuckDBSuccess; | |||
| } | |||
| ArrowConverter::ToArrowArray(*wrapper->current_chunk, reinterpret_cast<ArrowArray *>(*out_array), wrapper->options); | |||
| ArrowConverter::ToArrowArray(*wrapper->current_chunk, reinterpret_cast<ArrowArray *>(*out_array), | |||
| wrapper->result->client_properties); | |||
There was a problem hiding this comment.
If you're going to use the wrapper->result, please check if it's not null first
There was a problem hiding this comment.
Since checking wrapper->current-chunk before implies that it was retrieved from the query result, I think we can expect wrapper->result not to be null
There was a problem hiding this comment.
I agree, but that's considering the current implementation, if this detail changes in the future this could lead to a crash
|
The last CI failure seems to be unrelated to my changes |
|
Thanks! |
krlmlr
added a commit
to duckdb/duckdb-r
that referenced
this pull request
Dec 11, 2023
Merge pull request duckdb/duckdb#9164 from Mause/feature/jdbc-uuid-param Merge pull request duckdb/duckdb#9185 from pdet/adbc_07 Merge pull request duckdb/duckdb#9126 from Maxxen/parquet-kv-metadata Merge pull request duckdb/duckdb#9123 from lnkuiper/parquet_schema Merge pull request duckdb/duckdb#9086 from lnkuiper/json_inconsistent_structure Merge pull request duckdb/duckdb#8977 from Tishj/python_readcsv_multi_v2 Merge pull request duckdb/duckdb#9279 from hawkfish/nsdate-cast Merge pull request duckdb/duckdb#8851 from taniabogatsch/binary_lambdas Merge pull request duckdb/duckdb#8983 from Maxxen/types/fixedsizelist Merge pull request duckdb/duckdb#9318 from Maxxen/fix-unused Merge pull request duckdb/duckdb#9220 from hawkfish/exclude Merge pull request duckdb/duckdb#9230 from Maxxen/json-plan-serialization Merge pull request duckdb/duckdb#9011 from Tmonster/add_create_statement_support_to_fuzzer Merge pull request duckdb/duckdb#9400 from Maxxen/array-fixes Merge pull request duckdb/duckdb#8741 from Tishj/python_import_cache_upgrade Merge fixes Merge pull request duckdb/duckdb#9395 from taniabogatsch/lambda-performance Merge pull request duckdb/duckdb#9427 from Tishj/python_table_support_replacement_scan Merge pull request duckdb/duckdb#9516 from carlopi/fixformat Merge pull request duckdb/duckdb#9485 from Maxxen/fix-parquet-serialization Merge pull request duckdb/duckdb#9388 from chrisiou/issue217 Merge pull request duckdb/duckdb#9565 from Maxxen/fix-array-vector-sizes Merge pull request duckdb/duckdb#9583 from carlopi/feature Merge pull request duckdb/duckdb#8907 from cryoEncryp/new-list-functions Merge pull request duckdb/duckdb#8642 from Virgiel/capi-streaming-arrow Merge pull request duckdb/duckdb#8658 from Tishj/pytype_optional Merge pull request duckdb/duckdb#9040 from Light-City/feature/set_mg
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the C-Arrow API uses a wrapper type. As the wrapper was created
before streaming queries, it does not support them. The fact that the query
result and data chunk are hidden in the wrapper prevents users from using
current and future methods, and forces us to add additional functions to support
new features. If we wanted to add support for streaming arrow requests with the
wrapper, without breaking the current code, we'd have to add
duckdb_execute_prepared_arrow_streamingandduckdb_query_arrow_streaming.I suggest adding a function to convert the datachunk into an arrow array.
This is compatible with both types of query and should be more future-proof.
I've simplified
ArrowResultWrapper, so it no longer needs to storeClientPropertiessince it's already stored inQueryResult. I've added alow-level function that converts a data chunk into and arrow array.