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/07/02 14:09:40 UTC
[GitHub] [incubator-doris] caiconghui opened a new pull request #4006: [Feature] Support InPredicate in delete statement
caiconghui opened a new pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006
This PR is to add inPredicate support to delete statement, and add max_allowed_in_element_num_of_delete variable to limit element num of InPredicate in delete statement.
----------------------------------------------------------------
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 pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
morningman commented on pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#issuecomment-653043560
In fact, we are also trying to implement a batch delete function that can support the deletion of a large number of specified keys (including specifying multiple values in inPredicate)
In your current implementation, if there are too many values in InPredicate (for example, tens of thousands), then each column must be looped through tens of thousands of conditions during query, which may be inefficient.
Maybe you can discuss it with @yangzhg about it.
----------------------------------------------------------------
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 #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r450280629
##########
File path: fe/src/main/java/org/apache/doris/load/DeleteHandler.java
##########
@@ -516,25 +537,22 @@ private void checkDeleteV2(OlapTable table, Partition partition, List<Predicate>
}
Map<Long, List<Column>> indexIdToSchema = table.getIndexIdToSchema();
for (MaterializedIndex index : partition.getMaterializedIndices(MaterializedIndex.IndexExtState.VISIBLE)) {
+ if (table.getBaseIndexId() == index.getId()) {
+ continue;
Review comment:
Oh I see.
----------------------------------------------------------------
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] caiconghui commented on a change in pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r462418665
##########
File path: be/src/olap/olap_cond.cpp
##########
@@ -176,6 +150,14 @@ OLAPStatus Cond::init(const TCondition& tcond, const TabletColumn& column) {
tcond.column_name.c_str(), operand.c_str(), op);
return res;
}
+ if (min_value_field == nullptr || f->cmp(min_value_field) > 0) {
Review comment:
my bad, I will fix it A->cmp(B) > 0 means A > B
##########
File path: be/src/olap/olap_cond.h
##########
@@ -87,11 +88,13 @@ struct Cond {
}
CondOp op;
- // valid when op is not OP_IN
+ // valid when op is not OP_IN and OP_NOT_IN
WrapperField* operand_field;
- // valid when op is OP_IN
+ // valid when op is OP_IN or OP_NOT_IN
typedef std::unordered_set<const WrapperField*, FieldHash, FieldEqual> FieldSet;
FieldSet operand_set;
+ WrapperField* min_value_field;
Review comment:
done
##########
File path: fe/fe-core/src/main/java/org/apache/doris/load/DeleteHandler.java
##########
@@ -507,7 +516,19 @@ private void checkDeleteV2(OlapTable table, Partition partition, List<Predicate>
LiteralExpr.create(value, Type.fromPrimitiveType(column.getDataType()));
} catch (AnalysisException e) {
// ErrorReport.reportDdlException(ErrorCode.ERR_INVALID_VALUE, value);
- throw new DdlException("Invalid column value[" + value + "]");
+ throw new DdlException("Invalid column value[" + value + "] for column " + columnName);
+ }
+ } else if (condition instanceof InPredicate) {
+ String value = null;
+ try {
+ InPredicate inPredicate = (InPredicate) condition;
+ for (int i = 1; i <= inPredicate.getInElementNum(); i++) {
+ value = ((LiteralExpr) inPredicate.getChild(i)).getStringValue();
+ LiteralExpr.create(value, Type.fromPrimitiveType(column.getDataType()));
+ }
+ } catch (AnalysisException e) {
+ // ErrorReport.reportDdlException(ErrorCode.ERR_INVALID_VALUE, value);
Review comment:
done
##########
File path: be/src/olap/olap_cond.cpp
##########
@@ -210,19 +192,19 @@ bool Cond::eval(const RowCursorCell& cell) const {
case OP_GE:
return operand_field->field()->compare_cell(*operand_field, cell) <= 0;
case OP_IN: {
- for (const WrapperField* field : operand_set) {
- if (field->field()->compare_cell(*field, cell) == 0) {
- return true;
- }
- }
- return false;
+ WrapperField wrapperField(const_cast<Field *> (min_value_field->field()), cell);
+ auto ret = operand_set.find(&wrapperField) != operand_set.end();
+ wrapperField.release_field();
+ return ret;
+ }
+ case OP_NOT_IN: {
Review comment:
one is "!=", the other is "=="
##########
File path: be/src/olap/olap_cond.cpp
##########
@@ -210,19 +192,19 @@ bool Cond::eval(const RowCursorCell& cell) const {
case OP_GE:
return operand_field->field()->compare_cell(*operand_field, cell) <= 0;
case OP_IN: {
- for (const WrapperField* field : operand_set) {
- if (field->field()->compare_cell(*field, cell) == 0) {
- return true;
- }
- }
- return false;
+ WrapperField wrapperField(const_cast<Field *> (min_value_field->field()), cell);
+ auto ret = operand_set.find(&wrapperField) != operand_set.end();
+ wrapperField.release_field();
+ return ret;
+ }
+ case OP_NOT_IN: {
Review comment:
one is "!="(line 196), the other is "=="(line 202)
----------------------------------------------------------------
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] caiconghui commented on a change in pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r450222851
##########
File path: fe/src/main/java/org/apache/doris/load/DeleteHandler.java
##########
@@ -516,25 +537,22 @@ private void checkDeleteV2(OlapTable table, Partition partition, List<Predicate>
}
Map<Long, List<Column>> indexIdToSchema = table.getIndexIdToSchema();
for (MaterializedIndex index : partition.getMaterializedIndices(MaterializedIndex.IndexExtState.VISIBLE)) {
+ if (table.getBaseIndexId() == index.getId()) {
+ continue;
Review comment:
I find here double check, because base schema is equal to table schema? pre code has check table schema column 479 ~ 537
----------------------------------------------------------------
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 #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r449891254
##########
File path: fe/src/main/java/org/apache/doris/load/DeleteHandler.java
##########
@@ -516,25 +537,22 @@ private void checkDeleteV2(OlapTable table, Partition partition, List<Predicate>
}
Map<Long, List<Column>> indexIdToSchema = table.getIndexIdToSchema();
for (MaterializedIndex index : partition.getMaterializedIndices(MaterializedIndex.IndexExtState.VISIBLE)) {
+ if (table.getBaseIndexId() == index.getId()) {
+ continue;
Review comment:
Why skipping the base index?
If there is no rollup index, the entire check will be skipped.
##########
File path: be/src/olap/delete_handler.cpp
##########
@@ -66,9 +67,22 @@ OLAPStatus DeleteConditionHandler::generate_delete_predicate(
// 存储删除条件
for (const TCondition& condition : conditions) {
- string condition_str = construct_sub_predicates(condition);
- del_pred->add_sub_predicates(condition_str);
- LOG(INFO) << "store one sub-delete condition. condition=" << condition_str;
+ if (condition.condition_values.size() > 1) {
+ InPredicatePB* in_pred = del_pred->add_in_predicates();
+ in_pred->set_column_name(condition.column_name);
+ bool is_not_in = condition.condition_op == "!*=";
+ in_pred->set_is_not_in(is_not_in);
+ for (const auto& condition_value : condition.condition_values) {
+ in_pred->add_values(condition_value);
+ }
+ string condition_str;
+ json2pb::ProtoMessageToJson(*in_pred, &condition_str);
+ LOG(INFO) << "store one sub-delete condition. condition=" << condition_str;
Review comment:
This log may be very long. the number of conditions and column name is enough.
##########
File path: fe/src/main/java/org/apache/doris/qe/SessionVariable.java
##########
@@ -248,6 +249,8 @@
private int maxScanKeyNum = -1;
@VariableMgr.VarAttr(name = MAX_PUSHDOWN_CONDITIONS_PER_COLUMN)
private int maxPushdownConditionsPerColumn = -1;
+ @VariableMgr.VarAttr(name = MAX_ALLOWED_IN_ELEMENT_NUM_OF_DELETE)
+ private int maxAllowedInElementNumOfDelete = 1024;
Review comment:
Is it better be a FE's config instead of a session variable?
I think this should be a system-level restriction to prevent users from writing too many conditions, resulting in a reduction in the efficiency of reading and writing the underlying data in storage system.
##########
File path: be/src/olap/olap_cond.cpp
##########
@@ -57,77 +57,49 @@ using doris::ColumnStatistics;
namespace doris {
static CondOp parse_op_type(const string& op) {
- if (op.size() > 2) {
+ if (op.size() > 3) {
Review comment:
Could we defined this magic number somewhere?
##########
File path: be/src/olap/olap_cond.cpp
##########
@@ -378,20 +347,31 @@ int Cond::del_eval(const std::pair<WrapperField*, WrapperField*>& stat) const {
return ret;
}
case OP_IN: {
- FieldSet::const_iterator it = operand_set.begin();
- for (; it != operand_set.end(); ++it) {
- if ((*it)->cmp(stat.first) >= 0
- && (*it)->cmp(stat.second) <= 0) {
- if (stat.first->cmp(stat.second) == 0) {
- ret = DEL_SATISFIED;
- } else {
- ret = DEL_PARTIAL_SATISFIED;
- }
- break;
+ if (stat.first->cmp(stat.second) == 0) {
+ if (operand_set.find(stat.first) != operand_set.end()) {
+ ret = DEL_SATISFIED;
+ } else {
+ ret = DEL_NOT_SATISFIED;
+ }
+ } else {
+ if ((min_value_field->cmp(stat.first) >= 0 && min_value_field->cmp(stat.second) <= 0) ||
Review comment:
Should this be:
`min_value_field->cmp(stat.second) <= 0 && max_value_field->cmp(stat.first) >= 0`
?
##########
File path: be/src/olap/olap_cond.h
##########
@@ -46,7 +46,8 @@ enum CondOp {
OP_GE = 5, // greater or equal
OP_IN = 6, // IN
OP_IS = 7, // is null or not null
- OP_NULL = 8 // invalid OP
+ OP_NULL = 8, // invalid OP
+ OP_NOT_IN = 9 // not in
Review comment:
Normally, the invalid OP should be at the beginning(-1) or at last of the enum.
I am not sure if we persist the order of CondOp in storage. If not, I think its better to move the `OP_NULL` to the beginning and set it as -1.
##########
File path: be/src/olap/olap_cond.cpp
##########
@@ -262,28 +242,17 @@ bool Cond::eval(const std::pair<WrapperField*, WrapperField*>& statistic) const
return operand_field->cmp(statistic.second) <= 0;
}
case OP_IN: {
- FieldSet::const_iterator it = operand_set.begin();
- for (; it != operand_set.end(); ++it) {
- if ((*it)->cmp(statistic.first) >= 0
- && (*it)->cmp(statistic.second) <= 0) {
- return true;
- }
- }
- break;
+ return (min_value_field->cmp(statistic.first) >= 0 && min_value_field->cmp(statistic.second) <= 0) ||
Review comment:
Should this be:
`min_value_field->cmp(stat.second) <= 0 && max_value_field->cmp(stat.first) >= 0`
?
----------------------------------------------------------------
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] caiconghui commented on a change in pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r464236696
##########
File path: gensrc/proto/olap_file.proto
##########
@@ -181,6 +181,13 @@ enum KeysType {
message DeletePredicatePB {
required int32 version = 1;
repeated string sub_predicates = 2;
+ repeated InPredicatePB in_predicates = 3;
+}
+
+message InPredicatePB {
Review comment:
done
----------------------------------------------------------------
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] caiconghui commented on a change in pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r450646519
##########
File path: fe/src/main/java/org/apache/doris/load/DeleteHandler.java
##########
@@ -569,6 +587,19 @@ private void checkDeleteV2(OlapTable table, Partition partition, List<Predicate>
sb.append(" IS NULL");
}
deleteConditions.add(sb.toString());
+ } else if (condition instanceof InPredicate) {
+ InPredicate inPredicate = (InPredicate) condition;
+ SlotRef slotRef = (SlotRef) inPredicate.getChild(0);
+ String columnName = slotRef.getColumnName();
+ StringBuilder strBuilder = new StringBuilder();
+ String notStr = inPredicate.isNotIn() ? "NOT " : "";
+ strBuilder.append(columnName).append(" " + notStr + "IN (");
+ for (int i = 1; i <= inPredicate.getInElementNum(); ++i) {
+ strBuilder.append(inPredicate.getChild(i).toSql());
+ strBuilder.append((i + 1 != inPredicate.getInElementNum()) ? ", " : "");
Review comment:
oh， mybad，thanks，I will fix it
##########
File path: fe/src/main/java/org/apache/doris/load/DeleteHandler.java
##########
@@ -569,6 +587,19 @@ private void checkDeleteV2(OlapTable table, Partition partition, List<Predicate>
sb.append(" IS NULL");
}
deleteConditions.add(sb.toString());
+ } else if (condition instanceof InPredicate) {
+ InPredicate inPredicate = (InPredicate) condition;
+ SlotRef slotRef = (SlotRef) inPredicate.getChild(0);
+ String columnName = slotRef.getColumnName();
+ StringBuilder strBuilder = new StringBuilder();
+ String notStr = inPredicate.isNotIn() ? "NOT " : "";
+ strBuilder.append(columnName).append(" " + notStr + "IN (");
+ for (int i = 1; i <= inPredicate.getInElementNum(); ++i) {
+ strBuilder.append(inPredicate.getChild(i).toSql());
+ strBuilder.append((i + 1 != inPredicate.getInElementNum()) ? ", " : "");
Review comment:
oh， my bad，thanks，I will fix it
----------------------------------------------------------------
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 #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
morningman merged pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006
----------------------------------------------------------------
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] caiconghui commented on a change in pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r450637016
##########
File path: fe/src/main/java/org/apache/doris/load/DeleteHandler.java
##########
@@ -507,7 +516,19 @@ private void checkDeleteV2(OlapTable table, Partition partition, List<Predicate>
LiteralExpr.create(value, Type.fromPrimitiveType(column.getDataType()));
} catch (AnalysisException e) {
// ErrorReport.reportDdlException(ErrorCode.ERR_INVALID_VALUE, value);
- throw new DdlException("Invalid column value[" + value + "]");
+ throw new DdlException("Invalid column value[" + value + "] for column " + columnName);
+ }
+ } else if (condition instanceof InPredicate) {
+ String value = null;
+ try {
+ InPredicate inPredicate = (InPredicate) condition;
+ for (int i = 1; i <= inPredicate.getInElementNum(); i++) {
+ value = ((LiteralExpr) inPredicate.getChild(i)).getStringValue();
+ LiteralExpr.create(value, Type.fromPrimitiveType(column.getDataType()));
+ }
Review comment:
check valid or throw exception
----------------------------------------------------------------
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] wutiangan commented on a change in pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
wutiangan commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r450627407
##########
File path: fe/src/main/java/org/apache/doris/load/DeleteHandler.java
##########
@@ -569,6 +587,19 @@ private void checkDeleteV2(OlapTable table, Partition partition, List<Predicate>
sb.append(" IS NULL");
}
deleteConditions.add(sb.toString());
+ } else if (condition instanceof InPredicate) {
+ InPredicate inPredicate = (InPredicate) condition;
+ SlotRef slotRef = (SlotRef) inPredicate.getChild(0);
+ String columnName = slotRef.getColumnName();
+ StringBuilder strBuilder = new StringBuilder();
+ String notStr = inPredicate.isNotIn() ? "NOT " : "";
+ strBuilder.append(columnName).append(" " + notStr + "IN (");
+ for (int i = 1; i <= inPredicate.getInElementNum(); ++i) {
+ strBuilder.append(inPredicate.getChild(i).toSql());
+ strBuilder.append((i + 1 != inPredicate.getInElementNum()) ? ", " : "");
Review comment:
the last loop is “ i == inPredicate.getInElementNum()”, should " i + 1 != inPredicate.getInElementNum()" modify "i != inPredicate.getInElementNum()"?
----------------------------------------------------------------
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] kangkaisen commented on a change in pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
kangkaisen commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r464158371
##########
File path: gensrc/proto/olap_file.proto
##########
@@ -181,6 +181,13 @@ enum KeysType {
message DeletePredicatePB {
required int32 version = 1;
repeated string sub_predicates = 2;
+ repeated InPredicatePB in_predicates = 3;
+}
+
+message InPredicatePB {
Review comment:
For protobuf, we prefer optional than required, because we maybe remove or rename `column_name` from `InPredicatePB`. Please change all `required ` to `optional`.
----------------------------------------------------------------
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 pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
morningman commented on pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#issuecomment-667507130
UT failed.
FE：
DeleteStmtTest.testAnalyze
----------------------------------------------------------------
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 #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r463960031
##########
File path: fe/fe-core/src/main/java/org/apache/doris/common/Config.java
##########
@@ -1192,4 +1191,11 @@
*/
@ConfField(mutable = true, masterOnly = false)
public static int cache_result_max_row_count = 3000;
+
+ /**
+ * Used to limit element num of InPredicate in delete statement.
+ */
+ @ConfField(mutable = true, masterOnly = true)
Review comment:
```suggestion
@ConfField(mutable = true)
```
This is not a "master only" config.
##########
File path: be/src/olap/delete_handler.cpp
##########
@@ -66,9 +67,22 @@ OLAPStatus DeleteConditionHandler::generate_delete_predicate(
// 存储删除条件
for (const TCondition& condition : conditions) {
- string condition_str = construct_sub_predicates(condition);
- del_pred->add_sub_predicates(condition_str);
- LOG(INFO) << "store one sub-delete condition. condition=" << condition_str;
+ if (condition.condition_values.size() > 1) {
Review comment:
I think it's better to check the `condition.condition_op` to see if this condition is InPredicate, not by the size of `condition.condition_values`.
----------------------------------------------------------------
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] caiconghui commented on a change in pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r463969349
##########
File path: be/src/olap/delete_handler.cpp
##########
@@ -66,9 +67,22 @@ OLAPStatus DeleteConditionHandler::generate_delete_predicate(
// 存储删除条件
for (const TCondition& condition : conditions) {
- string condition_str = construct_sub_predicates(condition);
- del_pred->add_sub_predicates(condition_str);
- LOG(INFO) << "store one sub-delete condition. condition=" << condition_str;
+ if (condition.condition_values.size() > 1) {
Review comment:
because inPredicate with one element to stored as BinaryPredicate which is more more light and efficient.
----------------------------------------------------------------
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] wutiangan commented on a change in pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
wutiangan commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r450632141
##########
File path: fe/src/main/java/org/apache/doris/load/DeleteHandler.java
##########
@@ -507,7 +516,19 @@ private void checkDeleteV2(OlapTable table, Partition partition, List<Predicate>
LiteralExpr.create(value, Type.fromPrimitiveType(column.getDataType()));
} catch (AnalysisException e) {
// ErrorReport.reportDdlException(ErrorCode.ERR_INVALID_VALUE, value);
- throw new DdlException("Invalid column value[" + value + "]");
+ throw new DdlException("Invalid column value[" + value + "] for column " + columnName);
+ }
+ } else if (condition instanceof InPredicate) {
+ String value = null;
+ try {
+ InPredicate inPredicate = (InPredicate) condition;
+ for (int i = 1; i <= inPredicate.getInElementNum(); i++) {
+ value = ((LiteralExpr) inPredicate.getChild(i)).getStringValue();
+ LiteralExpr.create(value, Type.fromPrimitiveType(column.getDataType()));
+ }
Review comment:
what does the for loop do? LiteralExpr.create has no return value?
----------------------------------------------------------------
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] wutiangan commented on a change in pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
wutiangan commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r450628421
##########
File path: fe/src/main/java/org/apache/doris/load/DeleteHandler.java
##########
@@ -450,7 +451,22 @@ public DeleteJob getDeleteJob(long transactionId) {
return idToDeleteJob.get(transactionId);
}
- private void checkDeleteV2(OlapTable table, Partition partition, List<Predicate> conditions, List<String> deleteConditions, boolean preCheck)
+ private SlotRef getSlotRef(Predicate condition) {
+ SlotRef slotRef = null;
+ if (condition instanceof BinaryPredicate) {
+ BinaryPredicate binaryPredicate = (BinaryPredicate) condition;
+ slotRef = (SlotRef) binaryPredicate.getChild(0);
Review comment:
can slotRef be the second child of binaryPreidicate?
----------------------------------------------------------------
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 #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r462319981
##########
File path: fe/fe-core/src/main/java/org/apache/doris/analysis/DeleteStmt.java
##########
@@ -119,11 +120,29 @@ private void analyzePredicate(Expr predicate) throws AnalysisException {
IsNullPredicate isNullPredicate = (IsNullPredicate) predicate;
Expr leftExpr = isNullPredicate.getChild(0);
if (!(leftExpr instanceof SlotRef)) {
- throw new AnalysisException("Left expr should be column name");
+ throw new AnalysisException("Left expr of is_null predicate should be column name");
}
deleteConditions.add(isNullPredicate);
+ } else if (predicate instanceof InPredicate) {
+ InPredicate inPredicate = (InPredicate) predicate;
+ Expr leftExpr = inPredicate.getChild(0);
+ if (!(leftExpr instanceof SlotRef)) {
+ throw new AnalysisException("Left expr of in predicate should be column name");
+ }
+ int inElementNum = inPredicate.getInElementNum();
+ int maxAllowedInElementNumOfDelete = Config.max_allowed_in_element_num_of_delete;
+ if (inElementNum > maxAllowedInElementNumOfDelete) {
+ throw new AnalysisException("Element num of in predicate should not be more than " + maxAllowedInElementNumOfDelete);
+ }
+ for (int i = 1; i <= inPredicate.getInElementNum(); i++) {
+ Expr expr = inPredicate.getChild(i);
+ if (!(expr instanceof LiteralExpr)) {
+ throw new AnalysisException("Right expr of binary predicate should be value");
Review comment:
```suggestion
throw new AnalysisException("Child of in predicate should be value");
```
##########
File path: fe/fe-core/src/main/java/org/apache/doris/analysis/DeleteStmt.java
##########
@@ -119,11 +120,29 @@ private void analyzePredicate(Expr predicate) throws AnalysisException {
IsNullPredicate isNullPredicate = (IsNullPredicate) predicate;
Expr leftExpr = isNullPredicate.getChild(0);
if (!(leftExpr instanceof SlotRef)) {
- throw new AnalysisException("Left expr should be column name");
+ throw new AnalysisException("Left expr of is_null predicate should be column name");
}
deleteConditions.add(isNullPredicate);
+ } else if (predicate instanceof InPredicate) {
+ InPredicate inPredicate = (InPredicate) predicate;
+ Expr leftExpr = inPredicate.getChild(0);
+ if (!(leftExpr instanceof SlotRef)) {
+ throw new AnalysisException("Left expr of in predicate should be column name");
+ }
+ int inElementNum = inPredicate.getInElementNum();
+ int maxAllowedInElementNumOfDelete = Config.max_allowed_in_element_num_of_delete;
+ if (inElementNum > maxAllowedInElementNumOfDelete) {
+ throw new AnalysisException("Element num of in predicate should not be more than " + maxAllowedInElementNumOfDelete);
+ }
+ for (int i = 1; i <= inPredicate.getInElementNum(); i++) {
+ Expr expr = inPredicate.getChild(i);
+ if (!(expr instanceof LiteralExpr)) {
+ throw new AnalysisException("Right expr of binary predicate should be value");
+ }
+ }
+ deleteConditions.add(inPredicate);
} else {
- throw new AnalysisException("Where clause should be compound or binary predicate");
+ throw new AnalysisException("Where clause should be compound or binary predicate or is_null predicate or in predicate");
Review comment:
```suggestion
throw new AnalysisException("Where clause only supports compound predicate, binary predicate, is_null predicate or in predicate");
```
##########
File path: be/src/olap/olap_cond.cpp
##########
@@ -210,19 +192,19 @@ bool Cond::eval(const RowCursorCell& cell) const {
case OP_GE:
return operand_field->field()->compare_cell(*operand_field, cell) <= 0;
case OP_IN: {
- for (const WrapperField* field : operand_set) {
- if (field->field()->compare_cell(*field, cell) == 0) {
- return true;
- }
- }
- return false;
+ WrapperField wrapperField(const_cast<Field *> (min_value_field->field()), cell);
+ auto ret = operand_set.find(&wrapperField) != operand_set.end();
+ wrapperField.release_field();
+ return ret;
+ }
+ case OP_NOT_IN: {
Review comment:
These 2 case (OP_IN and OP_NOT_IN) looks same?
##########
File path: fe/fe-core/src/main/java/org/apache/doris/load/DeleteHandler.java
##########
@@ -507,7 +516,19 @@ private void checkDeleteV2(OlapTable table, Partition partition, List<Predicate>
LiteralExpr.create(value, Type.fromPrimitiveType(column.getDataType()));
} catch (AnalysisException e) {
// ErrorReport.reportDdlException(ErrorCode.ERR_INVALID_VALUE, value);
- throw new DdlException("Invalid column value[" + value + "]");
+ throw new DdlException("Invalid column value[" + value + "] for column " + columnName);
+ }
+ } else if (condition instanceof InPredicate) {
+ String value = null;
+ try {
+ InPredicate inPredicate = (InPredicate) condition;
+ for (int i = 1; i <= inPredicate.getInElementNum(); i++) {
+ value = ((LiteralExpr) inPredicate.getChild(i)).getStringValue();
+ LiteralExpr.create(value, Type.fromPrimitiveType(column.getDataType()));
+ }
+ } catch (AnalysisException e) {
+ // ErrorReport.reportDdlException(ErrorCode.ERR_INVALID_VALUE, value);
Review comment:
Remove unused code
##########
File path: be/src/olap/olap_cond.h
##########
@@ -87,11 +88,13 @@ struct Cond {
}
CondOp op;
- // valid when op is not OP_IN
+ // valid when op is not OP_IN and OP_NOT_IN
WrapperField* operand_field;
- // valid when op is OP_IN
+ // valid when op is OP_IN or OP_NOT_IN
typedef std::unordered_set<const WrapperField*, FieldHash, FieldEqual> FieldSet;
FieldSet operand_set;
+ WrapperField* min_value_field;
Review comment:
Add comment to explains these 2 new fields.
##########
File path: be/src/olap/olap_cond.cpp
##########
@@ -176,6 +150,14 @@ OLAPStatus Cond::init(const TCondition& tcond, const TabletColumn& column) {
tcond.column_name.c_str(), operand.c_str(), op);
return res;
}
+ if (min_value_field == nullptr || f->cmp(min_value_field) > 0) {
Review comment:
I am a little confused.
`A->cmp(B) > 0` means `A > B` or `A < B`?
----------------------------------------------------------------
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] caiconghui commented on a change in pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r463969454
##########
File path: fe/fe-core/src/main/java/org/apache/doris/common/Config.java
##########
@@ -1192,4 +1191,11 @@
*/
@ConfField(mutable = true, masterOnly = false)
public static int cache_result_max_row_count = 3000;
+
+ /**
+ * Used to limit element num of InPredicate in delete statement.
+ */
+ @ConfField(mutable = true, masterOnly = true)
Review comment:
delete operation need a txn which only master can execute ?
----------------------------------------------------------------
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] caiconghui commented on pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
caiconghui commented on pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#issuecomment-653031083
for #4008
----------------------------------------------------------------
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] caiconghui commented on a change in pull request #4006: [Feature] Support InPredicate in delete statement
Posted by GitBox <gi...@apache.org>.
caiconghui commented on a change in pull request #4006:
URL: https://github.com/apache/incubator-doris/pull/4006#discussion_r450638700
##########
File path: fe/src/main/java/org/apache/doris/load/DeleteHandler.java
##########
@@ -450,7 +451,22 @@ public DeleteJob getDeleteJob(long transactionId) {
return idToDeleteJob.get(transactionId);
}
- private void checkDeleteV2(OlapTable table, Partition partition, List<Predicate> conditions, List<String> deleteConditions, boolean preCheck)
+ private SlotRef getSlotRef(Predicate condition) {
+ SlotRef slotRef = null;
+ if (condition instanceof BinaryPredicate) {
+ BinaryPredicate binaryPredicate = (BinaryPredicate) condition;
+ slotRef = (SlotRef) binaryPredicate.getChild(0);
Review comment:
not allowed
----------------------------------------------------------------
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