[Arrow (Dev)] Refactor arrow scan internals #8430
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
pdet
suggested changes
Aug 1, 2023
| unique_ptr<ArrowType> dictionary_type; | ||
| }; | ||
|
|
||
| using arrow_column_map_t = unordered_map<idx_t, ArrowType>; |
There was a problem hiding this comment.
I think the type alias is a bit of an excessive abstraction here
There was a problem hiding this comment.
Hmm yea I agree, before this refactor it was all over the place, but now it's only in a single location
There was a problem hiding this comment.
Actually, on further inspection I think it makes the prototypes less bulky and clearly defines the relation between ArrowTableType and the arrow scan functions.
It also allows us to change these things in one place, for example when I made the change to unique_ptr over move semantics it was really nice to just change the using definition and not have to hunt down the other uses of it.
…that happens, checking if consistent
Tishj
force-pushed
the
arrow_conversion_refactor
branch
from
a51e99e
to
ee66ea7
Compare
|
Thanks! |
Maxxen
added a commit
to duckdb/duckdb_spatial
that referenced
this pull request
Aug 31, 2023
Apply patch from duckdb/duckdb#8430 and update duckdb to latest main
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.
Previously we populated
ArrowConvertDatawhile scanning the arrow schema and the function returned a DuckDB LogicalType.This has been removed, instead the function now outputs an
ArrowType, this contains both the LogicalType and the data that would be put into theArrowConvertData.This allows us to simplify the arrow scan a lot, as we no longer need to pass the column index, the map and the 'arrow_convert_index'
This
arrow_convert_indexconsisted of a vector ofArrowConvertDataIndices, this was state used to remember which column/column child we were scanning.This has now also been removed in its entirety.