From 32da3e27300ad348f14c85fd012139c09f381ba6 Mon Sep 17 00:00:00 2001 From: Johan Adami Date: Sun, 19 Mar 2023 15:14:58 -0400 Subject: [PATCH 1/8] optimize queries where lhs and rhs of predicate are equal --- .../core/query/optimizer/QueryOptimizer.java | 9 +- .../BaseAndOrBooleanFilterOptimizer.java | 76 ++++++++++++++ .../filter/FlattenAndOrFilterOptimizer.java | 5 +- .../IdenticalPredicateFilterOptimizer.java | 98 +++++++++++++++++++ .../filter/MergeEqInFilterOptimizer.java | 87 ++++++++-------- .../filter/NumericalFilterOptimizer.java | 53 +--------- .../query/optimizer/QueryOptimizerTest.java | 68 +++++++++---- 7 files changed, 275 insertions(+), 121 deletions(-) create mode 100644 pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/BaseAndOrBooleanFilterOptimizer.java create mode 100644 pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/IdenticalPredicateFilterOptimizer.java diff --git a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java index d0b0c549c34a..695179d68be1 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java @@ -24,12 +24,7 @@ import javax.annotation.Nullable; import org.apache.pinot.common.request.Expression; import org.apache.pinot.common.request.PinotQuery; -import org.apache.pinot.core.query.optimizer.filter.FilterOptimizer; -import org.apache.pinot.core.query.optimizer.filter.FlattenAndOrFilterOptimizer; -import org.apache.pinot.core.query.optimizer.filter.MergeEqInFilterOptimizer; -import org.apache.pinot.core.query.optimizer.filter.MergeRangeFilterOptimizer; -import org.apache.pinot.core.query.optimizer.filter.NumericalFilterOptimizer; -import org.apache.pinot.core.query.optimizer.filter.TimePredicateFilterOptimizer; +import org.apache.pinot.core.query.optimizer.filter.*; import org.apache.pinot.core.query.optimizer.statement.StatementOptimizer; import org.apache.pinot.core.query.optimizer.statement.StringPredicateFilterOptimizer; import org.apache.pinot.spi.config.table.TableConfig; @@ -44,7 +39,7 @@ public class QueryOptimizer { // values to the proper format so that they can be properly parsed private static final List FILTER_OPTIMIZERS = Arrays.asList(new FlattenAndOrFilterOptimizer(), new MergeEqInFilterOptimizer(), new NumericalFilterOptimizer(), - new TimePredicateFilterOptimizer(), new MergeRangeFilterOptimizer()); + new TimePredicateFilterOptimizer(), new MergeRangeFilterOptimizer(), new IdenticalPredicateFilterOptimizer()); private static final List STATEMENT_OPTIMIZERS = Collections.singletonList(new StringPredicateFilterOptimizer()); 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 new file mode 100644 index 000000000000..643f2c01ef5a --- /dev/null +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/BaseAndOrBooleanFilterOptimizer.java @@ -0,0 +1,76 @@ +package org.apache.pinot.core.query.optimizer.filter; + +import org.apache.pinot.common.request.Expression; +import org.apache.pinot.common.request.Function; +import org.apache.pinot.common.utils.request.RequestUtils; +import org.apache.pinot.spi.data.Schema; +import org.apache.pinot.sql.FilterKind; + +import javax.annotation.Nullable; +import java.util.List; + +public abstract class BaseAndOrBooleanFilterOptimizer implements FilterOptimizer { + + protected static final Expression TRUE = RequestUtils.getLiteralExpression(true); + protected static final Expression FALSE = RequestUtils.getLiteralExpression(false); + + @Override + public abstract Expression optimize(Expression filterExpression, @Nullable Schema schema); + + /** + * If any of the operands of AND function is "false", then the AND function itself is false and can be replaced with + * "false" literal. Otherwise, remove all the "true" operands of the AND function. Similarly, if any of the operands + * of OR function is "true", then the OR function itself is true and can be replaced with "true" literal. Otherwise, + * remove all the "false" operands of the OR function. + */ + protected Expression optimizeCurrent(Expression expression) { + Function function = expression.getFunctionCall(); + String operator = function.getOperator(); + List operands = function.getOperands(); + if (operator.equals(FilterKind.AND.name())) { + // If any of the literal operands are FALSE, then replace AND function with FALSE. + for (Expression operand : operands) { + if (isAlwaysFalse(operand)) { + return FALSE; + } + } + + // Remove all Literal operands that are TRUE. + operands.removeIf(this::isAlwaysTrue); + if (operands.isEmpty()) { + return TRUE; + } + } else if (operator.equals(FilterKind.OR.name())) { + // If any of the literal operands are TRUE, then replace OR function with TRUE + for (Expression operand : operands) { + if (isAlwaysTrue(operand)) { + return TRUE; + } + } + + // Remove all Literal operands that are FALSE. + operands.removeIf(this::isAlwaysFalse); + if (operands.isEmpty()) { + return FALSE; + } + } else if (operator.equals(FilterKind.NOT.name())) { + assert operands.size() == 1; + Expression operand = operands.get(0); + if (isAlwaysTrue(operand)) { + return FALSE; + } + if (isAlwaysFalse(operand)) { + return TRUE; + } + } + return expression; + } + + protected boolean isAlwaysFalse(Expression operand) { + return operand.equals(FALSE); + } + + protected boolean isAlwaysTrue(Expression operand) { + return operand.equals(TRUE); + } +} diff --git a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/FlattenAndOrFilterOptimizer.java b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/FlattenAndOrFilterOptimizer.java index 1b04f472de99..a1de7bd7c6e8 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/FlattenAndOrFilterOptimizer.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/FlattenAndOrFilterOptimizer.java @@ -40,6 +40,9 @@ public Expression optimize(Expression filterExpression, @Nullable Schema schema) private Expression optimize(Expression filterExpression) { Function function = filterExpression.getFunctionCall(); + if (function == null) { + return filterExpression; + } String operator = function.getOperator(); if (!operator.equals(FilterKind.AND.name()) && !operator.equals(FilterKind.OR.name())) { return filterExpression; @@ -50,7 +53,7 @@ private Expression optimize(Expression filterExpression) { for (Expression child : children) { Expression optimizedChild = optimize(child); Function childFunction = optimizedChild.getFunctionCall(); - if (childFunction.getOperator().equals(operator)) { + if (childFunction != null && childFunction.getOperator().equals(operator)) { newChildren.addAll(childFunction.getOperands()); } else { newChildren.add(optimizedChild); 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 new file mode 100644 index 000000000000..fccf6f42eb30 --- /dev/null +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/IdenticalPredicateFilterOptimizer.java @@ -0,0 +1,98 @@ +package org.apache.pinot.core.query.optimizer.filter; + +import org.apache.pinot.common.request.Expression; +import org.apache.pinot.common.request.Function; +import org.apache.pinot.spi.data.Schema; +import org.apache.pinot.sql.FilterKind; + +import javax.annotation.Nullable; +import java.util.List; + +import static org.apache.pinot.sql.FilterKind.EQUALS; +import static org.apache.pinot.sql.FilterKind.NOT_EQUALS; + +public class IdenticalPredicateFilterOptimizer extends BaseAndOrBooleanFilterOptimizer { + + @Override + public Expression optimize(Expression filterExpression, @Nullable Schema schema) { + Function function = filterExpression.getFunctionCall(); + if (function == null) { + return filterExpression; + } + + List operands = function.getOperands(); + FilterKind kind = FilterKind.valueOf(function.getOperator()); + switch (kind) { + case AND: + case OR: + case NOT: + // Recursively traverse the expression tree to find an operator node that can be rewritten. + operands.forEach(operand -> optimize(operand, schema)); + + // We have rewritten the child operands, so rewrite the parent if needed. + return optimizeCurrent(filterExpression); + case EQUALS: + if (hasIdenticalLhsAndRhs(filterExpression)) { + return TRUE; + } + return filterExpression; + case NOT_EQUALS: + if (hasIdenticalLhsAndRhs(filterExpression)) { + return FALSE; + } + return filterExpression; + default: + return filterExpression; + } + } + + @Override + protected boolean isAlwaysFalse(Expression operand) { + if (super.isAlwaysFalse(operand)) { + return true; + } else if (hasIdenticalLhsAndRhs(operand)) { + return operand.getFunctionCall().getOperator().equals(NOT_EQUALS.name()); + } + return false; + } + + @Override + protected boolean isAlwaysTrue(Expression operand) { + if (super.isAlwaysTrue(operand)) { + return true; + } else if (hasIdenticalLhsAndRhs(operand)) { + return operand.getFunctionCall().getOperator().equals(EQUALS.name()); + } + return false; + } + + private boolean hasIdenticalLhsAndRhs(Expression operand) { + List children = operand.getFunctionCall().getOperands(); + boolean hasTwoChildren = children.size() == 2; + Expression firstChild = children.get(0); + if (firstChild.getFunctionCall() == null) { + return false; + } + boolean firstChildIsMinusOperator = firstChild.getFunctionCall().getOperator().equals("minus"); + boolean firstChildHasTwoOperands = firstChild.getFunctionCall().getOperandsSize() == 2; + Expression minusOperandFirstChild = firstChild.getFunctionCall().getOperands().get(0); + Expression minusOperandSecondChild = firstChild.getFunctionCall().getOperands().get(1); + boolean bothOperandsAreEqual = minusOperandFirstChild.equals(minusOperandSecondChild); + Expression secondChild = children.get(1); + boolean isSecondChildLiteralZero = isLiteralZero(secondChild); + + return hasTwoChildren && + firstChildIsMinusOperator && + firstChildHasTwoOperands && + bothOperandsAreEqual && + isSecondChildLiteralZero; + } + + private boolean isLiteralZero(Expression expression) { + if (!expression.isSetLiteral()) { + return false; + } + Object literalValue = expression.getLiteral().getFieldValue(); + return literalValue.equals(0) || literalValue.equals(0L) || literalValue.equals(0d); + } +} diff --git a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/MergeEqInFilterOptimizer.java b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/MergeEqInFilterOptimizer.java index 21fbf324bdeb..49a9ad171f2c 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/MergeEqInFilterOptimizer.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/MergeEqInFilterOptimizer.java @@ -56,6 +56,9 @@ public Expression optimize(Expression filterExpression, @Nullable Schema schema) private Expression optimize(Expression filterExpression) { Function function = filterExpression.getFunctionCall(); + if (function == null) { + return filterExpression; + } String operator = function.getOperator(); if (operator.equals(FilterKind.OR.name())) { List children = function.getOperands(); @@ -66,49 +69,53 @@ private Expression optimize(Expression filterExpression) { // Iterate over all the child filters to merge EQ and IN predicates for (Expression child : children) { Function childFunction = child.getFunctionCall(); - String childOperator = childFunction.getOperator(); - assert !childOperator.equals(FilterKind.OR.name()); - if (childOperator.equals(FilterKind.AND.name()) || childOperator.equals(FilterKind.NOT.name())) { - childFunction.getOperands().replaceAll(this::optimize); + if (childFunction == null) { newChildren.add(child); - } else if (childOperator.equals(FilterKind.EQUALS.name())) { - List operands = childFunction.getOperands(); - Expression lhs = operands.get(0); - Expression value = operands.get(1); - Set values = valuesMap.get(lhs); - if (values == null) { - values = new HashSet<>(); - values.add(value); - valuesMap.put(lhs, values); - } else { - values.add(value); - // Recreate filter when multiple predicates can be merged - recreateFilter = true; - } - } else if (childOperator.equals(FilterKind.IN.name())) { - List operands = childFunction.getOperands(); - Expression lhs = operands.get(0); - Set inPredicateValuesSet = new HashSet<>(); - int numOperands = operands.size(); - for (int i = 1; i < numOperands; i++) { - inPredicateValuesSet.add(operands.get(i)); - } - int numUniqueValues = inPredicateValuesSet.size(); - if (numUniqueValues == 1 || numUniqueValues != numOperands - 1) { - // Recreate filter when the IN predicate contains only 1 value (can be rewritten to EQ predicate), or values - // can be de-duplicated - recreateFilter = true; - } - Set values = valuesMap.get(lhs); - if (values == null) { - valuesMap.put(lhs, inPredicateValuesSet); + } else { + String childOperator = childFunction.getOperator(); + assert !childOperator.equals(FilterKind.OR.name()); + if (childOperator.equals(FilterKind.AND.name()) || childOperator.equals(FilterKind.NOT.name())) { + childFunction.getOperands().replaceAll(this::optimize); + newChildren.add(child); + } else if (childOperator.equals(FilterKind.EQUALS.name())) { + List operands = childFunction.getOperands(); + Expression lhs = operands.get(0); + Expression value = operands.get(1); + Set values = valuesMap.get(lhs); + if (values == null) { + values = new HashSet<>(); + values.add(value); + valuesMap.put(lhs, values); + } else { + values.add(value); + // Recreate filter when multiple predicates can be merged + recreateFilter = true; + } + } else if (childOperator.equals(FilterKind.IN.name())) { + List operands = childFunction.getOperands(); + Expression lhs = operands.get(0); + Set inPredicateValuesSet = new HashSet<>(); + int numOperands = operands.size(); + for (int i = 1; i < numOperands; i++) { + inPredicateValuesSet.add(operands.get(i)); + } + int numUniqueValues = inPredicateValuesSet.size(); + if (numUniqueValues == 1 || numUniqueValues != numOperands - 1) { + // Recreate filter when the IN predicate contains only 1 value (can be rewritten to EQ predicate), + // or values can be de-duplicated + recreateFilter = true; + } + Set values = valuesMap.get(lhs); + if (values == null) { + valuesMap.put(lhs, inPredicateValuesSet); + } else { + values.addAll(inPredicateValuesSet); + // Recreate filter when multiple predicates can be merged + recreateFilter = true; + } } else { - values.addAll(inPredicateValuesSet); - // Recreate filter when multiple predicates can be merged - recreateFilter = true; + newChildren.add(child); } - } else { - newChildren.add(child); } } 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 a50484889fa0..1bd0080897ce 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 @@ -62,9 +62,7 @@ * * TODO: Add support for BETWEEN, IN, and NOT IN operators. */ -public class NumericalFilterOptimizer implements FilterOptimizer { - private static final Expression TRUE = RequestUtils.getLiteralExpression(true); - private static final Expression FALSE = RequestUtils.getLiteralExpression(false); +public class NumericalFilterOptimizer extends BaseAndOrBooleanFilterOptimizer { @Override public Expression optimize(Expression expression, @Nullable Schema schema) { @@ -116,55 +114,6 @@ public Expression optimize(Expression expression, @Nullable Schema schema) { return expression; } - /** - * If any of the operands of AND function is "false", then the AND function itself is false and can be replaced with - * "false" literal. Otherwise, remove all the "true" operands of the AND function. Similarly, if any of the operands - * of OR function is "true", then the OR function itself is true and can be replaced with "true" literal. Otherwise, - * remove all the "false" operands of the OR function. - */ - private static Expression optimizeCurrent(Expression expression) { - Function function = expression.getFunctionCall(); - String operator = function.getOperator(); - List operands = function.getOperands(); - if (operator.equals(FilterKind.AND.name())) { - // If any of the literal operands are FALSE, then replace AND function with FALSE. - for (Expression operand : operands) { - if (operand.equals(FALSE)) { - return FALSE; - } - } - - // Remove all Literal operands that are TRUE. - operands.removeIf(x -> x.equals(TRUE)); - if (operands.isEmpty()) { - return TRUE; - } - } else if (operator.equals(FilterKind.OR.name())) { - // If any of the literal operands are TRUE, then replace OR function with TRUE - for (Expression operand : operands) { - if (operand.equals(TRUE)) { - return TRUE; - } - } - - // Remove all Literal operands that are FALSE. - operands.removeIf(x -> x.equals(FALSE)); - if (operands.isEmpty()) { - return FALSE; - } - } else if (operator.equals(FilterKind.NOT.name())) { - assert operands.size() == 1; - Expression operand = operands.get(0); - if (operand.equals(TRUE)) { - return FALSE; - } - if (operand.equals(FALSE)) { - return TRUE; - } - } - return expression; - } - /** * Rewrite expressions of form "column = literal" or "column != literal" to ensure that RHS literal is the same * datatype as LHS column. diff --git a/pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/QueryOptimizerTest.java b/pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/QueryOptimizerTest.java index 6054de965823..4ec5697efb8f 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/QueryOptimizerTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/QueryOptimizerTest.java @@ -225,9 +225,28 @@ public void testQueries() { testQuery( "SELECT * FROM testTable WHERE int >= 20 AND (int > 10 AND (int IN (1, 2) OR (int = 2 OR int = 3)) AND int <=" + " 30)", "SELECT * FROM testTable WHERE int BETWEEN 20 AND 30 AND int IN (1, 2, 3)"); + + // IdenticalPredicateOptimizer + testQuery("SELECT * FROM testTable WHERE 1=1", "SELECT * FROM testTable WHERE true"); + testQuery("SELECT * FROM testTable WHERE 1!=1", "SELECT * FROM testTable WHERE false"); + testQuery("SELECT * FROM testTable WHERE 1=1 AND 1!=1", "SELECT * FROM testTable WHERE false"); + testQuery("SELECT * FROM testTable WHERE 1=1 OR 1!=1", "SELECT * FROM testTable WHERE true"); + + testQuery("SELECT * FROM testTable WHERE \"a\"=\"a\"", "SELECT * FROM testTable WHERE true"); + testQuery("SELECT * FROM testTable WHERE \"a\"!=\"a\"", "SELECT * FROM testTable WHERE false"); + testQuery("SELECT * FROM testTable WHERE \"a\"=\"a\" AND \"a\"!=\"a\"", "SELECT * FROM testTable WHERE false"); + testQuery("SELECT * FROM testTable WHERE \"a\"=\"a\" OR \"a\"!=\"a\"", "SELECT * FROM testTable WHERE true"); + + testQuery("SELECT * FROM testTable WHERE 1=1 AND \"a\"=\"a\"", "SELECT * FROM testTable WHERE true"); + testQuery("SELECT * FROM testTable WHERE 1=1 OR \"a\"=\"a\"", "SELECT * FROM testTable WHERE true"); + testQuery("SELECT * FROM testTable WHERE 1!=1 AND \"a\"=\"a\"", "SELECT * FROM testTable WHERE false"); + testQuery("SELECT * FROM testTable WHERE 1=1 AND \"a\"!=\"a\"", "SELECT * FROM testTable WHERE false"); + testQuery("SELECT * FROM testTable WHERE 1!=1 OR \"a\"=\"a\"", "SELECT * FROM testTable WHERE true"); + testQuery("SELECT * FROM testTable WHERE 1=1 OR \"a\"!=\"a\"", "SELECT * FROM testTable WHERE true"); } private static void testQuery(String actual, String expected) { + assertNotEquals(actual, expected, "You must provide different queries to test"); PinotQuery actualPinotQuery = CalciteSqlParser.compileToPinotQuery(actual); OPTIMIZER.optimize(actualPinotQuery, SCHEMA); // Also optimize the expected query because the expected range can only be generate via optimizer @@ -245,30 +264,37 @@ private static void comparePinotQuery(PinotQuery actual, PinotQuery expected) { } private static void compareFilterExpression(Expression actual, Expression expected) { - Function actualFilterFunction = actual.getFunctionCall(); - Function expectedFilterFunction = expected.getFunctionCall(); - FilterKind actualFilterKind = FilterKind.valueOf(actualFilterFunction.getOperator()); - FilterKind expectedFilterKind = FilterKind.valueOf(expectedFilterFunction.getOperator()); - List actualOperands = actualFilterFunction.getOperands(); - List expectedOperands = expectedFilterFunction.getOperands(); - if (!actualFilterKind.isRange()) { - assertEquals(actualFilterKind, expectedFilterKind); - assertEquals(actualOperands.size(), expectedOperands.size()); - if (actualFilterKind == FilterKind.AND || actualFilterKind == FilterKind.OR) { - compareFilterExpressionChildren(actualOperands, expectedOperands); - } else { - assertEquals(actualOperands.get(0), expectedOperands.get(0)); - if (actualFilterKind == FilterKind.IN || actualFilterKind == FilterKind.NOT_IN) { - // Handle different order of values - assertEqualsNoOrder(actualOperands.toArray(), expectedOperands.toArray()); + if (actual.isSetLiteral()) { + assertNull(actual.getFunctionCall()); + assertNull(expected.getFunctionCall()); + assertTrue(expected.isSetLiteral()); + assertEquals(actual.getLiteral(), expected.getLiteral()); + } else { + Function actualFilterFunction = actual.getFunctionCall(); + Function expectedFilterFunction = expected.getFunctionCall(); + FilterKind actualFilterKind = FilterKind.valueOf(actualFilterFunction.getOperator()); + FilterKind expectedFilterKind = FilterKind.valueOf(expectedFilterFunction.getOperator()); + List actualOperands = actualFilterFunction.getOperands(); + List expectedOperands = expectedFilterFunction.getOperands(); + if (!actualFilterKind.isRange()) { + assertEquals(actualFilterKind, expectedFilterKind); + assertEquals(actualOperands.size(), expectedOperands.size()); + if (actualFilterKind == FilterKind.AND || actualFilterKind == FilterKind.OR) { + compareFilterExpressionChildren(actualOperands, expectedOperands); } else { - assertEquals(actualOperands, expectedOperands); + assertEquals(actualOperands.get(0), expectedOperands.get(0)); + if (actualFilterKind == FilterKind.IN || actualFilterKind == FilterKind.NOT_IN) { + // Handle different order of values + assertEqualsNoOrder(actualOperands.toArray(), expectedOperands.toArray()); + } else { + assertEquals(actualOperands, expectedOperands); + } } + } else { + assertTrue(expectedFilterKind.isRange()); + assertEquals(getRangeString(actualFilterKind, actualOperands), + getRangeString(expectedFilterKind, expectedOperands)); } - } else { - assertTrue(expectedFilterKind.isRange()); - assertEquals(getRangeString(actualFilterKind, actualOperands), - getRangeString(expectedFilterKind, expectedOperands)); } } From dc932e4afef68606539c375c3f2ce3cb2931a00a Mon Sep 17 00:00:00 2001 From: Johan Adami Date: Sun, 19 Mar 2023 15:29:10 -0400 Subject: [PATCH 2/8] add comments --- .../filter/BaseAndOrBooleanFilterOptimizer.java | 13 +++++++++---- .../filter/IdenticalPredicateFilterOptimizer.java | 10 ++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) 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 643f2c01ef5a..2e6a2ab95e19 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 @@ -9,6 +9,11 @@ import javax.annotation.Nullable; import java.util.List; +/** + * This base class acts as a helper for any optimizer that is effectively removing filter conditions. + * It provides TRUE/FALSE literal classes that can be used to replace filter expressions that are always true/false. + * It provides an optimization implementation for AND/OR/NOT expressions. + */ public abstract class BaseAndOrBooleanFilterOptimizer implements FilterOptimizer { protected static final Expression TRUE = RequestUtils.getLiteralExpression(true); @@ -28,27 +33,27 @@ protected Expression optimizeCurrent(Expression expression) { String operator = function.getOperator(); List operands = function.getOperands(); if (operator.equals(FilterKind.AND.name())) { - // If any of the literal operands are FALSE, then replace AND function with FALSE. + // If any of the literal operands are always false, then replace AND function with FALSE. for (Expression operand : operands) { if (isAlwaysFalse(operand)) { return FALSE; } } - // Remove all Literal operands that are TRUE. + // Remove all Literal operands that are always true. operands.removeIf(this::isAlwaysTrue); if (operands.isEmpty()) { return TRUE; } } else if (operator.equals(FilterKind.OR.name())) { - // If any of the literal operands are TRUE, then replace OR function with TRUE + // If any of the literal operands are always true, then replace OR function with TRUE for (Expression operand : operands) { if (isAlwaysTrue(operand)) { return TRUE; } } - // Remove all Literal operands that are FALSE. + // Remove all Literal operands that are always false. operands.removeIf(this::isAlwaysFalse); if (operands.isEmpty()) { return 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 fccf6f42eb30..a23ad76a4b61 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 @@ -11,6 +11,11 @@ import static org.apache.pinot.sql.FilterKind.EQUALS; import static org.apache.pinot.sql.FilterKind.NOT_EQUALS; +/** + * This optimizer converts all predicates where the left hand side == right hand side to + * a simple TRUE/FALSE literal value. While filters like, WHERE 1=1 OR "col1"="col1" are not + * typical, they end up expensive in Pinot because they are rewritten as A-A==0. + */ public class IdenticalPredicateFilterOptimizer extends BaseAndOrBooleanFilterOptimizer { @Override @@ -66,6 +71,11 @@ protected boolean isAlwaysTrue(Expression operand) { return false; } + /** + * Pinot queries of the WHERE 1 != 1 AND "col1" = "col2" variety are rewritten as + * 1-1 != 0 AND "col1"-"col2" = 0. Therefore, we check specifically for the case where + * the operand is set up in this fashion. + */ private boolean hasIdenticalLhsAndRhs(Expression operand) { List children = operand.getFunctionCall().getOperands(); boolean hasTwoChildren = children.size() == 2; From 7ca69728aa255041e62faab5b43d40b011892236 Mon Sep 17 00:00:00 2001 From: Johan Adami Date: Sun, 19 Mar 2023 17:28:44 -0400 Subject: [PATCH 3/8] lint fixes --- .../core/query/optimizer/QueryOptimizer.java | 8 +- .../BaseAndOrBooleanFilterOptimizer.java | 18 ++ .../IdenticalPredicateFilterOptimizer.java | 167 ++++++++++-------- .../filter/NumericalFilterOptimizer.java | 1 - .../query/optimizer/QueryOptimizerTest.java | 4 + 5 files changed, 119 insertions(+), 79 deletions(-) diff --git a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java index 695179d68be1..dd7f80f9d045 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java @@ -24,7 +24,13 @@ import javax.annotation.Nullable; import org.apache.pinot.common.request.Expression; import org.apache.pinot.common.request.PinotQuery; -import org.apache.pinot.core.query.optimizer.filter.*; +import org.apache.pinot.core.query.optimizer.filter.FilterOptimizer; +import org.apache.pinot.core.query.optimizer.filter.FlattenAndOrFilterOptimizer; +import org.apache.pinot.core.query.optimizer.filter.IdenticalPredicateFilterOptimizer; +import org.apache.pinot.core.query.optimizer.filter.MergeEqInFilterOptimizer; +import org.apache.pinot.core.query.optimizer.filter.MergeRangeFilterOptimizer; +import org.apache.pinot.core.query.optimizer.filter.NumericalFilterOptimizer; +import org.apache.pinot.core.query.optimizer.filter.TimePredicateFilterOptimizer; import org.apache.pinot.core.query.optimizer.statement.StatementOptimizer; import org.apache.pinot.core.query.optimizer.statement.StringPredicateFilterOptimizer; import org.apache.pinot.spi.config.table.TableConfig; 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 2e6a2ab95e19..151b6e3cdb48 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 @@ -1,3 +1,21 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ package org.apache.pinot.core.query.optimizer.filter; import org.apache.pinot.common.request.Expression; 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 a23ad76a4b61..7c4b817d8256 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 @@ -1,3 +1,21 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ package org.apache.pinot.core.query.optimizer.filter; import org.apache.pinot.common.request.Expression; @@ -8,8 +26,6 @@ import javax.annotation.Nullable; import java.util.List; -import static org.apache.pinot.sql.FilterKind.EQUALS; -import static org.apache.pinot.sql.FilterKind.NOT_EQUALS; /** * This optimizer converts all predicates where the left hand side == right hand side to @@ -18,91 +34,88 @@ */ public class IdenticalPredicateFilterOptimizer extends BaseAndOrBooleanFilterOptimizer { - @Override - public Expression optimize(Expression filterExpression, @Nullable Schema schema) { - Function function = filterExpression.getFunctionCall(); - if (function == null) { - return filterExpression; - } + @Override + public Expression optimize(Expression filterExpression, @Nullable Schema schema) { + Function function = filterExpression.getFunctionCall(); + if (function == null) { + return filterExpression; + } - List operands = function.getOperands(); - FilterKind kind = FilterKind.valueOf(function.getOperator()); - switch (kind) { - case AND: - case OR: - case NOT: - // Recursively traverse the expression tree to find an operator node that can be rewritten. - operands.forEach(operand -> optimize(operand, schema)); + List operands = function.getOperands(); + FilterKind kind = FilterKind.valueOf(function.getOperator()); + switch (kind) { + case AND: + case OR: + case NOT: + // Recursively traverse the expression tree to find an operator node that can be rewritten. + operands.forEach(operand -> optimize(operand, schema)); - // We have rewritten the child operands, so rewrite the parent if needed. - return optimizeCurrent(filterExpression); - case EQUALS: - if (hasIdenticalLhsAndRhs(filterExpression)) { - return TRUE; - } - return filterExpression; - case NOT_EQUALS: - if (hasIdenticalLhsAndRhs(filterExpression)) { - return FALSE; - } - return filterExpression; - default: - return filterExpression; + // We have rewritten the child operands, so rewrite the parent if needed. + return optimizeCurrent(filterExpression); + case EQUALS: + if (hasIdenticalLhsAndRhs(filterExpression)) { + return TRUE; } - } - - @Override - protected boolean isAlwaysFalse(Expression operand) { - if (super.isAlwaysFalse(operand)) { - return true; - } else if (hasIdenticalLhsAndRhs(operand)) { - return operand.getFunctionCall().getOperator().equals(NOT_EQUALS.name()); + return filterExpression; + case NOT_EQUALS: + if (hasIdenticalLhsAndRhs(filterExpression)) { + return FALSE; } - return false; + return filterExpression; + default: + return filterExpression; } + } - @Override - protected boolean isAlwaysTrue(Expression operand) { - if (super.isAlwaysTrue(operand)) { - return true; - } else if (hasIdenticalLhsAndRhs(operand)) { - return operand.getFunctionCall().getOperator().equals(EQUALS.name()); - } - return false; + @Override + protected boolean isAlwaysFalse(Expression operand) { + if (super.isAlwaysFalse(operand)) { + return true; + } else if (hasIdenticalLhsAndRhs(operand)) { + return operand.getFunctionCall().getOperator().equals(FilterKind.NOT_EQUALS.name()); } + return false; + } - /** - * Pinot queries of the WHERE 1 != 1 AND "col1" = "col2" variety are rewritten as - * 1-1 != 0 AND "col1"-"col2" = 0. Therefore, we check specifically for the case where - * the operand is set up in this fashion. - */ - private boolean hasIdenticalLhsAndRhs(Expression operand) { - List children = operand.getFunctionCall().getOperands(); - boolean hasTwoChildren = children.size() == 2; - Expression firstChild = children.get(0); - if (firstChild.getFunctionCall() == null) { - return false; - } - boolean firstChildIsMinusOperator = firstChild.getFunctionCall().getOperator().equals("minus"); - boolean firstChildHasTwoOperands = firstChild.getFunctionCall().getOperandsSize() == 2; - Expression minusOperandFirstChild = firstChild.getFunctionCall().getOperands().get(0); - Expression minusOperandSecondChild = firstChild.getFunctionCall().getOperands().get(1); - boolean bothOperandsAreEqual = minusOperandFirstChild.equals(minusOperandSecondChild); - Expression secondChild = children.get(1); - boolean isSecondChildLiteralZero = isLiteralZero(secondChild); + @Override + protected boolean isAlwaysTrue(Expression operand) { + if (super.isAlwaysTrue(operand)) { + return true; + } else if (hasIdenticalLhsAndRhs(operand)) { + return operand.getFunctionCall().getOperator().equals(FilterKind.EQUALS.name()); + } + return false; + } - return hasTwoChildren && - firstChildIsMinusOperator && - firstChildHasTwoOperands && - bothOperandsAreEqual && - isSecondChildLiteralZero; + /** + * Pinot queries of the WHERE 1 != 1 AND "col1" = "col2" variety are rewritten as + * 1-1 != 0 AND "col1"-"col2" = 0. Therefore, we check specifically for the case where + * the operand is set up in this fashion. + */ + private boolean hasIdenticalLhsAndRhs(Expression operand) { + List children = operand.getFunctionCall().getOperands(); + boolean hasTwoChildren = children.size() == 2; + Expression firstChild = children.get(0); + if (firstChild.getFunctionCall() == null) { + return false; } + boolean firstChildIsMinusOperator = firstChild.getFunctionCall().getOperator().equals("minus"); + boolean firstChildHasTwoOperands = firstChild.getFunctionCall().getOperandsSize() == 2; + Expression minusOperandFirstChild = firstChild.getFunctionCall().getOperands().get(0); + Expression minusOperandSecondChild = firstChild.getFunctionCall().getOperands().get(1); + boolean bothOperandsAreEqual = minusOperandFirstChild.equals(minusOperandSecondChild); + Expression secondChild = children.get(1); + boolean isSecondChildLiteralZero = isLiteralZero(secondChild); - private boolean isLiteralZero(Expression expression) { - if (!expression.isSetLiteral()) { - return false; - } - Object literalValue = expression.getLiteral().getFieldValue(); - return literalValue.equals(0) || literalValue.equals(0L) || literalValue.equals(0d); + return hasTwoChildren && firstChildIsMinusOperator && firstChildHasTwoOperands && bothOperandsAreEqual + && isSecondChildLiteralZero; + } + + private boolean isLiteralZero(Expression expression) { + if (!expression.isSetLiteral()) { + return false; } + Object literalValue = expression.getLiteral().getFieldValue(); + return literalValue.equals(0) || literalValue.equals(0L) || literalValue.equals(0d); + } } 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 1bd0080897ce..60112d6b45d6 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 @@ -25,7 +25,6 @@ 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.FieldSpec; import org.apache.pinot.spi.data.Schema; import org.apache.pinot.sql.FilterKind; diff --git a/pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/QueryOptimizerTest.java b/pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/QueryOptimizerTest.java index 4ec5697efb8f..2a4f2921cd2e 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/QueryOptimizerTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/QueryOptimizerTest.java @@ -243,6 +243,10 @@ public void testQueries() { testQuery("SELECT * FROM testTable WHERE 1=1 AND \"a\"!=\"a\"", "SELECT * FROM testTable WHERE false"); testQuery("SELECT * FROM testTable WHERE 1!=1 OR \"a\"=\"a\"", "SELECT * FROM testTable WHERE true"); testQuery("SELECT * FROM testTable WHERE 1=1 OR \"a\"!=\"a\"", "SELECT * FROM testTable WHERE true"); + + testQuery("SELECT * FROM testTable WHERE 1.0=1.0", "SELECT * FROM testTable WHERE true"); + testQuery("SELECT * FROM testTable WHERE 1.0=1", "SELECT * FROM testTable WHERE true"); + testQuery("SELECT * FROM testTable WHERE 1.01=1", "SELECT * FROM testTable WHERE false"); } private static void testQuery(String actual, String expected) { From 7d3c85c3b5abd15078a57218ba9316e3868a2613 Mon Sep 17 00:00:00 2001 From: Johan Adami Date: Sun, 19 Mar 2023 21:09:28 -0400 Subject: [PATCH 4/8] use the right license format --- .../filter/BaseAndOrBooleanFilterOptimizer.java | 5 ++--- .../filter/IdenticalPredicateFilterOptimizer.java | 11 +++++------ 2 files changed, 7 insertions(+), 9 deletions(-) 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 151b6e3cdb48..8fccfd85f793 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 @@ -18,15 +18,14 @@ */ package org.apache.pinot.core.query.optimizer.filter; +import java.util.List; +import javax.annotation.Nullable; import org.apache.pinot.common.request.Expression; import org.apache.pinot.common.request.Function; import org.apache.pinot.common.utils.request.RequestUtils; import org.apache.pinot.spi.data.Schema; import org.apache.pinot.sql.FilterKind; -import javax.annotation.Nullable; -import java.util.List; - /** * This base class acts as a helper for any optimizer that is effectively removing filter conditions. * It provides TRUE/FALSE literal classes that can be used to replace filter expressions that are always 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 7c4b817d8256..841a40231f81 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 @@ -6,9 +6,9 @@ * to you under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at - *

- * http://www.apache.org/licenses/LICENSE-2.0 - *

+ * + * http://www.apache.org/licenses/LICENSE-2.0 + * * Unless required by applicable law or agreed to in writing, * software distributed under the License is distributed on an * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY @@ -18,14 +18,13 @@ */ package org.apache.pinot.core.query.optimizer.filter; +import java.util.List; +import javax.annotation.Nullable; import org.apache.pinot.common.request.Expression; import org.apache.pinot.common.request.Function; import org.apache.pinot.spi.data.Schema; import org.apache.pinot.sql.FilterKind; -import javax.annotation.Nullable; -import java.util.List; - /** * This optimizer converts all predicates where the left hand side == right hand side to From c7c578fb25a392e0846951373c8534646190d6ca Mon Sep 17 00:00:00 2001 From: Johan Adami Date: Sun, 19 Mar 2023 22:23:47 -0400 Subject: [PATCH 5/8] better null protection; leave suggestion for query optimization --- .../recommender/io/InputManager.java | 2 ++ .../IdenticalPredicateFilterOptimizer.java | 22 ++++++++++++++----- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/recommender/io/InputManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/recommender/io/InputManager.java index 4436fc356c95..28dd14f88a40 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/recommender/io/InputManager.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/recommender/io/InputManager.java @@ -170,6 +170,8 @@ private void validateQueries() { for (String queryString : _queryWeightMap.keySet()) { try { PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(queryString); + // TODO: should we catch and ignore any errors here. If we error on query optimization, + // we probably shouldn't fail the query as well. _queryOptimizer.optimize(pinotQuery, _schema); QueryContext queryContext = QueryContextConverterUtils.getQueryContext(pinotQuery); 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 841a40231f81..2eeb14205fe5 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 @@ -90,24 +90,34 @@ protected boolean isAlwaysTrue(Expression operand) { * Pinot queries of the WHERE 1 != 1 AND "col1" = "col2" variety are rewritten as * 1-1 != 0 AND "col1"-"col2" = 0. Therefore, we check specifically for the case where * the operand is set up in this fashion. + * + * 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. */ private boolean hasIdenticalLhsAndRhs(Expression operand) { List children = operand.getFunctionCall().getOperands(); boolean hasTwoChildren = children.size() == 2; Expression firstChild = children.get(0); - if (firstChild.getFunctionCall() == null) { + if (firstChild.getFunctionCall() == null || !hasTwoChildren) { return false; } boolean firstChildIsMinusOperator = firstChild.getFunctionCall().getOperator().equals("minus"); + if (!firstChildIsMinusOperator) { + return false; + } boolean firstChildHasTwoOperands = firstChild.getFunctionCall().getOperandsSize() == 2; + if (!firstChildHasTwoOperands) { + return false; + } Expression minusOperandFirstChild = firstChild.getFunctionCall().getOperands().get(0); Expression minusOperandSecondChild = firstChild.getFunctionCall().getOperands().get(1); - boolean bothOperandsAreEqual = minusOperandFirstChild.equals(minusOperandSecondChild); + if (minusOperandFirstChild == null || minusOperandSecondChild == null || !minusOperandFirstChild.equals( + minusOperandSecondChild)) { + return false; + } Expression secondChild = children.get(1); - boolean isSecondChildLiteralZero = isLiteralZero(secondChild); - - return hasTwoChildren && firstChildIsMinusOperator && firstChildHasTwoOperands && bothOperandsAreEqual - && isSecondChildLiteralZero; + return isLiteralZero(secondChild); } private boolean isLiteralZero(Expression expression) { From c432e13beaa9b9406f5b2072691632bb2bb19f46 Mon Sep 17 00:00:00 2001 From: Johan Adami Date: Wed, 22 Mar 2023 19:09:11 -0400 Subject: [PATCH 6/8] clean up the interface for BaseAndOrBooleanFilterOptimizer; respond to other PR comments --- .../recommender/io/InputManager.java | 4 +- .../core/query/optimizer/QueryOptimizer.java | 5 +- .../BaseAndOrBooleanFilterOptimizer.java | 151 +++++++++++------- .../IdenticalPredicateFilterOptimizer.java | 58 +++---- .../filter/NumericalFilterOptimizer.java | 47 ++---- .../query/optimizer/QueryOptimizerTest.java | 3 + 6 files changed, 139 insertions(+), 129 deletions(-) diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/recommender/io/InputManager.java b/pinot-controller/src/main/java/org/apache/pinot/controller/recommender/io/InputManager.java index 28dd14f88a40..4834866c518b 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/recommender/io/InputManager.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/recommender/io/InputManager.java @@ -170,8 +170,8 @@ private void validateQueries() { for (String queryString : _queryWeightMap.keySet()) { try { PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(queryString); - // TODO: should we catch and ignore any errors here. If we error on query optimization, - // we probably shouldn't fail the query as well. + // TODO: we should catch and log errors here so we don't fail queries on optimization. + // For now, because this modifies the query in place, we let the error propagate. _queryOptimizer.optimize(pinotQuery, _schema); QueryContext queryContext = QueryContextConverterUtils.getQueryContext(pinotQuery); diff --git a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java index dd7f80f9d045..0b98cdb88ff6 100644 --- a/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java +++ b/pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java @@ -44,8 +44,9 @@ public class QueryOptimizer { // - TimePredicateFilterOptimizer and MergeRangeFilterOptimizer relies on NumericalFilterOptimizer to convert the // values to the proper format so that they can be properly parsed private static final List FILTER_OPTIMIZERS = - Arrays.asList(new FlattenAndOrFilterOptimizer(), new MergeEqInFilterOptimizer(), new NumericalFilterOptimizer(), - new TimePredicateFilterOptimizer(), new MergeRangeFilterOptimizer(), new IdenticalPredicateFilterOptimizer()); + Arrays.asList(new FlattenAndOrFilterOptimizer(), new IdenticalPredicateFilterOptimizer(), + new MergeEqInFilterOptimizer(), new NumericalFilterOptimizer(), new TimePredicateFilterOptimizer(), + new MergeRangeFilterOptimizer()); private static final List STATEMENT_OPTIMIZERS = Collections.singletonList(new StringPredicateFilterOptimizer()); 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 8fccfd85f793..e372ff100c78 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,11 +21,14 @@ 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; + /** * This base class acts as a helper for any optimizer that is effectively removing filter conditions. * It provides TRUE/FALSE literal classes that can be used to replace filter expressions that are always true/false. @@ -33,66 +36,106 @@ */ public abstract class BaseAndOrBooleanFilterOptimizer implements FilterOptimizer { - protected static final Expression TRUE = RequestUtils.getLiteralExpression(true); - protected static final Expression FALSE = RequestUtils.getLiteralExpression(false); + protected static final Expression TRUE = RequestUtils.getLiteralExpression(true); + protected static final Expression FALSE = RequestUtils.getLiteralExpression(false); + + /** + * This recursively optimizes each part of the filter expression. For any AND/OR/NOT, + * we optimize each child, then we optimize the remaining statement. If there is only + * a child statement, we optimize that. + */ + @Override + public Expression optimize(Expression filterExpression, @Nullable Schema schema) { + if (!canBeOptimized(filterExpression, schema)) { + return filterExpression; + } + + Function function = filterExpression.getFunctionCall(); + List operands = function.getOperands(); + FilterKind kind = FilterKind.valueOf(function.getOperator()); + switch (kind) { + case AND: + case OR: + case NOT: + // Recursively traverse the expression tree to find an operator node that can be rewritten. + operands.forEach(operand -> optimize(operand, schema)); - @Override - public abstract Expression optimize(Expression filterExpression, @Nullable Schema schema); + // We have rewritten the child operands, so rewrite the parent if needed. + return optimizeCurrent(filterExpression); + default: + return optimizeChild(filterExpression, schema); + } + } - /** - * If any of the operands of AND function is "false", then the AND function itself is false and can be replaced with - * "false" literal. Otherwise, remove all the "true" operands of the AND function. Similarly, if any of the operands - * of OR function is "true", then the OR function itself is true and can be replaced with "true" literal. Otherwise, - * remove all the "false" operands of the OR function. - */ - protected Expression optimizeCurrent(Expression expression) { - Function function = expression.getFunctionCall(); - String operator = function.getOperator(); - List operands = function.getOperands(); - 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)) { - return FALSE; - } - } + abstract boolean canBeOptimized(Expression filterExpression, @Nullable Schema schema); - // Remove all Literal operands that are always true. - operands.removeIf(this::isAlwaysTrue); - 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)) { - return TRUE; - } - } + /** + * Optimize any cases that are not AND/OR/NOT. This should be done by converting any cases + * that are always true to TRUE or always false to FALSE. + */ + abstract Expression optimizeChild(Expression filterExpression, @Nullable Schema schema); - // Remove all Literal operands that are always false. - operands.removeIf(this::isAlwaysFalse); - if (operands.isEmpty()) { - return FALSE; - } - } else if (operator.equals(FilterKind.NOT.name())) { - assert operands.size() == 1; - Expression operand = operands.get(0); - if (isAlwaysTrue(operand)) { - return FALSE; - } - if (isAlwaysFalse(operand)) { - return TRUE; - } + /** + * If any of the operands of AND function is "false", then the AND function itself is false and can be replaced with + * "false" literal. Otherwise, remove all the "true" operands of the AND function. Similarly, if any of the operands + * of OR function is "true", then the OR function itself is true and can be replaced with "true" literal. Otherwise, + * remove all the "false" operands of the OR function. + */ + protected Expression optimizeCurrent(Expression expression) { + Function function = expression.getFunctionCall(); + String operator = function.getOperator(); + List operands = function.getOperands(); + 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)) { + return FALSE; } - return expression; - } + } - protected boolean isAlwaysFalse(Expression operand) { - return operand.equals(FALSE); - } + // Remove all Literal operands that are always true. + operands.removeIf(this::isAlwaysTrue); + 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)) { + return TRUE; + } + } - protected boolean isAlwaysTrue(Expression operand) { - return operand.equals(TRUE); + // Remove all Literal operands that are always false. + operands.removeIf(this::isAlwaysFalse); + if (operands.isEmpty()) { + return FALSE; + } + } else if (operator.equals(FilterKind.NOT.name())) { + assert operands.size() == 1; + Expression operand = operands.get(0); + if (isAlwaysTrue(operand)) { + return FALSE; + } + if (isAlwaysFalse(operand)) { + 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)); + } } 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 2eeb14205fe5..b8e44fa2ead5 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 @@ -34,56 +34,30 @@ public class IdenticalPredicateFilterOptimizer extends BaseAndOrBooleanFilterOptimizer { @Override - public Expression optimize(Expression filterExpression, @Nullable Schema schema) { - Function function = filterExpression.getFunctionCall(); - if (function == null) { - return filterExpression; - } + boolean canBeOptimized(Expression filterExpression, @Nullable Schema schema) { + // if there's no function call, there's no lhs or rhs + return filterExpression.getFunctionCall() != null; + } - List operands = function.getOperands(); + @Override + Expression optimizeChild(Expression filterExpression, @Nullable Schema schema) { + Function function = filterExpression.getFunctionCall(); FilterKind kind = FilterKind.valueOf(function.getOperator()); switch (kind) { - case AND: - case OR: - case NOT: - // Recursively traverse the expression tree to find an operator node that can be rewritten. - operands.forEach(operand -> optimize(operand, schema)); - - // We have rewritten the child operands, so rewrite the parent if needed. - return optimizeCurrent(filterExpression); case EQUALS: if (hasIdenticalLhsAndRhs(filterExpression)) { - return TRUE; + setExpressionToBoolean(filterExpression, true); } - return filterExpression; + break; case NOT_EQUALS: if (hasIdenticalLhsAndRhs(filterExpression)) { - return FALSE; + setExpressionToBoolean(filterExpression, false); } - return filterExpression; + break; default: - return filterExpression; - } - } - - @Override - protected boolean isAlwaysFalse(Expression operand) { - if (super.isAlwaysFalse(operand)) { - return true; - } else if (hasIdenticalLhsAndRhs(operand)) { - return operand.getFunctionCall().getOperator().equals(FilterKind.NOT_EQUALS.name()); + break; } - return false; - } - - @Override - protected boolean isAlwaysTrue(Expression operand) { - if (super.isAlwaysTrue(operand)) { - return true; - } else if (hasIdenticalLhsAndRhs(operand)) { - return operand.getFunctionCall().getOperator().equals(FilterKind.EQUALS.name()); - } - return false; + return filterExpression; } /** @@ -96,7 +70,11 @@ protected boolean isAlwaysTrue(Expression operand) { * to return null and fail the query. */ private boolean hasIdenticalLhsAndRhs(Expression operand) { - List children = operand.getFunctionCall().getOperands(); + Function function = operand.getFunctionCall(); + if (function == null) { + return false; + } + List children = function.getOperands(); boolean hasTwoChildren = children.size() == 2; Expression firstChild = children.get(0); if (firstChild.getFunctionCall() == null || !hasTwoChildren) { 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 60112d6b45d6..93e17ae17e0e 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 @@ -30,7 +30,6 @@ import org.apache.pinot.sql.FilterKind; - /** * Numerical expressions of form "column literal", where operator can be '=', '!=', '>', '>=', '<', or '<=', * can compare a column of one datatype (say INT) with a literal of different datatype (say DOUBLE). These expressions @@ -64,30 +63,23 @@ public class NumericalFilterOptimizer extends BaseAndOrBooleanFilterOptimizer { @Override - public Expression optimize(Expression expression, @Nullable Schema schema) { - ExpressionType type = expression.getType(); - if (type != ExpressionType.FUNCTION || schema == null) { - // We have nothing to rewrite if expression is not a function or schema is null - return expression; - } + boolean canBeOptimized(Expression filterExpression, @Nullable Schema schema) { + ExpressionType type = filterExpression.getType(); + // We have nothing to rewrite if expression is not a function or schema is null + return type == ExpressionType.FUNCTION && schema != null; + } - Function function = expression.getFunctionCall(); - List operands = function.getOperands(); + @Override + Expression optimizeChild(Expression filterExpression, @Nullable Schema schema) { + Function function = filterExpression.getFunctionCall(); FilterKind kind = FilterKind.valueOf(function.getOperator()); switch (kind) { - case AND: - case OR: - case NOT: - // Recursively traverse the expression tree to find an operator node that can be rewritten. - operands.forEach(operand -> optimize(operand, schema)); - - // We have rewritten the child operands, so rewrite the parent if needed. - return optimizeCurrent(expression); case IS_NULL: case IS_NOT_NULL: // No need to try to optimize IS_NULL and IS_NOT_NULL operations on numerical columns. break; default: + List operands = function.getOperands(); // Verify that LHS is a numeric column and RHS is a numeric literal before rewriting. Expression lhs = operands.get(0); Expression rhs = operands.get(1); @@ -97,12 +89,12 @@ public Expression optimize(Expression expression, @Nullable Schema schema) { switch (kind) { case EQUALS: case NOT_EQUALS: - return rewriteEqualsExpression(expression, kind, dataType, rhs); + return rewriteEqualsExpression(filterExpression, kind, dataType, rhs); case GREATER_THAN: case GREATER_THAN_OR_EQUAL: case LESS_THAN: case LESS_THAN_OR_EQUAL: - return rewriteRangeExpression(expression, kind, lhs, rhs, schema); + return rewriteRangeExpression(filterExpression, kind, lhs, rhs, schema); default: break; } @@ -110,7 +102,7 @@ public Expression optimize(Expression expression, @Nullable Schema schema) { } break; } - return expression; + return filterExpression; } /** @@ -406,11 +398,11 @@ private static FieldSpec.DataType getDataType(Expression expression, Schema sche if (fieldSpec != null && fieldSpec.isSingleValueField()) { return fieldSpec.getDataType(); } - } else if (expression.getType() == ExpressionType.FUNCTION - && "cast".equalsIgnoreCase(expression.getFunctionCall().getOperator())) { + } else if (expression.getType() == ExpressionType.FUNCTION && "cast".equalsIgnoreCase( + expression.getFunctionCall().getOperator())) { // expression is not identifier but we can also determine the data type. - String targetTypeLiteral = expression.getFunctionCall().getOperands().get(1).getLiteral().getStringValue() - .toUpperCase(); + String targetTypeLiteral = + expression.getFunctionCall().getOperands().get(1).getLiteral().getStringValue().toUpperCase(); FieldSpec.DataType dataType; if ("INTEGER".equals(targetTypeLiteral)) { dataType = FieldSpec.DataType.INT; @@ -440,11 +432,4 @@ private static boolean isNumericLiteral(Expression expression) { } return false; } - - /** Change the expression value to boolean literal with given value. */ - private static void setExpressionToBoolean(Expression expression, boolean value) { - expression.unsetFunctionCall(); - expression.setType(ExpressionType.LITERAL); - expression.setLiteral(Literal.boolValue(value)); - } } diff --git a/pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/QueryOptimizerTest.java b/pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/QueryOptimizerTest.java index 2a4f2921cd2e..631b4ee9d2ef 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/QueryOptimizerTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/query/optimizer/QueryOptimizerTest.java @@ -247,6 +247,9 @@ public void testQueries() { testQuery("SELECT * FROM testTable WHERE 1.0=1.0", "SELECT * FROM testTable WHERE true"); testQuery("SELECT * FROM testTable WHERE 1.0=1", "SELECT * FROM testTable WHERE true"); testQuery("SELECT * FROM testTable WHERE 1.01=1", "SELECT * FROM testTable WHERE false"); + + testQuery("SELECT * FROM testTable WHERE 1=1 AND true", "SELECT * FROM testTable WHERE true"); + testQuery("SELECT * FROM testTable WHERE \"a\"=\"a\" AND true", "SELECT * FROM testTable WHERE true"); } private static void testQuery(String actual, String expected) { From bacf64e09330892a7b5418ff7eb12d1fb7d2c65c Mon Sep 17 00:00:00 2001 From: Johan Adami Date: Mon, 27 Mar 2023 23:20:12 -0400 Subject: [PATCH 7/8] more cleanup; less mutation --- .../BaseAndOrBooleanFilterOptimizer.java | 30 ++++++------------- .../IdenticalPredicateFilterOptimizer.java | 24 +++++++-------- .../filter/NumericalFilterOptimizer.java | 29 +++++++++--------- 3 files changed, 34 insertions(+), 49 deletions(-) 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); From ffd0f020ba2a1676dd9c54d6c94cc00ef8fbe743 Mon Sep 17 00:00:00 2001 From: Johan Adami Date: Mon, 27 Mar 2023 23:56:05 -0400 Subject: [PATCH 8/8] lint fix --- .../core/query/optimizer/filter/NumericalFilterOptimizer.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 80c723f25805..5a8baeb3faca 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 @@ -239,7 +239,8 @@ private static Expression rewriteRangeExpression(Expression range, FilterKind ki // 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. - return getExpressionFromBoolean(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.