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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
