Skip to content
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

Sparse global order reader: getting rid of the result cell slab copy. #2411

Merged
merged 1 commit into from
Jul 25, 2021

Conversation

KiterLuc
Copy link
Contributor

This gets rid of the need for a copy of the result cell slabs by keeping
and index of where to end the copy operations in ReaderBase.

Also noticed the context caches were only used to store cell slab
partitions so moved the code to generate those to simple functions.


TYPE: IMPROVEMENT
DESC: Sparse global order reader: no more result cell slab copy.

@shortcut-integration
Copy link

This pull request has been linked to Clubhouse Story #8332: Read refactor: create basic sparse global order reader..

tiledb/sm/query/reader_base.h Outdated Show resolved Hide resolved
tiledb/sm/query/reader_base.h Outdated Show resolved Hide resolved
tiledb/sm/query/reader_base.h Outdated Show resolved Hide resolved
tiledb/sm/query/reader_base.h Outdated Show resolved Hide resolved
tiledb/sm/query/reader_base.h Outdated Show resolved Hide resolved
tiledb/sm/query/reader_base.cc Outdated Show resolved Hide resolved
to_copy.emplace_back(tile, it->start_, num_cells);
copy_end_.first++;
copy_end_.second = it->start_ + num_cells;
break;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous code checked num_cells in the while condition to break when there is no more room to copy, which wasn't accurate since is could also go below 0. I removed the num_cells condition and added the break.

// Count number of elements actually copied.
uint64_t cells_copied = 0;
for (auto rcs : copied) {
cells_copied += rcs.length_;
for (uint64_t i = 0; i < copy_end_.first - 1; i++) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is copy_end_.first - 1 correct or should it be copy_end_.first?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is correct because of cells_copied += copy_end_.second; below the for loop.

This gets rid of the need for a copy of the result cell slabs by keeping
and index of where to end the copy operations in ReaderBase.

Also noticed the context caches were only used to store cell slab
partitions so moved the code to generate those to simple functions.

---
TYPE: IMPROVEMENT
DESC: Sparse global order reader: no more result cell slab copy.
@KiterLuc KiterLuc force-pushed the lr/sparse-global-order-followup/ch8332 branch from c9670cc to cbc7c13 Compare July 24, 2021 22:45
@stavrospapadopoulos stavrospapadopoulos merged commit 3ea048c into dev Jul 25, 2021
@stavrospapadopoulos stavrospapadopoulos deleted the lr/sparse-global-order-followup/ch8332 branch July 25, 2021 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants