Improve IColumn insertFrom & scatter/scatterTo interface
@yibin87 is already working on this.
Since Feb 6, 2023.
- Dominant language
- C++
- Stars
- 1k
- Forks
- 423
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 24
Description
Enhancement
- Currently, the scatter/scatterTo default implementation is not locality friendly:
ScatterColumns columns;
initializeScatterColumns(columns, num_columns, num_rows);
for (size_t i = 0; i < num_rows; ++i)
static_cast<Derived &>(*columns[selector[i]]).insertFrom(*this, i);
Current implementation sacrifies the dst column access locality to achieve sequential element access for src column. For the worst case, the dst column would change for every insertion. When dst columns' size becomes larger, the problem might get worse.
It can be optimized like this:
ScatterColumns columns;
initializeScatterColumns(columns, num_columns, num_rows);
for (size_t i = 0; i < num_columns; ++i)
static_cast<Derived &>(*columns[i]).insertDisjunctFrom(*this, index_vec);
- Currently, IColumn provide insertFrom and insertRangeFrom interface, first for scalar insertion and second for range insertion.
virtual void insertFrom(const IColumn & src, size_t n) { insert(src[n]); }
virtual void insertRangeFrom(const IColumn & src, size_t start, size_t length) = 0;
For non-continuous mutilple insertions, for example, insert [0,2,4,6,8,10,.....,2n]th element, currently, we have to write like this:
for (size_t i = 0; i < 2n + 2; i += 2)
col_a->insertFrom(col_b, i);
However, insertFrom is a virtual function, that means one virtual call for one element insertion which is not efficient. It can be improved by providing a insertMultipleFrom(const IColumn & src, std::vector<size_t> & index_vec).
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.