Implement binding for List#757
Open
aljazerzen wants to merge 3 commits into
Open
Conversation
Refactor `Display for Type` into `name() -> &str`. Add `Value::data_type_name()`, so it does not need to infer the type recursively just to get its name.
See reasoing in PR description. Also simplifies value_ref_from_value, so the caller can decide how to handle unsupported types.
Author
|
@mlafeldt What's happening here? This is blocking me and it seem a bit rude to ask for a PR and then not review it. |
|
I've created a new crate, implemented almost many of types, you can check it here |
This file contains hidden or 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
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.
Resolves a part of #680
Susersedes #643
I recomment review commit-by-commit:
Main problem
Value::Listis not cheaply convertible intoValueRef::List.Before this PR, binding (and appending) converted all values to
ValueRef,and then matched on the variant to call appropriate
ffi::duckdb_bind_xxx.For container types, we cannot cheaply convert between
ValueandValueRef,so I've added two binding paths:
value_to_duckdbandvalue_ref_to_duckdb.Both convert to
ffi::duckdb_value, which is then passed toffi::duckdb_bind_value/ffi::duckdb_append_value.These two helpers do also support converting all other types, but they always
use the faster path of calling specialized ffi functions.
Why add
ToSqlOutput::BorrowedValuevariant?Because we need to call
ToSql(Value)and return&Value, notValueRef.In essence, the problem is that there are two dimensions in which
Valueand
ValueRefdiffer:For most types except container types, both of these distinctions are irrelevant.
But for container types (e.g
List), there can be 4 possible representations:Vec<Value>, (this isValue)arrow::ListArray,&[Value],&arrow::ListArray.When binding,
ToSqlneeds a path fromowned dynamictoborrowed dynamic.PR #643 gets around this by adding a new variant to ListType, but I think that
is messy because it mixes static/dynamic distinction with arrow list type impls.