[Vectorization] support SegmentIterator vectorization by wangbo · Pull Request #7613 · apache/doris

wangbo · 2022-01-05T06:21:36Z

Proposed changes

Support SegmentIterator vectorization, see #7614

Types of changes

What types of changes does your code introduce to Doris?
Put an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update (if none of the other choices apply)
Code refactor (Modify the code structure, format the code, etc...)
Optimization. Including functional usability improvements and performance improvements.
Dependency. Such as changes related to third-party components.
Other.

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

I have created an issue on (Fix #ISSUE) and described the bug/feature there in detail
Compiling and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
If these changes need document changes, I have updated the document
Any dependent changes have been merged

Further comments

If this is a relatively large or complex change, kick off the discussion at [email protected] by explaining why you chose the solution you did and what alternatives you considered, etc...

zuochunwei · 2022-01-07T02:51:29Z

i will review this PR later

zuochunwei · 2022-01-11T06:13:09Z

    virtual void evaluate_vec(vectorized::IColumn& column, uint16_t size, bool* flags) const {};
    uint32_t column_id() const { return _column_id; }

+    virtual bool is_in_predicate() { return false; }


is_in_predicate should be declared as const because it seems read-only

zuochunwei · 2022-01-11T06:16:37Z

+vectorized::IColumn::MutablePtr Schema::get_predicate_column_ptr(FieldType type) {
+    switch (type) {
+        case OLAP_FIELD_TYPE_TINYINT:
+            return doris::vectorized::PredicateColumnType<doris::vectorized::Int8>::create();


too much switch type case xxx, i have seen many times such functions, building a mapping may a good choice.
template technology may be useful to satisfied this need

This method could just be called once, branch doesn't matter.
And in the actual performance test, this won't cost too much time.

zuochunwei · 2022-01-11T06:27:45Z

+            dst_col_ptr = nullable_column->get_nested_column_ptr().get();
+
+            // fill null bitmap here, not null;
+            for (int j = begin; j < end; j++) {


consider using add_num_element instead

zuochunwei · 2022-01-11T06:29:24Z

    virtual void replace_column_data_default(size_t self_row = 0) = 0;

-    bool is_date_type() { return is_date; }
+    virtual bool is_date_type() { return is_date; }


is_xxx should be marked as const

This is a temporary implementation, this method would be removed later.

zuochunwei · 2022-01-11T06:30:19Z

        return false;
    }

+    bool is_date_type() override { return get_nested_column().is_date_type(); }


the same problem to above

zuochunwei · 2022-01-11T09:12:46Z

i need spend more time to check this PR carefully, so give me more time, i will compete in a few days!

zuochunwei · 2022-01-11T09:16:18Z

+                if (len != 0) {
+                    BitmapValue value;
+                    value.deserialize(&_data[start_offset]);
+                    *pvalue = std::move(value);


why not use pvalue->deserialize() directly?

I'm not sure, I refer the logic in RowBlockV2::_copy_data_to_column.

zuochunwei · 2022-01-11T09:37:14Z

+    bool is_mem_reuse = block->mem_reuse();
+    SCOPED_RAW_TIMER(&_opts.stats->block_load_ns);
+    if (UNLIKELY(!_inited)) {
+        RETURN_IF_ERROR(_init(true));


conditionly init is not a good practice although this usage is applied frequently
separating init from next_batch is more reasonable because these two methods do absolutely different things

This method could only be called once in a segment_iterator's lifecycle, this is not frequently at all.
Add a args just for code reuse, you can see the method _init 's implementation.

i know your meaning, i am talking from the reasonableness
too many usages in doris, but i still think should separate these two functions
i modified this similar usage in BlockReader

zuochunwei · 2022-01-11T09:43:48Z

+        for (int i = block->columns() - 1; i >= column_to_keep; i--) {
+            block->erase(i);
+        }
+        block->clear_column_data();


use block->clear_column_data(_schema.num_column_ids()) is better!

The column of block has be dealt in SegmentIterator::_init_current_block.
So here block should only contain return columns.

check clear_column_data(with arg) 's implement, i think it provided what you want

zuochunwei · 2022-01-11T09:53:00Z

+    return Status::OK();
+}
+
+void SegmentIterator::_init_current_block(vectorized::Block* block, std::vector<vectorized::MutableColumnPtr>& current_columns) {


why pass member data _current_return_columns as member function‘s argument?
why not use _current_return_columns in _init_current_block directly?

zuochunwei · 2022-01-11T10:03:14Z

+                }
+            }
+            // not sure whether block is clear before enter segmentIter, so clear it here.
+            if (is_mem_reuse) {


it' s not a good idea to clear here. think who should take responsibility for clear job.
in another words, we should wrap the reuse logic rather than exposed everywhere

You are right, but this is just a defensive strategy.
What you talk about should be considered after this PR merged, that's not this PR wish to solve.

wangbo added area/vectorization area/storage Issues or PRs related to storage engine labels Jan 5, 2022

HappenLee reviewed Jan 11, 2022

View reviewed changes

Comment thread be/src/olap/rowset/segment_v2/binary_dict_page.cpp Resolved

Comment thread be/src/olap/rowset/segment_v2/binary_dict_page.cpp Resolved

HappenLee reviewed Jan 11, 2022

View reviewed changes

Comment thread be/src/olap/schema.cpp Resolved

zuochunwei reviewed Jan 11, 2022

View reviewed changes

HappenLee force-pushed the vectorized branch from ba09d3d to fc05698 Compare January 13, 2022 11:43

[Vectorization] support SegmentIterator vectorization

79dfd8a

wangbo force-pushed the segment_iter_rebase_pr branch from 9088814 to 79dfd8a Compare January 13, 2022 12:22

HappenLee merged commit ee90c94 into apache:vectorized Jan 14, 2022

HappenLee pushed a commit that referenced this pull request Jan 17, 2022

[Vectorization] Support SegmentIterator vectorization (#7613)

e23e332

Conversation

wangbo commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Checklist

Further comments

Uh oh!

zuochunwei commented Jan 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuochunwei Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuochunwei commented Jan 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuochunwei Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuochunwei Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuochunwei Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wangbo commented Jan 5, 2022 •

edited

Loading

zuochunwei Jan 11, 2022 •

edited

Loading

zuochunwei Jan 11, 2022 •

edited

Loading

zuochunwei Jan 12, 2022 •

edited

Loading

zuochunwei Jan 11, 2022 •

edited

Loading