MultiCfIterator - AttributeGroupIter Impl & CoalescingIter Optimization #12534
Conversation
jaykorean
force-pushed
the
attribute_group_iterator
branch
from
47e7cbb
to
0e00bb9
Compare
jaykorean
changed the title
|
@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@jaykorean has updated the pull request. You must reimport the pull request before landing. |
|
@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
jaykorean
force-pushed
the
attribute_group_iterator
branch
from
9aba0d8
to
fcb2472
Compare
|
@jaykorean has updated the pull request. You must reimport the pull request before landing. |
|
@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Thanks for the patch @jaykorean ! Looks awesome!!! Just a couple of minor comments (see below)
| // TODO - Implement AttributeGroup population | ||
| const autovector<MultiCfIteratorInfo>& items) { | ||
| for (const auto& item : items) { | ||
| attribute_groups_.emplace_back(item.cfh, &item.iterator->columns()); |
There was a problem hiding this comment.
Just to confirm, items here is in the expected order (i.e. any CFs are in the order specified during iterator creation) because that's the order they're popped off the heap, right?
|
Also, I don't recall if we've added any history entries for |
|
@ltamasi I was planning to add |
jaykorean
force-pushed
the
attribute_group_iterator
branch
from
fcb2472
to
5c37791
Compare
|
@jaykorean has updated the pull request. You must reimport the pull request before landing. |
|
@jaykorean has updated the pull request. You must reimport the pull request before landing. |
|
@jaykorean has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@jaykorean merged this pull request in d34712e. |
Summary
Continuing from the previous MultiCfIterator Implementations - (#12422, #12480 #12465), this PR completes the
AttributeGroupIteratorby implementingAttributeGroupIteratorImpl::AddToAttributeGroups(). While implementing theAttributeGroupIterator, we had to make some changes inMultiCfIteratorImpland found an opportunity to improveCoalesce()inCoalescingIterator.Lifting
UNDER CONSTRUCTION - DO NOT USEcomment by replacing it withEXPERIMENTALHere are some implementation details:
IteratorAttributeGroupsis introduced to avoid having to copy allWideColumnobjects during iteration.PopulateIterator()no longer advances non-top iterators that have the same key as the top iterator in the heap.AdvanceIterator()needs to advance the non-top iterators when they have the same key as the top iterator in the heap.PopulateIterator()now collects all items with the same key and callspopulate_func(items)at once.Coalesce()such that we no longer do K-1 rounds of 2-way merge, but do one K-way merge instead.Test Plan
Uncommented the assertions in
verifyAttributeGroupIterator()