You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2022/07/07 12:52:27 UTC
[GitHub] [doris] Lchangliang opened a new pull request, #10684: [BugFix] Column datas doesn't match nullmap when vectorization load
Lchangliang opened a new pull request, #10684:
URL: https://github.com/apache/doris/pull/10684
# Proposed changes
Issue Number: close #xxx
## Problem Summary:
In
```
SegmentWriter::append_block(const vectorized::Block* block, size_t row_pos, size_t num_rows)
```,
if a block is divided into several parts to append, we need to use row_pos and num_rows to know the range to append. There is a problem when the column is nullable type.
In
```
void OlapBlockDataConvertor::OlapColumnDataConvertorBase::set_source_column(
const ColumnWithTypeAndName& typed_column, size_t row_pos, size_t num_rows) {
DCHECK(row_pos + num_rows <= typed_column.column->size())
<< "row_pos=" << row_pos << ", num_rows=" << num_rows
<< ", typed_column.column->size()=" << typed_column.column->size();
_typed_column = typed_column;
_row_pos = row_pos;
_num_rows = num_rows;
if (_typed_column.column->is_nullable()) {
auto nullable_column =
assert_cast<const vectorized::ColumnNullable*>(_typed_column.column.get());
_nullmap = nullable_column->get_null_map_data().data();
}
}
```,
OlapColumnDataConvertorBase will get the nullmap which associated with column.
When call convert_to_olap method
```
Status convert_to_olap() override {
const vectorized::ColumnVector<T>* column_data = nullptr;
if (_nullmap) {
auto nullable_column =
assert_cast<const vectorized::ColumnNullable*>(_typed_column.column.get());
column_data = assert_cast<const vectorized::ColumnVector<T>*>(
nullable_column->get_nested_column_ptr().get());
} else {
column_data =
assert_cast<const vectorized::ColumnVector<T>*>(_typed_column.column.get());
}
assert(column_data);
_values = (const T*)(column_data->get_data().data()) + _row_pos;
return Status::OK();
}
```
it will move the ptr of values through the _row_pos. But nullmap doesn't do it and it will return directly.
and then when call append_nullable method
```
Status ColumnWriter::append_nullable(const uint8_t* null_map, const uint8_t** ptr,
size_t num_rows) {
size_t offset = 0;
auto next_run_step = [&]() {
size_t step = 1;
for (auto i = offset + 1; i < num_rows; ++i) {
if (null_map[offset] == null_map[i])
step++;
else
break;
}
return step;
};
do {
auto step = next_run_step();
if (null_map[offset]) {
RETURN_IF_ERROR(append_nulls(step));
*ptr += get_field()->size() * step;
} else {
// TODO:
// 1. `*ptr += get_field()->size() * step;` should do in this function, not append_data;
// 2. support array vectorized load and ptr offset add
RETURN_IF_ERROR(append_data(ptr, step));
}
offset += step;
} while (offset < num_rows);
return Status::OK();
}
```
it will always read the nullmap starting from zero so that the datas in block starting from row_pos doesn't match the nullmap. This can lead to that the position of nullmap is one, but the data exists.
## Checklist(Required)
1. Does it affect the original behavior: (Yes/No/I Don't know)
2. Has unit tests been added: (Yes/No/No Need)
3. Has document been added or modified: (Yes/No/No Need)
4. Does it need to update dependencies: (Yes/No)
5. Are there any changes that cannot be rolled back: (Yes/No)
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] yiguolei commented on a diff in pull request #10684: [BugFix] Column datas doesn't match nullmap when vectorization load
Posted by GitBox <gi...@apache.org>.
yiguolei commented on code in PR #10684:
URL: https://github.com/apache/doris/pull/10684#discussion_r916410856
##########
be/src/vec/olap/olap_data_convertor.cpp:
##########
@@ -138,6 +138,7 @@ void OlapBlockDataConvertor::OlapColumnDataConvertorBase::set_source_column(
auto nullable_column =
assert_cast<const vectorized::ColumnNullable*>(_typed_column.column.get());
_nullmap = nullable_column->get_null_map_data().data();
+ _nullmap += row_pos;
Review Comment:
If the method is call twice, the nullmap maybe wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] Gabriel39 commented on a diff in pull request #10684: [BugFix] Column datas doesn't match nullmap when vectorization load
Posted by GitBox <gi...@apache.org>.
Gabriel39 commented on code in PR #10684:
URL: https://github.com/apache/doris/pull/10684#discussion_r916410863
##########
be/src/vec/olap/olap_data_convertor.cpp:
##########
@@ -138,6 +138,7 @@ void OlapBlockDataConvertor::OlapColumnDataConvertorBase::set_source_column(
auto nullable_column =
assert_cast<const vectorized::ColumnNullable*>(_typed_column.column.get());
_nullmap = nullable_column->get_null_map_data().data();
+ _nullmap += row_pos;
Review Comment:
`row_pos` has been already used to compute offset of _nullmap somewhere (HLL, BitMap, String, etc.). Maybe some basic types `row_pos` is missing to compute `_nullmap` and we should only fix this in those types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] github-actions[bot] commented on pull request #10684: [BugFix] Column datas doesn't match nullmap when vectorization load
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #10684:
URL: https://github.com/apache/doris/pull/10684#issuecomment-1177581040
PR approved by anyone and no changes requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] github-actions[bot] commented on pull request #10684: [BugFix] Column datas doesn't match nullmap when vectorization load
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #10684:
URL: https://github.com/apache/doris/pull/10684#issuecomment-1178526169
PR approved by at least one committer and no changes requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] yiguolei merged pull request #10684: [BugFix] Column datas doesn't match nullmap when vectorization load
Posted by GitBox <gi...@apache.org>.
yiguolei merged PR #10684:
URL: https://github.com/apache/doris/pull/10684
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] github-actions[bot] commented on pull request #10684: [BugFix] Column datas doesn't match nullmap when vectorization load
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #10684:
URL: https://github.com/apache/doris/pull/10684#issuecomment-1177580998
PR approved by at least one committer and no changes requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org