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 2020/11/19 06:17:47 UTC
[GitHub] [incubator-doris] acelyc111 opened a new pull request #4925: [Refactor] Refactor DeleteHandler and Cond module
acelyc111 opened a new pull request #4925:
URL: https://github.com/apache/incubator-doris/pull/4925
## Proposed changes
This patch mainly do the following refactors:
- Use int64_t instead of int32_t for 'version' in DeleteHandler
- Move some comments from .cpp to .h file, add some new comments in .h files, and also remove some meaningless comments
- Use switch...case... instead of multiple if..else.. for DeleteConditionHandler::is_condition_value_valid
- Use range loop to simplify code
- Reduce some compare operations in Cond::del_eval
- Improve some branch predictions in Reader
- Fix and improve some unit tests
## 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)
- [x] Code refactor (Modify the code structure, format the code, etc...)
## 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 create an issue on (Fix #ISSUE), and have described the bug/feature there in detail
- [x] 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 this change need a document change, I have updated the document
- [] Any dependent changes have been merged
----------------------------------------------------------------
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.
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] [incubator-doris] acelyc111 commented on a change in pull request #4925: [Refactor] Refactor DeleteHandler and Cond module
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on a change in pull request #4925:
URL: https://github.com/apache/incubator-doris/pull/4925#discussion_r528290975
##########
File path: be/src/olap/reader.cpp
##########
@@ -599,13 +599,12 @@ OLAPStatus Reader::_init_params(const ReaderParams& read_params) {
if (_tablet->tablet_schema().has_sequence_col()) {
_sequence_col_idx = _tablet->tablet_schema().sequence_col_idx();
- if (_sequence_col_idx != -1) {
- for (auto col : _return_columns) {
- // query has sequence col
- if (col == _sequence_col_idx) {
- _has_sequence_col = true;
- break;
- }
+ DCHECK_NE(_sequence_col_idx, -1);
Review comment:
Because `has_sequence_col ` in line 600 is `inline bool has_sequence_col() const { return _sequence_col_idx != -1; }`, so `_sequence_col_idx` shoule always be `-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.
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] [incubator-doris] acelyc111 commented on a change in pull request #4925: [Refactor] Refactor DeleteHandler and Cond module
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on a change in pull request #4925:
URL: https://github.com/apache/incubator-doris/pull/4925#discussion_r528290975
##########
File path: be/src/olap/reader.cpp
##########
@@ -599,13 +599,12 @@ OLAPStatus Reader::_init_params(const ReaderParams& read_params) {
if (_tablet->tablet_schema().has_sequence_col()) {
_sequence_col_idx = _tablet->tablet_schema().sequence_col_idx();
- if (_sequence_col_idx != -1) {
- for (auto col : _return_columns) {
- // query has sequence col
- if (col == _sequence_col_idx) {
- _has_sequence_col = true;
- break;
- }
+ DCHECK_NE(_sequence_col_idx, -1);
Review comment:
@morningman Because `has_sequence_col() ` in line 600 is `inline bool has_sequence_col() const { return _sequence_col_idx != -1; }`, so `_sequence_col_idx` shoule always be `-1` here.
----------------------------------------------------------------
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.
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] [incubator-doris] morningman merged pull request #4925: [Refactor] Refactor DeleteHandler and Cond module
Posted by GitBox <gi...@apache.org>.
morningman merged pull request #4925:
URL: https://github.com/apache/incubator-doris/pull/4925
----------------------------------------------------------------
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.
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] [incubator-doris] morningman commented on a change in pull request #4925: [Refactor] Refactor DeleteHandler and Cond module
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #4925:
URL: https://github.com/apache/incubator-doris/pull/4925#discussion_r528201502
##########
File path: be/src/olap/olap_cond.cpp
##########
@@ -382,28 +386,30 @@ int Cond::del_eval(const std::pair<WrapperField*, WrapperField*>& stat) const {
} else if (stat.first->is_null() && !stat.second->is_null()) {
ret = DEL_PARTIAL_SATISFIED;
} else {
+ DCHECK(false);
//不会出现min不为NULL，max为NULL
- ret = DEL_NOT_SATISFIED;
+ ret = DEL_SATISFIED;
Review comment:
I think `DEL_NOT_SATISFIED` is more safe. Or you'd better change DCHECK to CHECK
##########
File path: be/src/olap/delete_handler.cpp
##########
@@ -162,27 +165,23 @@ OLAPStatus DeleteConditionHandler::check_condition_valid(
// 检查指定的列是不是key，是不是float或double类型
const TabletColumn& column = schema.column(field_index);
-
if ((!column.is_key() && schema.keys_type() != KeysType::DUP_KEYS)
|| column.type() == OLAP_FIELD_TYPE_DOUBLE
|| column.type() == OLAP_FIELD_TYPE_FLOAT) {
LOG(WARNING) << "field is not key column, or storage model is not duplicate, or data type is float or double.";
return OLAP_ERR_DELETE_INVALID_CONDITION;
}
- // 检查删除条件中指定的过滤值是否符合每个类型自身的要求
- // 1. 对于整数类型(int8,int16,in32,int64,uint8,uint16,uint32,uint64)，检查是否溢出
- // 2. 对于decimal类型，检查是否超过建表时指定的精度和标度
- // 3. 对于date和datetime类型，检查指定的过滤值是否符合日期格式以及是否指定错误的值
- // 4. 对于string和varchar类型，检查指定的过滤值是否超过建表时指定的长度
+ // 检查'op'与'operands'数是否合法
Review comment:
I think we can translate it to English this time
##########
File path: be/src/olap/reader.cpp
##########
@@ -599,13 +599,12 @@ OLAPStatus Reader::_init_params(const ReaderParams& read_params) {
if (_tablet->tablet_schema().has_sequence_col()) {
_sequence_col_idx = _tablet->tablet_schema().sequence_col_idx();
- if (_sequence_col_idx != -1) {
- for (auto col : _return_columns) {
- // query has sequence col
- if (col == _sequence_col_idx) {
- _has_sequence_col = true;
- break;
- }
+ DCHECK_NE(_sequence_col_idx, -1);
Review comment:
Why using DCHECK here?
##########
File path: be/src/olap/olap_cond.cpp
##########
@@ -382,28 +386,30 @@ int Cond::del_eval(const std::pair<WrapperField*, WrapperField*>& stat) const {
} else if (stat.first->is_null() && !stat.second->is_null()) {
ret = DEL_PARTIAL_SATISFIED;
} else {
+ DCHECK(false);
//不会出现min不为NULL，max为NULL
- ret = DEL_NOT_SATISFIED;
+ ret = DEL_SATISFIED;
}
} else {
if (stat.first->is_null() && stat.second->is_null()) {
ret = DEL_NOT_SATISFIED;
} else if (stat.first->is_null() && !stat.second->is_null()) {
ret = DEL_PARTIAL_SATISFIED;
} else {
+ DCHECK(false);
Review comment:
Change to CHECK, or ret = DEL_NOT_SATISFIED
----------------------------------------------------------------
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.
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] [incubator-doris] acelyc111 commented on a change in pull request #4925: [Refactor] Refactor DeleteHandler and Cond module
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on a change in pull request #4925:
URL: https://github.com/apache/incubator-doris/pull/4925#discussion_r528290975
##########
File path: be/src/olap/reader.cpp
##########
@@ -599,13 +599,12 @@ OLAPStatus Reader::_init_params(const ReaderParams& read_params) {
if (_tablet->tablet_schema().has_sequence_col()) {
_sequence_col_idx = _tablet->tablet_schema().sequence_col_idx();
- if (_sequence_col_idx != -1) {
- for (auto col : _return_columns) {
- // query has sequence col
- if (col == _sequence_col_idx) {
- _has_sequence_col = true;
- break;
- }
+ DCHECK_NE(_sequence_col_idx, -1);
Review comment:
Because `has_sequence_col() ` in line 600 is `inline bool has_sequence_col() const { return _sequence_col_idx != -1; }`, so `_sequence_col_idx` shoule always be `-1` here.
----------------------------------------------------------------
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.
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