Jackie-Jiang commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r863118643
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -33,75 +36,126 @@ 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, null));
}
Expression havingExpression = pinotQuery.getHavingExpression();
if (havingExpression != null) {
-
pinotQuery.setHavingExpression(updateComparisonPredicate(havingExpression));
+ pinotQuery.setHavingExpression(updatePredicate(havingExpression, null));
}
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) {
- 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());
- }
- List<Expression> operands = function.getOperands();
- switch (filterKind) {
- case AND:
- case OR:
- case NOT:
-
operands.replaceAll(PredicateComparisonRewriter::updateComparisonPredicate);
- break;
- case EQUALS:
- case NOT_EQUALS:
- case GREATER_THAN:
- case GREATER_THAN_OR_EQUAL:
- case LESS_THAN:
- case LESS_THAN_OR_EQUAL:
- Expression firstOperand = operands.get(0);
- Expression secondOperand = operands.get(1);
+ /**
+ * This method converts a predicate expression to the 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
+ * @param parentFunction parentFunction parent expression
+ * @return re-written expression.
+ */
+ private static Expression updatePredicate(Expression expression, Function
parentFunction) {
Review Comment:
We don't need to pass in the `parentFunction` here. The rewrite should not
rely on the parent function
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -33,75 +36,126 @@ 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, null));
}
Expression havingExpression = pinotQuery.getHavingExpression();
if (havingExpression != null) {
-
pinotQuery.setHavingExpression(updateComparisonPredicate(havingExpression));
+ pinotQuery.setHavingExpression(updatePredicate(havingExpression, null));
}
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) {
- 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());
- }
- List<Expression> operands = function.getOperands();
- switch (filterKind) {
- case AND:
- case OR:
- case NOT:
-
operands.replaceAll(PredicateComparisonRewriter::updateComparisonPredicate);
- break;
- case EQUALS:
- case NOT_EQUALS:
- case GREATER_THAN:
- case GREATER_THAN_OR_EQUAL:
- case LESS_THAN:
- case LESS_THAN_OR_EQUAL:
- Expression firstOperand = operands.get(0);
- Expression secondOperand = operands.get(1);
+ /**
+ * This method converts a predicate expression to the 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
+ * @param parentFunction parentFunction parent expression
+ * @return re-written expression.
+ */
+ private static Expression updatePredicate(Expression expression, Function
parentFunction) {
+ ExpressionType type = expression.getType();
+ if (type == ExpressionType.FUNCTION) {
+ Function function = expression.getFunctionCall();
+ String functionOperator = function.getOperator();
- // Handle predicate like '10 = a' -> 'a = 10'
- if (firstOperand.isSetLiteral()) {
- if (!secondOperand.isSetLiteral()) {
- function.setOperator(getOppositeOperator(filterKind).name());
- operands.set(0, secondOperand);
- operands.set(1, firstOperand);
+ if (!EnumUtils.isValidEnum(FilterKind.class, functionOperator)) {
+ // If the function is not of FilterKind, we might have to rewrite the
function if parentFunction is not a
+ // predicate.
+ // 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 = convertPredicateToBooleanExpression(expression,
parentFunction);
Review Comment:
The example here does not apply because `AND` is a valid `FilterKind`, and
won't get into this branch.
This can be simplified to just rewriting the expression to `expression =
true` without any additional check
##########
pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/FilterKind.java:
##########
@@ -19,33 +19,41 @@
package org.apache.pinot.pql.parsers.pql2.ast;
public enum FilterKind {
- AND,
- OR,
- NOT,
- EQUALS,
- NOT_EQUALS,
- GREATER_THAN,
- GREATER_THAN_OR_EQUAL,
- LESS_THAN,
- LESS_THAN_OR_EQUAL,
- LIKE,
- BETWEEN,
- RANGE,
- IN,
- NOT_IN,
- REGEXP_LIKE,
- IS_NULL,
- IS_NOT_NULL,
- TEXT_MATCH,
- JSON_MATCH;
+ AND(false, false),
Review Comment:
I checked the usage of `isPredicate()` and it is actually not needed. See
the comments in `PredicateComparisonRewriter`.
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -33,75 +36,126 @@ 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, null));
}
Expression havingExpression = pinotQuery.getHavingExpression();
if (havingExpression != null) {
-
pinotQuery.setHavingExpression(updateComparisonPredicate(havingExpression));
+ pinotQuery.setHavingExpression(updatePredicate(havingExpression, null));
}
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) {
- 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());
- }
- List<Expression> operands = function.getOperands();
- switch (filterKind) {
- case AND:
- case OR:
- case NOT:
-
operands.replaceAll(PredicateComparisonRewriter::updateComparisonPredicate);
- break;
- case EQUALS:
- case NOT_EQUALS:
- case GREATER_THAN:
- case GREATER_THAN_OR_EQUAL:
- case LESS_THAN:
- case LESS_THAN_OR_EQUAL:
- Expression firstOperand = operands.get(0);
- Expression secondOperand = operands.get(1);
+ /**
+ * This method converts a predicate expression to the 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
+ * @param parentFunction parentFunction parent expression
+ * @return re-written expression.
+ */
+ private static Expression updatePredicate(Expression expression, Function
parentFunction) {
+ ExpressionType type = expression.getType();
+ if (type == ExpressionType.FUNCTION) {
+ Function function = expression.getFunctionCall();
+ String functionOperator = function.getOperator();
- // Handle predicate like '10 = a' -> 'a = 10'
- if (firstOperand.isSetLiteral()) {
- if (!secondOperand.isSetLiteral()) {
- function.setOperator(getOppositeOperator(filterKind).name());
- operands.set(0, secondOperand);
- operands.set(1, firstOperand);
+ if (!EnumUtils.isValidEnum(FilterKind.class, functionOperator)) {
+ // If the function is not of FilterKind, we might have to rewrite the
function if parentFunction is not a
+ // predicate.
+ // 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 = convertPredicateToBooleanExpression(expression,
parentFunction);
+ return expression;
+ } else {
+ FilterKind filterKind = FilterKind.valueOf(function.getOperator());
+ List<Expression> operands = function.getOperands();
+ switch (filterKind) {
+ case AND:
+ case OR:
+ case NOT:
+ for (int i = 0; i < operands.size(); i++) {
+ Expression operand = operands.get(i);
+ operands.set(i, updatePredicate(operand, function));
}
break;
- }
+ case EQUALS:
+ case NOT_EQUALS:
+ case GREATER_THAN:
+ case GREATER_THAN_OR_EQUAL:
+ case LESS_THAN:
+ case LESS_THAN_OR_EQUAL:
+ Expression firstOperand = operands.get(0);
+ Expression secondOperand = operands.get(1);
- // Handle predicate like 'a > b' -> 'a - b > 0'
- if (!secondOperand.isSetLiteral()) {
- Expression minusExpression =
RequestUtils.getFunctionExpression("minus");
-
minusExpression.getFunctionCall().setOperands(Arrays.asList(firstOperand,
secondOperand));
- operands.set(0, minusExpression);
- operands.set(1, RequestUtils.getLiteralExpression(0));
+ // Handle predicate like '10 = a' -> 'a = 10'
+ if (firstOperand.isSetLiteral()) {
+ if (!secondOperand.isSetLiteral()) {
+ function.setOperator(getOppositeOperator(filterKind).name());
+ operands.set(0, secondOperand);
+ operands.set(1, firstOperand);
+ }
+ break;
+ }
+
+ // Handle predicate like 'a > b' -> 'a - b > 0'
+ if (!secondOperand.isSetLiteral()) {
+ Expression minusExpression =
RequestUtils.getFunctionExpression("minus");
+
minusExpression.getFunctionCall().setOperands(Arrays.asList(firstOperand,
secondOperand));
+ operands.set(0, minusExpression);
+ operands.set(1, RequestUtils.getLiteralExpression(0));
+ break;
+ }
break;
- }
- break;
- default:
- int numOperands = operands.size();
- for (int i = 1; i < numOperands; i++) {
- if (!operands.get(i).isSetLiteral()) {
- throw new SqlCompilationException(
- String.format("For %s predicate, the operands except for the
first one must be literal, got: %s",
- filterKind, expression));
+ default:
+ int numOperands = operands.size();
+ for (int i = 1; i < numOperands; i++) {
+ if (!operands.get(i).isSetLiteral()) {
+ throw new SqlCompilationException(String
+ .format("For %s predicate, the operands except for the
first one must be literal, got: %s",
+ filterKind, expression));
+ }
}
- }
- break;
+ break;
+ }
}
+ } else if (type == ExpressionType.IDENTIFIER) {
+ expression = convertPredicateToBooleanExpression(expression,
parentFunction);
}
+
Review Comment:
Sounds good, we may add a TODO here and handle it separately
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -33,75 +36,126 @@ 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, null));
}
Expression havingExpression = pinotQuery.getHavingExpression();
if (havingExpression != null) {
-
pinotQuery.setHavingExpression(updateComparisonPredicate(havingExpression));
+ pinotQuery.setHavingExpression(updatePredicate(havingExpression, null));
}
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) {
- 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());
- }
- List<Expression> operands = function.getOperands();
- switch (filterKind) {
- case AND:
- case OR:
- case NOT:
-
operands.replaceAll(PredicateComparisonRewriter::updateComparisonPredicate);
- break;
- case EQUALS:
- case NOT_EQUALS:
- case GREATER_THAN:
- case GREATER_THAN_OR_EQUAL:
- case LESS_THAN:
- case LESS_THAN_OR_EQUAL:
- Expression firstOperand = operands.get(0);
- Expression secondOperand = operands.get(1);
+ /**
+ * This method converts a predicate expression to the 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
+ * @param parentFunction parentFunction parent expression
+ * @return re-written expression.
+ */
+ private static Expression updatePredicate(Expression expression, Function
parentFunction) {
+ ExpressionType type = expression.getType();
+ if (type == ExpressionType.FUNCTION) {
+ Function function = expression.getFunctionCall();
+ String functionOperator = function.getOperator();
- // Handle predicate like '10 = a' -> 'a = 10'
- if (firstOperand.isSetLiteral()) {
- if (!secondOperand.isSetLiteral()) {
- function.setOperator(getOppositeOperator(filterKind).name());
- operands.set(0, secondOperand);
- operands.set(1, firstOperand);
+ if (!EnumUtils.isValidEnum(FilterKind.class, functionOperator)) {
+ // If the function is not of FilterKind, we might have to rewrite the
function if parentFunction is not a
+ // predicate.
+ // 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 = convertPredicateToBooleanExpression(expression,
parentFunction);
+ return expression;
+ } else {
+ FilterKind filterKind = FilterKind.valueOf(function.getOperator());
+ List<Expression> operands = function.getOperands();
+ switch (filterKind) {
+ case AND:
+ case OR:
+ case NOT:
+ for (int i = 0; i < operands.size(); i++) {
+ Expression operand = operands.get(i);
+ operands.set(i, updatePredicate(operand, function));
}
break;
- }
+ case EQUALS:
+ case NOT_EQUALS:
+ case GREATER_THAN:
+ case GREATER_THAN_OR_EQUAL:
+ case LESS_THAN:
+ case LESS_THAN_OR_EQUAL:
+ Expression firstOperand = operands.get(0);
+ Expression secondOperand = operands.get(1);
- // Handle predicate like 'a > b' -> 'a - b > 0'
- if (!secondOperand.isSetLiteral()) {
- Expression minusExpression =
RequestUtils.getFunctionExpression("minus");
-
minusExpression.getFunctionCall().setOperands(Arrays.asList(firstOperand,
secondOperand));
- operands.set(0, minusExpression);
- operands.set(1, RequestUtils.getLiteralExpression(0));
+ // Handle predicate like '10 = a' -> 'a = 10'
+ if (firstOperand.isSetLiteral()) {
+ if (!secondOperand.isSetLiteral()) {
+ function.setOperator(getOppositeOperator(filterKind).name());
+ operands.set(0, secondOperand);
+ operands.set(1, firstOperand);
+ }
+ break;
+ }
+
+ // Handle predicate like 'a > b' -> 'a - b > 0'
+ if (!secondOperand.isSetLiteral()) {
+ Expression minusExpression =
RequestUtils.getFunctionExpression("minus");
+
minusExpression.getFunctionCall().setOperands(Arrays.asList(firstOperand,
secondOperand));
+ operands.set(0, minusExpression);
+ operands.set(1, RequestUtils.getLiteralExpression(0));
+ break;
+ }
break;
- }
- break;
- default:
- int numOperands = operands.size();
- for (int i = 1; i < numOperands; i++) {
- if (!operands.get(i).isSetLiteral()) {
- throw new SqlCompilationException(
- String.format("For %s predicate, the operands except for the
first one must be literal, got: %s",
- filterKind, expression));
+ default:
+ int numOperands = operands.size();
+ for (int i = 1; i < numOperands; i++) {
+ if (!operands.get(i).isSetLiteral()) {
+ throw new SqlCompilationException(String
+ .format("For %s predicate, the operands except for the
first one must be literal, got: %s",
+ filterKind, expression));
+ }
}
- }
- break;
+ break;
+ }
}
+ } else if (type == ExpressionType.IDENTIFIER) {
+ expression = convertPredicateToBooleanExpression(expression,
parentFunction);
}
+
+ return expression;
+ }
+
+ /**
+ * Rewrite if one of the two conditions are satisfied:
+ * 1. parent function is empty.
+ * 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"
+ * 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ * Example1: "select * from table where col1 AND startsWith(col2,
'str')" converts to
+ * "select * from table where col1 = true AND startsWith(col2,
'str') = true"
+ * @param expression Expression
+ * @param parentFunction Parent expression
+ * @return Rewritten expression
+ */
+ private static Expression convertPredicateToBooleanExpression(Expression
expression, Function parentFunction) {
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class,
parentFunction.getOperator()) && !FilterKind
Review Comment:
This condition should always be met, and we should be able to rewrite the
expression without any check. `parentFunction` is not needed here
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
