You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "yixiutt (via GitHub)" <gi...@apache.org> on 2023/02/10 08:37:29 UTC
[GitHub] [doris] yixiutt opened a new pull request, #16605: [bugfix](column_reader) index_page should not be pre-decoded
yixiutt opened a new pull request, #16605:
URL: https://github.com/apache/doris/pull/16605
In our current logic, index page will be pre-decoded but it will return OK as index page use BinaryPlainPageBuilder and first 4 bytes of the page is a offset so it's high probablility not equal to EncodingTypePB::DICT_ENCODING which is 5.
Code in bitshuffle_page_pre_decode.h
```
if constexpr (USED_IN_DICT_ENCODING) {
auto type = decode_fixed32_le((const uint8_t*)&data.data[0]);
if (static_cast<EncodingTypePB>(type) != EncodingTypePB::DICT_ENCODING) {
return Status::OK();
}
size_of_dict_header = BINARY_DICT_PAGE_HEADER_SIZE;
data.remove_prefix(4);
}
```
But if type just equal to EncodingTypePB::DICT_ENCODING and then it will use BitShuffle to decode BinaryPlainPage, which will leads to an fatal error.
# Proposed changes
Issue Number: close #xxx
## Problem summary
Describe your changes.
## 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 (If Yes, please explain WHY)
- [ ] 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] carlvinhust2012 commented on a diff in pull request #16605: [bugfix](column_reader) index_page should not be pre-decoded
Posted by "carlvinhust2012 (via GitHub)" <gi...@apache.org>.
carlvinhust2012 commented on code in PR #16605:
URL: https://github.com/apache/doris/pull/16605#discussion_r1102461763
##########
be/src/olap/rowset/segment_v2/column_reader.cpp:
##########
@@ -210,6 +210,10 @@ Status ColumnReader::read_page(const ColumnIteratorOptions& iter_opts, const Pag
opts.type = iter_opts.type;
opts.encoding_info = _encoding_info;
opts.io_ctx = iter_opts.io_ctx;
+ // index page should not pre decode
+ if (iter_opts.type == INDEX_PAGE) {
Review Comment:
maybe add some regression test will be more better ?
--
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 #16605: [bugfix](column_reader) index_page should not be pre-decoded
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #16605:
URL: https://github.com/apache/doris/pull/16605#issuecomment-1425422436
clang-tidy review says "All clean, LGTM! :+1:"
--
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] hello-stephen commented on pull request #16605: [bugfix](column_reader) index_page should not be pre-decoded
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #16605:
URL: https://github.com/apache/doris/pull/16605#issuecomment-1425900015
TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 34.17 seconds
stream load tsv: 465 seconds loaded 74807831229 Bytes, about 153 MB/s
stream load json: 37 seconds loaded 2358488459 Bytes, about 60 MB/s
stream load orc: 68 seconds loaded 1101869774 Bytes, about 15 MB/s
https://doris-community-test-1308700295.cos.ap-hongkong.myqcloud.com/tmp/20230210143935_clickbench_pr_93863.html
--
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] dataroaring merged pull request #16605: [bugfix](column_reader) index_page should not be pre-decoded
Posted by "dataroaring (via GitHub)" <gi...@apache.org>.
dataroaring merged PR #16605:
URL: https://github.com/apache/doris/pull/16605
--
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] zhannngchen commented on pull request #16605: [bugfix](column_reader) index_page should not be pre-decoded
Posted by "zhannngchen (via GitHub)" <gi...@apache.org>.
zhannngchen commented on PR #16605:
URL: https://github.com/apache/doris/pull/16605#issuecomment-1425763979
Add a DCHECK for the type variable to make sure we used the right decoder?
```
if constexpr (USED_IN_DICT_ENCODING) {
auto type = decode_fixed32_le((const uint8_t*)&data.data[0]);
if (static_cast<EncodingTypePB>(type) != EncodingTypePB::DICT_ENCODING) {
return Status::OK();
}
size_of_dict_header = BINARY_DICT_PAGE_HEADER_SIZE;
data.remove_prefix(4);
}
```
--
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] zhannngchen commented on pull request #16605: [bugfix](column_reader) index_page should not be pre-decoded
Posted by "zhannngchen (via GitHub)" <gi...@apache.org>.
zhannngchen commented on PR #16605:
URL: https://github.com/apache/doris/pull/16605#issuecomment-1425762899
Add a DCHECK for the type variable to make sure we used the right decoder?
```
if constexpr (USED_IN_DICT_ENCODING) {
auto type = decode_fixed32_le((const uint8_t*)&data.data[0]);
if (static_cast<EncodingTypePB>(type) != EncodingTypePB::DICT_ENCODING) {
return Status::OK();
}
size_of_dict_header = BINARY_DICT_PAGE_HEADER_SIZE;
data.remove_prefix(4);
}
```
--
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] yixiutt commented on a diff in pull request #16605: [bugfix](column_reader) index_page should not be pre-decoded
Posted by "yixiutt (via GitHub)" <gi...@apache.org>.
yixiutt commented on code in PR #16605:
URL: https://github.com/apache/doris/pull/16605#discussion_r1103935888
##########
be/src/olap/rowset/segment_v2/column_reader.cpp:
##########
@@ -210,6 +210,10 @@ Status ColumnReader::read_page(const ColumnIteratorOptions& iter_opts, const Pag
opts.type = iter_opts.type;
opts.encoding_info = _encoding_info;
opts.io_ctx = iter_opts.io_ctx;
+ // index page should not pre decode
+ if (iter_opts.type == INDEX_PAGE) {
Review Comment:
it's a inner bug and logic here is two mistakes leads to running correnct, we already have case.
--
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] carlvinhust2012 commented on a diff in pull request #16605: [bugfix](column_reader) index_page should not be pre-decoded
Posted by "carlvinhust2012 (via GitHub)" <gi...@apache.org>.
carlvinhust2012 commented on code in PR #16605:
URL: https://github.com/apache/doris/pull/16605#discussion_r1102461763
##########
be/src/olap/rowset/segment_v2/column_reader.cpp:
##########
@@ -210,6 +210,10 @@ Status ColumnReader::read_page(const ColumnIteratorOptions& iter_opts, const Pag
opts.type = iter_opts.type;
opts.encoding_info = _encoding_info;
opts.io_ctx = iter_opts.io_ctx;
+ // index page should not pre decode
+ if (iter_opts.type == INDEX_PAGE) {
Review Comment:
maybe add some regression test will more better ?
--
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] yixiutt commented on pull request #16605: [bugfix](column_reader) index_page should not be pre-decoded
Posted by "yixiutt (via GitHub)" <gi...@apache.org>.
yixiutt commented on PR #16605:
URL: https://github.com/apache/doris/pull/16605#issuecomment-1427231123
> Add a DCHECK for the type variable to make sure we used the right decoder?
>
> ```
> if constexpr (USED_IN_DICT_ENCODING) {
> auto type = decode_fixed32_le((const uint8_t*)&data.data[0]);
> if (static_cast<EncodingTypePB>(type) != EncodingTypePB::DICT_ENCODING) {
> return Status::OK();
> }
> size_of_dict_header = BINARY_DICT_PAGE_HEADER_SIZE;
> data.remove_prefix(4);
> }
> ```
only USED_IN_DICT_ENCODING have a type, other encoder may encode offset or nums in the first bytes
--
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