Jackie-Jiang commented on code in PR #10444:
URL: https://github.com/apache/pinot/pull/10444#discussion_r1144217959
##########
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) {
Review Comment:
Is this added to handle the `WHERE true` clause? I assume current code will
cause NPE?
##########
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,
Review Comment:
Good point. Ignoring the error is more robust, while failing the query can
help catch the bug in the optimizer and prevent certain unexpected performance
degradation. Currently optimize logic is applied in-place (there is no return
value), so I personally prefer directly failing the query since the query might
already be modified and messed up
##########
pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/BaseAndOrBooleanFilterOptimizer.java:
##########
@@ -0,0 +1,98 @@
+/**
+ * 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 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;
+
+/**
+ * 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);
+ protected static final Expression FALSE =
RequestUtils.getLiteralExpression(false);
+
+ @Override
+ public abstract Expression optimize(Expression filterExpression, @Nullable
Schema schema);
Review Comment:
(minor) No need to override this API to an abstract method
##########
pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/BaseAndOrBooleanFilterOptimizer.java:
##########
@@ -0,0 +1,98 @@
+/**
+ * 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 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;
+
+/**
+ * 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);
Review Comment:
This file doesn't follow the [Pinot
Style](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#setup-ide)
##########
pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/IdenticalPredicateFilterOptimizer.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * 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 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;
+
+
+/**
+ * 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
+ public Expression optimize(Expression filterExpression, @Nullable Schema
schema) {
+ Function function = filterExpression.getFunctionCall();
+ if (function == null) {
+ return filterExpression;
+ }
+
+ List<Expression> 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) {
Review Comment:
Do we need to override this method? After the DFS, all the children should
already be optimized
##########
pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/FlattenAndOrFilterOptimizer.java:
##########
@@ -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)) {
Review Comment:
We should be able to eliminate the `true` and `false` here. Probably out of
the scope of this PR, and we can add a TODO to address later
##########
pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/QueryOptimizer.java:
##########
@@ -44,7 +45,7 @@ public class QueryOptimizer {
// values to the proper format so that they can be properly parsed
private static final List<FilterOptimizer> FILTER_OPTIMIZERS =
Arrays.asList(new FlattenAndOrFilterOptimizer(), new
MergeEqInFilterOptimizer(), new NumericalFilterOptimizer(),
- new TimePredicateFilterOptimizer(), new MergeRangeFilterOptimizer());
+ new TimePredicateFilterOptimizer(), new MergeRangeFilterOptimizer(),
new IdenticalPredicateFilterOptimizer());
Review Comment:
Should we apply this optimizer in the end? If it doesn't rely on other
optimizers, we can put it next to the flatten optimizer to avoid other
optimizer to optimize on identical predicate
##########
pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/filter/BaseAndOrBooleanFilterOptimizer.java:
##########
@@ -0,0 +1,98 @@
+/**
+ * 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 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;
+
+/**
+ * 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);
+ 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<Expression> 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;
+ }
+ }
+
+ // 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;
+ }
+ }
+
+ // 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;
+ }
+
+ protected boolean isAlwaysFalse(Expression operand) {
Review Comment:
If we don't need to override this method (see comment below), we can change
`optimizeCurrent` into a util method
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
