diff --git a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/BaseAndOrBooleanFilterOptimizer.java b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/BaseAndOrBooleanFilterOptimizer.java index e372ff100c78..5ef10d09721e 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/BaseAndOrBooleanFilterOptimizer.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/BaseAndOrBooleanFilterOptimizer.java @@ -21,9 +21,7 @@ import java.util.List; import javax.annotation.Nullable; import org.apache.pinot.common.request.Expression; -import org.apache.pinot.common.request.ExpressionType; import org.apache.pinot.common.request.Function; -import org.apache.pinot.common.request.Literal; import org.apache.pinot.common.utils.request.RequestUtils; import org.apache.pinot.spi.data.Schema; import org.apache.pinot.sql.FilterKind; @@ -58,7 +56,7 @@ public Expression optimize(Expression filterExpression, @Nullable Schema schema) case OR: case NOT: // Recursively traverse the expression tree to find an operator node that can be rewritten. - operands.forEach(operand -> optimize(operand, schema)); + operands.replaceAll(operand -> optimize(operand, schema)); // We have rewritten the child operands, so rewrite the parent if needed. return optimizeCurrent(filterExpression); @@ -88,54 +86,44 @@ protected Expression optimizeCurrent(Expression expression) { if (operator.equals(FilterKind.AND.name())) { // If any of the literal operands are always false, then replace AND function with FALSE. for (Expression operand : operands) { - if (isAlwaysFalse(operand)) { + if (operand.equals(FALSE)) { return FALSE; } } // Remove all Literal operands that are always true. - operands.removeIf(this::isAlwaysTrue); + operands.removeIf(operand -> operand.equals(TRUE)); if (operands.isEmpty()) { return TRUE; } } else if (operator.equals(FilterKind.OR.name())) { // If any of the literal operands are always true, then replace OR function with TRUE for (Expression operand : operands) { - if (isAlwaysTrue(operand)) { + if (operand.equals(TRUE)) { return TRUE; } } // Remove all Literal operands that are always false. - operands.removeIf(this::isAlwaysFalse); + operands.removeIf(operand -> operand.equals(FALSE)); if (operands.isEmpty()) { return FALSE; } } else if (operator.equals(FilterKind.NOT.name())) { assert operands.size() == 1; Expression operand = operands.get(0); - if (isAlwaysTrue(operand)) { + if (operand.equals(TRUE)) { return FALSE; } - if (isAlwaysFalse(operand)) { + if (operand.equals(FALSE)) { return TRUE; } } return expression; } - private boolean isAlwaysFalse(Expression operand) { - return operand.equals(FALSE); - } - - private boolean isAlwaysTrue(Expression operand) { - return operand.equals(TRUE); - } - /** Change the expression value to boolean literal with given value. */ - protected static void setExpressionToBoolean(Expression expression, boolean value) { - expression.unsetFunctionCall(); - expression.setType(ExpressionType.LITERAL); - expression.setLiteral(Literal.boolValue(value)); + protected static Expression getExpressionFromBoolean(boolean value) { + return value ? TRUE : FALSE; } } diff --git a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/IdenticalPredicateFilterOptimizer.java b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/IdenticalPredicateFilterOptimizer.java index b8e44fa2ead5..cd03842c6d47 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/IdenticalPredicateFilterOptimizer.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/IdenticalPredicateFilterOptimizer.java @@ -45,13 +45,13 @@ Expression optimizeChild(Expression filterExpression, @Nullable Schema schema) { FilterKind kind = FilterKind.valueOf(function.getOperator()); switch (kind) { case EQUALS: - if (hasIdenticalLhsAndRhs(filterExpression)) { - setExpressionToBoolean(filterExpression, true); + if (hasIdenticalLhsAndRhs(function.getOperands())) { + return TRUE; } break; case NOT_EQUALS: - if (hasIdenticalLhsAndRhs(filterExpression)) { - setExpressionToBoolean(filterExpression, false); + if (hasIdenticalLhsAndRhs(function.getOperands())) { + return FALSE; } break; default: @@ -68,15 +68,13 @@ Expression optimizeChild(Expression filterExpression, @Nullable Schema schema) { * We return false specifically after every check to ensure we're only continuing when * the input looks as expected. Otherwise, it's easy to for one of the operand functions * to return null and fail the query. + * + * TODO: The rewrite is already happening in PredicateComparisonRewriter.updateFunctionExpression(), + * so we might just compare the lhs and rhs there. */ - private boolean hasIdenticalLhsAndRhs(Expression operand) { - Function function = operand.getFunctionCall(); - if (function == null) { - return false; - } - List children = function.getOperands(); - boolean hasTwoChildren = children.size() == 2; - Expression firstChild = children.get(0); + private boolean hasIdenticalLhsAndRhs(List operands) { + boolean hasTwoChildren = operands.size() == 2; + Expression firstChild = operands.get(0); if (firstChild.getFunctionCall() == null || !hasTwoChildren) { return false; } @@ -94,7 +92,7 @@ private boolean hasIdenticalLhsAndRhs(Expression operand) { minusOperandSecondChild)) { return false; } - Expression secondChild = children.get(1); + Expression secondChild = operands.get(1); return isLiteralZero(secondChild); } diff --git a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/NumericalFilterOptimizer.java b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/NumericalFilterOptimizer.java index 93e17ae17e0e..80c723f25805 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/NumericalFilterOptimizer.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/NumericalFilterOptimizer.java @@ -127,7 +127,7 @@ private static Expression rewriteEqualsExpression(Expression equals, FilterKind int converted = (int) actual; if (converted != actual) { // Long value does not fall within the bounds of INT column. - setExpressionToBoolean(equals, result); + return getExpressionFromBoolean(result); } else { // Replace long value with converted int value. rhs.getLiteral().setLongValue(converted); @@ -138,7 +138,7 @@ private static Expression rewriteEqualsExpression(Expression equals, FilterKind float converted = (float) actual; if (BigDecimal.valueOf(actual).compareTo(BigDecimal.valueOf(converted)) != 0) { // Long to float conversion is lossy. - setExpressionToBoolean(equals, result); + return getExpressionFromBoolean(result); } else { // Replace long value with converted float value. rhs.getLiteral().setDoubleValue(converted); @@ -149,7 +149,7 @@ private static Expression rewriteEqualsExpression(Expression equals, FilterKind double converted = (double) actual; if (BigDecimal.valueOf(actual).compareTo(BigDecimal.valueOf(converted)) != 0) { // Long to double conversion is lossy. - setExpressionToBoolean(equals, result); + return getExpressionFromBoolean(result); } else { // Replace long value with converted double value. rhs.getLiteral().setDoubleValue(converted); @@ -168,7 +168,7 @@ private static Expression rewriteEqualsExpression(Expression equals, FilterKind int converted = (int) actual; if (converted != actual) { // Double value does not fall within the bounds of INT column. - setExpressionToBoolean(equals, result); + return getExpressionFromBoolean(result); } else { // Replace double value with converted int value. rhs.getLiteral().setLongValue(converted); @@ -179,7 +179,7 @@ private static Expression rewriteEqualsExpression(Expression equals, FilterKind long converted = (long) actual; if (BigDecimal.valueOf(actual).compareTo(BigDecimal.valueOf(converted)) != 0) { // Double to long conversion is lossy. - setExpressionToBoolean(equals, result); + return getExpressionFromBoolean(result); } else { // Replace double value with converted long value. rhs.getLiteral().setLongValue(converted); @@ -190,7 +190,7 @@ private static Expression rewriteEqualsExpression(Expression equals, FilterKind float converted = (float) actual; if (converted != actual) { // Double to float conversion is lossy. - setExpressionToBoolean(equals, result); + return getExpressionFromBoolean(result); } else { // Replace double value with converted float value. rhs.getLiteral().setDoubleValue(converted); @@ -234,13 +234,12 @@ private static Expression rewriteRangeExpression(Expression range, FilterKind ki // INT column can never have a value greater than Integer.MAX_VALUE. < and <= expressions will always be // true, because an INT column will always have values greater than or equal to Integer.MIN_VALUE and less // than or equal to Integer.MAX_VALUE. - setExpressionToBoolean(range, kind == FilterKind.LESS_THAN || kind == FilterKind.LESS_THAN_OR_EQUAL); + return getExpressionFromBoolean(kind == FilterKind.LESS_THAN || kind == FilterKind.LESS_THAN_OR_EQUAL); } else if (comparison < 0) { // Literal value is less than the bounds of INT. > and >= expressions will always be true because an // INT column will always have a value greater than or equal to Integer.MIN_VALUE. < and <= expressions // will always be false, because an INT column will never have values less than Integer.MIN_VALUE. - setExpressionToBoolean(range, - kind == FilterKind.GREATER_THAN || kind == FilterKind.GREATER_THAN_OR_EQUAL); + return getExpressionFromBoolean(kind == FilterKind.GREATER_THAN || kind == FilterKind.GREATER_THAN_OR_EQUAL); } else { // Long literal value falls within the bounds of INT column, server will successfully convert the literal // value when needed. @@ -291,10 +290,10 @@ private static Expression rewriteRangeExpression(Expression range, FilterKind ki int comparison = Double.compare(actual, converted); if (comparison > 0 && converted == Integer.MAX_VALUE) { // Literal value is greater than the bounds of INT. - setExpressionToBoolean(range, kind == FilterKind.LESS_THAN || kind == FilterKind.LESS_THAN_OR_EQUAL); + return getExpressionFromBoolean(kind == FilterKind.LESS_THAN || kind == FilterKind.LESS_THAN_OR_EQUAL); } else if (comparison < 0 && converted == Integer.MIN_VALUE) { // Literal value is less than the bounds of INT. - setExpressionToBoolean(range, + return getExpressionFromBoolean( kind == FilterKind.GREATER_THAN || kind == FilterKind.GREATER_THAN_OR_EQUAL); } else { // Literal value falls within the bounds of INT. @@ -311,10 +310,10 @@ private static Expression rewriteRangeExpression(Expression range, FilterKind ki if (comparison > 0 && converted == Long.MAX_VALUE) { // Literal value is greater than the bounds of LONG. - setExpressionToBoolean(range, kind == FilterKind.LESS_THAN || kind == FilterKind.LESS_THAN_OR_EQUAL); + return getExpressionFromBoolean(kind == FilterKind.LESS_THAN || kind == FilterKind.LESS_THAN_OR_EQUAL); } else if (comparison < 0 && converted == Long.MIN_VALUE) { // Literal value is less than the bounds of LONG. - setExpressionToBoolean(range, + return getExpressionFromBoolean( kind == FilterKind.GREATER_THAN || kind == FilterKind.GREATER_THAN_OR_EQUAL); } else { // Rewrite range operator @@ -329,10 +328,10 @@ private static Expression rewriteRangeExpression(Expression range, FilterKind ki float converted = (float) actual; if (converted == Float.POSITIVE_INFINITY) { // Literal value is greater than the bounds of FLOAT - setExpressionToBoolean(range, kind == FilterKind.LESS_THAN || kind == FilterKind.LESS_THAN_OR_EQUAL); + return getExpressionFromBoolean(kind == FilterKind.LESS_THAN || kind == FilterKind.LESS_THAN_OR_EQUAL); } else if (converted == Float.NEGATIVE_INFINITY) { // Literal value is less than the bounds of LONG. - setExpressionToBoolean(range, + return getExpressionFromBoolean( kind == FilterKind.GREATER_THAN || kind == FilterKind.GREATER_THAN_OR_EQUAL); } else { int comparison = Double.compare(actual, converted);