[GitHub] [incubator-doris] morningman commented on a change in pull request #4925: [Refactor] Refactor DeleteHandler and Cond module

[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