Skip to content

Commit

Permalink
Support filtering on bool/scalar fields without evaluator (#8518)
Browse files Browse the repository at this point in the history 
* Support filtering on bool/scalar fields without evaluator

Fixes the bugs in #8444 and #8487.
Added unit tests.

* Address review comments and refactor code

* Address review comments 2

* RequestContextUtil changes

* Review comments in RequestContextUtils

* Refactor RequestContextUtils

* Refactor ComparisonPredicateRewriter

Co-authored-by: Vivek Iyer Vaidyanathan <vvaidyan@vvaidyan-mn1.linkedin.biz>
  • Loading branch information
@vvivekiyer
vvivekiyer and Vivek Iyer Vaidyanathan committed May 2, 2022
1 parent b5690a7 commit d49d117
Show file tree
Hide file tree
Showing 7 changed files with 465 additions and 128 deletions.
69 changes: 63 additions & 6 deletions pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import org.apache.commons.lang3.EnumUtils;
import org.apache.pinot.common.request.Expression;
import org.apache.pinot.common.request.ExpressionType;
import org.apache.pinot.common.request.FilterOperator;
Expand All @@ -37,6 +38,7 @@
import org.apache.pinot.common.request.context.predicate.TextMatchPredicate;
import org.apache.pinot.common.utils.RegexpPatternConverterUtils;
import org.apache.pinot.common.utils.request.FilterQueryTree;
import org.apache.pinot.common.utils.request.RequestUtils;
import org.apache.pinot.pql.parsers.Pql2Compiler;
import org.apache.pinot.pql.parsers.pql2.ast.AstNode;
import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
Expand Down Expand Up @@ -163,10 +165,37 @@ public static FunctionContext getFunction(FunctionCallAstNode astNode) {
/**
* Converts the given Thrift {@link Expression} into a {@link FilterContext}.
* <p>NOTE: Currently the query engine only accepts string literals as the right-hand side of the predicate, so we
* always convert the right-hand side expressions into strings.
* always convert the right-hand side expressions into strings. We also update boolean predicates that are
* missing an EQUALS filter operator.
*/
public static FilterContext getFilter(Expression thriftExpression) {
Function thriftFunction = thriftExpression.getFunctionCall();
ExpressionType type = thriftExpression.getType();
switch (type) {
case FUNCTION:
Function thriftFunction = thriftExpression.getFunctionCall();
return getFilter(thriftFunction);
case IDENTIFIER:
// Convert "WHERE a" to "WHERE a = true"
return new FilterContext(FilterContext.Type.PREDICATE, null,
new EqPredicate(getExpression(thriftExpression), getStringValue(RequestUtils.getLiteralExpression(true))));
case LITERAL:
// TODO: Handle literals.
throw new IllegalStateException();
default:
throw new IllegalStateException();
}
}

public static FilterContext getFilter(Function thriftFunction) {
String functionOperator = thriftFunction.getOperator();

// convert "WHERE startsWith(col, 'str')" to "WHERE startsWith(col, 'str') = true"
if (!EnumUtils.isValidEnum(FilterKind.class, functionOperator)) {
return new FilterContext(FilterContext.Type.PREDICATE, null,
new EqPredicate(ExpressionContext.forFunction(getFunction(thriftFunction)),
getStringValue(RequestUtils.getLiteralExpression(true))));
}

FilterKind filterKind = FilterKind.valueOf(thriftFunction.getOperator().toUpperCase());
List<Expression> operands = thriftFunction.getOperands();
int numOperands = operands.size();
Expand All @@ -185,7 +214,8 @@ public static FilterContext getFilter(Expression thriftExpression) {
return new FilterContext(FilterContext.Type.OR, children, null);
case NOT:
assert numOperands == 1;
return new FilterContext(FilterContext.Type.NOT, new ArrayList<>(Collections.singletonList(getFilter(operands.get(0)))), null);
return new FilterContext(FilterContext.Type.NOT,
new ArrayList<>(Collections.singletonList(getFilter(operands.get(0)))), null);
case EQUALS:
return new FilterContext(FilterContext.Type.PREDICATE, null,
new EqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
Expand Down Expand Up @@ -264,10 +294,36 @@ private static String getStringValue(Expression thriftExpression) {
/**
* Converts the given filter {@link ExpressionContext} into a {@link FilterContext}.
* <p>NOTE: Currently the query engine only accepts string literals as the right-hand side of the predicate, so we
* always convert the right-hand side expressions into strings.
* always convert the right-hand side expressions into strings. We also update boolean predicates that are
* missing an EQUALS filter operator.
*/
public static FilterContext getFilter(ExpressionContext filterExpression) {
FunctionContext filterFunction = filterExpression.getFunction();
ExpressionContext.Type type = filterExpression.getType();
switch (type) {
case FUNCTION:
FunctionContext filterFunction = filterExpression.getFunction();
return getFilter(filterFunction);
case IDENTIFIER:
return new FilterContext(FilterContext.Type.PREDICATE, null,
new EqPredicate(filterExpression, getStringValue(RequestUtils.getLiteralExpression(true))));
case LITERAL:
// TODO: Handle literals
throw new IllegalStateException();
default:
throw new IllegalStateException();
}
}

public static FilterContext getFilter(FunctionContext filterFunction) {
String functionOperator = filterFunction.getFunctionName().toUpperCase();

// convert "WHERE startsWith(col, 'str')" to "WHERE startsWith(col, 'str') = true"
if (!EnumUtils.isValidEnum(FilterKind.class, functionOperator)) {
return new FilterContext(FilterContext.Type.PREDICATE, null,
new EqPredicate(ExpressionContext.forFunction(filterFunction),
getStringValue(RequestUtils.getLiteralExpression(true))));
}

FilterKind filterKind = FilterKind.valueOf(filterFunction.getFunctionName().toUpperCase());
List<ExpressionContext> operands = filterFunction.getArguments();
int numOperands = operands.size();
Expand All @@ -286,7 +342,8 @@ public static FilterContext getFilter(ExpressionContext filterExpression) {
return new FilterContext(FilterContext.Type.OR, children, null);
case NOT:
assert numOperands == 1;
return new FilterContext(FilterContext.Type.NOT, new ArrayList<>(Collections.singletonList(getFilter(operands.get(0)))), null);
return new FilterContext(FilterContext.Type.NOT,
new ArrayList<>(Collections.singletonList(getFilter(operands.get(0)))), null);
case EQUALS:
return new FilterContext(FilterContext.Type.PREDICATE, null,
new EqPredicate(operands.get(0), getStringValue(operands.get(1))));
Expand Down
89 changes: 74 additions & 15 deletions ...mmon/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@
*/
package org.apache.pinot.sql.parsers.rewriter;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import org.apache.commons.lang3.EnumUtils;
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.PinotQuery;
import org.apache.pinot.common.utils.request.RequestUtils;
Expand All @@ -33,34 +36,69 @@ public class PredicateComparisonRewriter implements QueryRewriter {
public PinotQuery rewrite(PinotQuery pinotQuery) {
Expression filterExpression = pinotQuery.getFilterExpression();
if (filterExpression != null) {
pinotQuery.setFilterExpression(updateComparisonPredicate(filterExpression));
pinotQuery.setFilterExpression(updatePredicate(filterExpression));
}
Expression havingExpression = pinotQuery.getHavingExpression();
if (havingExpression != null) {
pinotQuery.setHavingExpression(updateComparisonPredicate(havingExpression));
pinotQuery.setHavingExpression(updatePredicate(havingExpression));
}
return pinotQuery;
}

// This method converts a predicate expression to the what Pinot could evaluate.
// For comparison expression, left operand could be any expression, but right operand only
// supports literal.
// E.g. 'WHERE a > b' will be updated to 'WHERE a - b > 0'
private static Expression updateComparisonPredicate(Expression expression) {
/**
* This method converts an expression to what Pinot could evaluate.
* 1. For comparison expression, left operand could be any expression, but right operand only
* supports literal. E.g. 'WHERE a > b' will be converted to 'WHERE a - b > 0'
* 2. Updates boolean predicates (literals and scalar functions) that are missing an EQUALS filter.
* E.g. 1: 'WHERE a' will be updated to 'WHERE a = true'
* E.g. 2: "WHERE startsWith(col, 'str')" will be updated to "WHERE startsWith(col, 'str') = true"
*
* @param expression current expression in the expression tree
* @return re-written expression.
*/
private static Expression updatePredicate(Expression expression) {
ExpressionType type = expression.getType();

switch (type) {
case FUNCTION:
return updateFunctionExpression(expression);
case IDENTIFIER:
return convertPredicateToEqualsBooleanExpression(expression);
case LITERAL:
// TODO: Convert literals to boolean expressions
return expression;
default:
throw new IllegalStateException();
}
}

/**
* Rewrites a function expression.
*
* @param expression
* @return re-written expression
*/
private static Expression updateFunctionExpression(Expression expression) {
Function function = expression.getFunctionCall();
if (function != null) {
FilterKind filterKind;
try {
filterKind = FilterKind.valueOf(function.getOperator());
} catch (Exception e) {
throw new SqlCompilationException("Unsupported filter kind: " + function.getOperator());
}
String functionOperator = function.getOperator();

if (!EnumUtils.isValidEnum(FilterKind.class, functionOperator)) {
// If the function is not of FilterKind, we have to rewrite the function.
// Example: A query like "select col1 from table where startsWith(col1, 'myStr') AND col2 > 10;" should be
// rewritten to "select col1 from table where startsWith(col1, 'myStr') = true AND col2 > 10;".
expression = convertPredicateToEqualsBooleanExpression(expression);
return expression;
} else {
FilterKind filterKind = FilterKind.valueOf(function.getOperator());
List<Expression> operands = function.getOperands();
switch (filterKind) {
case AND:
case OR:
case NOT:
operands.replaceAll(PredicateComparisonRewriter::updateComparisonPredicate);
for (int i = 0; i < operands.size(); i++) {
Expression operand = operands.get(i);
operands.set(i, updatePredicate(operand));
}
break;
case EQUALS:
case NOT_EQUALS:
Expand Down Expand Up @@ -102,9 +140,30 @@ private static Expression updateComparisonPredicate(Expression expression) {
break;
}
}

return expression;
}

/**
* Rewrite predicates to boolean expressions with EQUALS operator
* Example1: "select * from table where col1" converts to
* "select * from table where col1 = true"
* Example2: "select * from table where startsWith(col1, 'str')" converts to
* "select * from table where startsWith(col1, 'str') = true"
* @param expression Expression
* @return Rewritten expression
*/
private static Expression convertPredicateToEqualsBooleanExpression(Expression expression) {
Expression newExpression;
newExpression = RequestUtils.getFunctionExpression(FilterKind.EQUALS.name());
List<Expression> operands = new ArrayList<>();
operands.add(expression);
operands.add(RequestUtils.getLiteralExpression(true));
newExpression.getFunctionCall().setOperands(operands);

return newExpression;
}

/**
* The purpose of this method is to convert expression "0 < columnA" to "columnA > 0".
* The conversion would be:
Expand Down
Loading

0 comments on commit d49d117

Please sign in to comment.