This repository was archived by the owner on May 9, 2024. It is now read-only.
[Join] Zero-copy storage column check. #623
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
Devjiu
commented
Aug 8, 2023
| @@ -187,6 +187,53 @@ std::unique_ptr<AbstractDataToken> ArrowStorage::getZeroCopyBufferMemory( | |||
| return nullptr; | |||
| } | |||
|
|
|||
| std::unique_ptr<AbstractDataToken> ArrowStorage::getZeroCopyColumnData( | |||
| const ColumnRef& col_ref) { | |||
There was a problem hiding this comment.
Almost copy-paste of getZeroCopyBufferMemory. Should we unite this methods, or add private utility method?
There was a problem hiding this comment.
Marking it as temporary solution, should be removed in further prs.
Devjiu
commented
Aug 8, 2023
| @@ -436,6 +436,12 @@ ResultSetRegistry::getZeroCopyBufferMemory(const ChunkKey& key, size_t num_bytes | |||
| : nullptr; | |||
| } | |||
|
|
|||
| std::unique_ptr<AbstractDataToken> ResultSetRegistry::getZeroCopyColumnData( | |||
| const ColumnRef& col_ref) { | |||
| // UNREACHABLE(); | |||
There was a problem hiding this comment.
not sure how to handle this case correctly, should we support it?
Devjiu
force-pushed
the
dmitriim/join_column_data_zero_copy
branch
2 times, most recently
from
a39fffe to
35046cb
Compare
Devjiu
force-pushed
the
dmitriim/join_column_data_zero_copy
branch
2 times, most recently
from
f9f7000 to
b42d496
Compare
ienkovich
reviewed
Aug 16, 2023
| , direct_columnar_conversion_(false) | ||
| , thread_idx_(thread_idx) { | ||
| auto timer = DEBUG_TIMER(__func__); | ||
| const bool is_varlen = target_type->isArray() || target_type->isString(); |
There was a problem hiding this comment.
This is incorrect, we have fixed-length arrays. Proper check is target_type->isVarLen().
| size_t arrow_elem_size = fixed_type->bit_width() / 8; | ||
| size_t elems = elem_size / arrow_elem_size; | ||
| CHECK_GT(elems, (size_t)0); | ||
| auto data_to_fetch = table.col_data[col_idx]; |
There was a problem hiding this comment.
Suggested change
| auto data_to_fetch = table.col_data[col_idx]; | |
| auto &data_to_fetch = table.col_data[col_idx]; |
Devjiu
force-pushed
the
dmitriim/join_column_data_zero_copy
branch
from
b42d496 to
ec29e93
Compare
ienkovich
reviewed
Aug 21, 2023
| @@ -203,7 +248,7 @@ void ArrowStorage::fetchFixedLenData(const TableData& table, | |||
| // to get a proper slice. | |||
| size_t elems = elem_size / arrow_elem_size; | |||
| CHECK_GT(elems, (size_t)0); | |||
| auto data_to_fetch = | |||
| const auto& data_to_fetch = | |||
There was a problem hiding this comment.
Slice creates a new shared_ptr, therefore you cannot use reference here and all other similar places.
Devjiu
force-pushed
the
dmitriim/join_column_data_zero_copy
branch
from
ec29e93 to
a3cf4a0
Compare
This commit adds check for valid pointer to column buffer to skip copying. Most effective with enabled `enable-non-lazy-data-import` option. Checks number of chunks in storage. Partially resolves: #574 Signed-off-by: Dmitrii Makarenko <[email protected]>
Devjiu
force-pushed
the
dmitriim/join_column_data_zero_copy
branch
from
a3cf4a0 to
0b46841
Compare
ienkovich
approved these changes
Aug 23, 2023
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This commit adds check for valid pointer to column buffer to skip copying. Most effective with enabled
enable-non-lazy-data-importoption. Checks number of chunks in storage.Partially resolves: #574