You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/04/12 21:25:27 UTC
[GitHub] [pinot] vvivekiyer opened a new pull request, #8518: Support filtering on bool/scalar fields without evaluator
vvivekiyer opened a new pull request, #8518:
URL: https://github.com/apache/pinot/pull/8518
Fixes the bugs in #8444 and #8487.
Added unit tests. Testing is in-progress.
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
2. `bugfix` (*)
3. `performance`
4. `ui`
5. `backward-incompat`
6. `release-notes`
2. Remove these instructions before publishing the PR.
(*) Other labels to consider:
- `testing`
- `dependencies`
- `docker`
- `kubernetes`
- `observability`
- `security`
- `code-style`
- `extension-point`
- `refactor`
- `cleanup`
(**) Use `release-notes` label for scenarios like:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r849077605
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -2246,6 +2246,19 @@ public void testFlattenAndOr() {
}
}
Review Comment:
We may also want to add a test where the boolean expression is as expected (e.g `WHERE col1 = true`) and therefore rewrite is a NO-OP or a mix of expressions where some are rewritten and some are NO-OP?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r852297396
##########
pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/FilterKind.java:
##########
@@ -39,13 +41,26 @@ public enum FilterKind {
TEXT_MATCH,
JSON_MATCH;
+ private static final EnumSet<FilterKind> RANGE_FILTERS = EnumSet.of(GREATER_THAN, GREATER_THAN_OR_EQUAL, LESS_THAN,
Review Comment:
Thanks for pointing this out. Done.
##########
pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/FilterKind.java:
##########
@@ -39,13 +41,26 @@ public enum FilterKind {
TEXT_MATCH,
JSON_MATCH;
+ private static final EnumSet<FilterKind> RANGE_FILTERS = EnumSet.of(GREATER_THAN, GREATER_THAN_OR_EQUAL, LESS_THAN,
+ LESS_THAN_OR_EQUAL, BETWEEN, RANGE);
+ private static final EnumSet<FilterKind> PREDICATE_FILTERS = EnumSet.of(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);
/**
* Helper method that returns true if the enum maps to a Range.
*
* @return True if the enum is of Range type, false otherwise.
*/
public boolean isRange() {
- return this == GREATER_THAN || this == GREATER_THAN_OR_EQUAL || this == LESS_THAN || this == LESS_THAN_OR_EQUAL
- || this == BETWEEN || this == RANGE;
+ return RANGE_FILTERS.contains(this);
+ }
+
+ /**
+ * Returns true if the filter is a predicate. Returns false otherwise. This logic should mimic FilterContext.Type.
+ *
+ * @return where the filter is a predicate.
+ */
Review Comment:
Done.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r849963269
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -2254,9 +2267,9 @@ public void testFlattenAndOr() {
List<Expression> operands = functionCall.getOperands();
Assert.assertEquals(operands.size(), 4);
Assert.assertEquals(operands.get(0).getFunctionCall().getOperator(), FilterKind.GREATER_THAN.name());
- Assert.assertEquals(operands.get(1).getIdentifier().getName(), "col2");
+ Assert.assertEquals(operands.get(1).getFunctionCall().getOperator(), FilterKind.EQUALS.name());
Assert.assertEquals(operands.get(2).getFunctionCall().getOperator(), FilterKind.GREATER_THAN.name());
- Assert.assertEquals(operands.get(3).getIdentifier().getName(), "col4");
+ Assert.assertEquals(operands.get(3).getFunctionCall().getOperator(), FilterKind.EQUALS.name());
Review Comment:
Done.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r859461905
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -33,75 +36,138 @@ 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.
+ *
+ *
+ * @param expression current expression in the expression tree
+ * @param parentFunction parent expression
+ * @return re-written expression.
+ */
Review Comment:
Done.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r861402444
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -163,94 +165,113 @@ 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();
- FilterKind filterKind = FilterKind.valueOf(thriftFunction.getOperator().toUpperCase());
- List<Expression> operands = thriftFunction.getOperands();
- int numOperands = operands.size();
- switch (filterKind) {
- case AND:
- List<FilterContext> children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- return new FilterContext(FilterContext.Type.AND, children, null);
- case OR:
- children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- 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);
- case EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new EqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case NOT_EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotEqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IN:
- List<String> values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new InPredicate(getExpression(operands.get(0)), values));
- case NOT_IN:
- values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotInPredicate(getExpression(operands.get(0)), values));
- case GREATER_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case GREATER_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case LESS_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, false,
- getStringValue(operands.get(1))));
- case LESS_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, true,
- getStringValue(operands.get(1))));
- case BETWEEN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), true,
- getStringValue(operands.get(2))));
- case RANGE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case REGEXP_LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)),
- RegexpPatternConverterUtils.likeToRegexpLike(getStringValue(operands.get(1)))));
- case TEXT_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new TextMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case JSON_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new JsonMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IS_NULL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new IsNullPredicate(getExpression(operands.get(0))));
- case IS_NOT_NULL:
+ ExpressionType type = thriftExpression.getType();
Review Comment:
Suggest refactoring the code a little bit so that it does not show as refactoring the whole thing:
- Add `private static FilterContext getFilter(Function thriftFunction)`
- Do a `switch(type)` and call it if it is `FUNCTION` type
##########
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:
Is this change related? We should not rely on type `FilterKind` for the purpose of this PR
##########
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),
+ OR(false, false),
+ NOT(false, false),
+ EQUALS(true, false),
+ NOT_EQUALS(true, false),
+ GREATER_THAN(true, true),
+ GREATER_THAN_OR_EQUAL(true, true),
+ LESS_THAN(true, true),
+ LESS_THAN_OR_EQUAL(true, true),
+ LIKE(true, false),
+ BETWEEN(true, true),
+ RANGE(true, true),
+ IN(true, false),
+ NOT_IN(true, false),
+ REGEXP_LIKE(true, false),
+ IS_NULL(true, false),
+ IS_NOT_NULL(true, false),
+ TEXT_MATCH(true, false),
+ JSON_MATCH(true, false);
- /**
- * Helper method that returns true if the enum maps to a Range.
- *
- * @return True if the enum is of Range type, false otherwise.
- */
+ private final boolean _isPredicate;
+ private final boolean _isRange;
+
+ FilterKind(boolean isPredicate, boolean isRange) {
+ _isPredicate = isPredicate;
+ _isRange = isRange;
+ }
+
+ /** @return true if the enum is of Range type, false otherwise. */
public boolean isRange() {
- return this == GREATER_THAN || this == GREATER_THAN_OR_EQUAL || this == LESS_THAN || this == LESS_THAN_OR_EQUAL
- || this == BETWEEN || this == RANGE;
+ return _isRange;
+ }
+
+ /** @return true if the filter is a predicate. This logic should mimic FilterContext.Type. */
+ public boolean isPredicate() {
Review Comment:
Don't add this API for now if it is not used
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -163,94 +165,113 @@ 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();
- FilterKind filterKind = FilterKind.valueOf(thriftFunction.getOperator().toUpperCase());
- List<Expression> operands = thriftFunction.getOperands();
- int numOperands = operands.size();
- switch (filterKind) {
- case AND:
- List<FilterContext> children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- return new FilterContext(FilterContext.Type.AND, children, null);
- case OR:
- children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- 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);
- case EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new EqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case NOT_EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotEqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IN:
- List<String> values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new InPredicate(getExpression(operands.get(0)), values));
- case NOT_IN:
- values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotInPredicate(getExpression(operands.get(0)), values));
- case GREATER_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case GREATER_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case LESS_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, false,
- getStringValue(operands.get(1))));
- case LESS_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, true,
- getStringValue(operands.get(1))));
- case BETWEEN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), true,
- getStringValue(operands.get(2))));
- case RANGE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case REGEXP_LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)),
- RegexpPatternConverterUtils.likeToRegexpLike(getStringValue(operands.get(1)))));
- case TEXT_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new TextMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case JSON_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new JsonMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IS_NULL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new IsNullPredicate(getExpression(operands.get(0))));
- case IS_NOT_NULL:
+ ExpressionType type = thriftExpression.getType();
+ if (type == ExpressionType.FUNCTION) {
+ Function thriftFunction = thriftExpression.getFunctionCall();
+ 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 IsNotNullPredicate(getExpression(operands.get(0))));
- default:
- throw new IllegalStateException();
+ new EqPredicate(getExpression(thriftExpression), getStringValue(RequestUtils.getLiteralExpression(true))));
+ }
+
+ FilterKind filterKind = FilterKind.valueOf(thriftFunction.getOperator().toUpperCase());
+ List<Expression> operands = thriftFunction.getOperands();
+ int numOperands = operands.size();
+ switch (filterKind) {
+ case AND:
+ List<FilterContext> children = new ArrayList<>(numOperands);
+ for (Expression operand : operands) {
+ children.add(getFilter(operand));
+ }
+ return new FilterContext(FilterContext.Type.AND, children, null);
+ case OR:
+ children = new ArrayList<>(numOperands);
+ for (Expression operand : operands) {
+ children.add(getFilter(operand));
+ }
+ 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);
+ case EQUALS:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new EqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
+ case NOT_EQUALS:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new NotEqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
+ case IN:
+ List<String> values = new ArrayList<>(numOperands - 1);
+ for (int i = 1; i < numOperands; i++) {
+ values.add(getStringValue(operands.get(i)));
+ }
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new InPredicate(getExpression(operands.get(0)), values));
+ case NOT_IN:
+ values = new ArrayList<>(numOperands - 1);
+ for (int i = 1; i < numOperands; i++) {
+ values.add(getStringValue(operands.get(i)));
+ }
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new NotInPredicate(getExpression(operands.get(0)), values));
+ case GREATER_THAN:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RangePredicate(getExpression(operands.get(0)), false, getStringValue(operands.get(1)), false,
+ RangePredicate.UNBOUNDED));
+ case GREATER_THAN_OR_EQUAL:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), false,
+ RangePredicate.UNBOUNDED));
+ case LESS_THAN:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, false,
+ getStringValue(operands.get(1))));
+ case LESS_THAN_OR_EQUAL:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, true,
+ getStringValue(operands.get(1))));
+ case BETWEEN:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), true,
+ getStringValue(operands.get(2))));
+ case RANGE:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RangePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
+ case REGEXP_LIKE:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RegexpLikePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
+ case LIKE:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RegexpLikePredicate(getExpression(operands.get(0)),
+ RegexpPatternConverterUtils.likeToRegexpLike(getStringValue(operands.get(1)))));
+ case TEXT_MATCH:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new TextMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
+ case JSON_MATCH:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new JsonMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
+ case IS_NULL:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new IsNullPredicate(getExpression(operands.get(0))));
+ case IS_NOT_NULL:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new IsNotNullPredicate(getExpression(operands.get(0))));
+ default:
+ throw new IllegalStateException();
+ }
+ } else if (type == ExpressionType.IDENTIFIER) {
+ // Convert "WHERE a" to "WHERE a = true"
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new EqPredicate(getExpression(thriftExpression), getStringValue(RequestUtils.getLiteralExpression(true))));
}
+
+ throw new IllegalArgumentException();
Review Comment:
We should also handle `literal`, where we handle `false`, `0`, `0.0` as false, others as true. Currently literal is always represented as string, so we have to take this work-around. Once we preserve the literal type, this can be simplified
##########
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:
We should also update literal to match the SQL semantic
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on a diff in pull request #8518: [WIP] Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
richardstartin commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r848924792
##########
pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/FilterKind.java:
##########
@@ -48,4 +48,13 @@ public boolean isRange() {
return this == GREATER_THAN || this == GREATER_THAN_OR_EQUAL || this == LESS_THAN || this == LESS_THAN_OR_EQUAL
|| this == BETWEEN || this == RANGE;
}
+
+ /**
+ * Returns true if the filter is a predicate. Returns false otherwise. This logic should mimic FilterContext.Type.
+ *
+ * @return where the filter is a predicate.
+ */
+ public boolean isPredicate() {
+ return this != AND && this != OR && this != NOT;
+ }
Review Comment:
This, along with the method `isRange` above it, is a little messy - it would be better to add fields to the enum `_isRange` and `_isPredicate` and set them in the enum initialiser.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r853410387
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -18,21 +18,27 @@
*/
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;
import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
import org.apache.pinot.sql.parsers.SqlCompilationException;
-
public class PredicateComparisonRewriter implements QueryRewriter {
@Override
public PinotQuery rewrite(PinotQuery pinotQuery) {
Expression filterExpression = pinotQuery.getFilterExpression();
if (filterExpression != null) {
+ filterExpression = updateBooleanPredicates(filterExpression, null);
Review Comment:
The expression within the WHERE and HAVING clause should be of boolean type, and `sum(field) > 10` will return a boolean result. Some functions can also return boolean result, and is not captured as `FilterKind`, e.g. `isWholeNumber(sum(field))`. For the HAVING clause, the aggregation result is treated as a regular column, so we should handle it the same as WHERE clause.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r863226578
##########
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:
Done.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r860332072
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -166,91 +168,119 @@ public static FunctionContext getFunction(FunctionCallAstNode astNode) {
* always convert the right-hand side expressions into strings.
*/
public static FilterContext getFilter(Expression thriftExpression) {
- Function thriftFunction = thriftExpression.getFunctionCall();
- FilterKind filterKind = FilterKind.valueOf(thriftFunction.getOperator().toUpperCase());
- List<Expression> operands = thriftFunction.getOperands();
- int numOperands = operands.size();
- switch (filterKind) {
- case AND:
- List<FilterContext> children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- return new FilterContext(FilterContext.Type.AND, children, null);
- case OR:
- children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- 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);
- case EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new EqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case NOT_EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotEqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IN:
- List<String> values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new InPredicate(getExpression(operands.get(0)), values));
- case NOT_IN:
- values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotInPredicate(getExpression(operands.get(0)), values));
- case GREATER_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case GREATER_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case LESS_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, false,
- getStringValue(operands.get(1))));
- case LESS_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, true,
- getStringValue(operands.get(1))));
- case BETWEEN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), true,
- getStringValue(operands.get(2))));
- case RANGE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case REGEXP_LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)),
- RegexpPatternConverterUtils.likeToRegexpLike(getStringValue(operands.get(1)))));
- case TEXT_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new TextMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case JSON_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new JsonMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IS_NULL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new IsNullPredicate(getExpression(operands.get(0))));
- case IS_NOT_NULL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new IsNotNullPredicate(getExpression(operands.get(0))));
- default:
- throw new IllegalStateException();
+ return getFilterHelper(thriftExpression, null);
+ }
+
+ private static FilterContext getFilterHelper(Expression thriftExpression, Function parentFunction) {
Review Comment:
Makes sense. Made the changes. Please review.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r849961063
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -42,6 +44,73 @@ public PinotQuery rewrite(PinotQuery pinotQuery) {
return pinotQuery;
}
+ /**
+ * Updates boolean predicates 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 parent expression
+ * @return re-written expression.
+ */
+ private static Expression updateBooleanPredicates(Expression expression, Function parentFunction) {
+ ExpressionType type = expression.getType();
+ if (type == ExpressionType.FUNCTION) {
+ Function function = expression.getFunctionCall();
+
+ // If the function is not of FilterKind, we might have to rewrite the function if parentFunction is AND, OR or
+ // NOT. 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;".
+ if (!EnumUtils.isValidEnum(FilterKind.class, function.getOperator())) {
+
+ // Rewrite non FilterKind functions if one of the two conditions are satified:
+ // 1. parent function is empty. Example: "select * from table where startsWith(col, 'myStr');
+ // 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ // Example: "select * from table where startsWith(col, 'myStr') AND col2 > 10";
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class, parentFunction.getOperator())
+ && !FilterKind.valueOf(parentFunction.getOperator()).isPredicate())) {
+ Expression currExpression = expression.deepCopy();
+ expression = RequestUtils.getFunctionExpression(FilterKind.EQUALS.name());
+ List<Expression> operands = new ArrayList<>();
+ operands.add(currExpression);
+ operands.add(RequestUtils.getLiteralExpression(true));
+ expression.getFunctionCall().setOperands(operands);
+
+ return expression;
+ }
+ }
+
+ List<Expression> operands = expression.getFunctionCall().getOperands();
+
+ if (operands.size() > 0) {
+ // unset operands only within the "if" condition because 'no argument' functions expect operand list to
+ // be initialized.
+ expression.getFunctionCall().unsetOperands();
+
+ for (Expression exp : operands) {
+ expression.getFunctionCall().addToOperands(updateBooleanPredicates(exp, expression.getFunctionCall()));
Review Comment:
I've refactored code and reused variables. Please take a look.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r860320909
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -166,91 +168,119 @@ public static FunctionContext getFunction(FunctionCallAstNode astNode) {
* always convert the right-hand side expressions into strings.
*/
public static FilterContext getFilter(Expression thriftExpression) {
- Function thriftFunction = thriftExpression.getFunctionCall();
- FilterKind filterKind = FilterKind.valueOf(thriftFunction.getOperator().toUpperCase());
- List<Expression> operands = thriftFunction.getOperands();
- int numOperands = operands.size();
- switch (filterKind) {
- case AND:
- List<FilterContext> children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- return new FilterContext(FilterContext.Type.AND, children, null);
- case OR:
- children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- 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);
- case EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new EqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case NOT_EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotEqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IN:
- List<String> values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new InPredicate(getExpression(operands.get(0)), values));
- case NOT_IN:
- values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotInPredicate(getExpression(operands.get(0)), values));
- case GREATER_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case GREATER_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case LESS_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, false,
- getStringValue(operands.get(1))));
- case LESS_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, true,
- getStringValue(operands.get(1))));
- case BETWEEN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), true,
- getStringValue(operands.get(2))));
- case RANGE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case REGEXP_LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)),
- RegexpPatternConverterUtils.likeToRegexpLike(getStringValue(operands.get(1)))));
- case TEXT_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new TextMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case JSON_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new JsonMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IS_NULL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new IsNullPredicate(getExpression(operands.get(0))));
- case IS_NOT_NULL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new IsNotNullPredicate(getExpression(operands.get(0))));
- default:
- throw new IllegalStateException();
+ return getFilterHelper(thriftExpression, null);
+ }
+
+ private static FilterContext getFilterHelper(Expression thriftExpression, Function parentFunction) {
Review Comment:
I added it as a safeguard for future-proofing. It is true that we never call when parentFunction is a predicate. Hence I throw an exception when parentFunction is a predicate.
If it seems over-kill (confusing), I can keep the existing function definition as it is currently and make the changes. Thoughts?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r859463210
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -18,21 +18,27 @@
*/
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;
import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
import org.apache.pinot.sql.parsers.SqlCompilationException;
-
public class PredicateComparisonRewriter implements QueryRewriter {
@Override
public PinotQuery rewrite(PinotQuery pinotQuery) {
Expression filterExpression = pinotQuery.getFilterExpression();
if (filterExpression != null) {
+ filterExpression = updateBooleanPredicates(filterExpression, null);
Review Comment:
Thanks for the pointer. Handled this case.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
richardstartin commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r850721889
##########
pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/FilterKind.java:
##########
@@ -39,13 +41,26 @@ public enum FilterKind {
TEXT_MATCH,
JSON_MATCH;
+ private static final EnumSet<FilterKind> RANGE_FILTERS = EnumSet.of(GREATER_THAN, GREATER_THAN_OR_EQUAL, LESS_THAN,
Review Comment:
I would prefer this were solved differently as requested previously:
```java
public enum FilterKind {
AND(false, false),
OR(false, false),
NOT(false, false),
EQUALS(true, false),
NOT_EQUALS(true, false),
GREATER_THAN(true, true),
GREATER_THAN_OR_EQUAL(true, true),
LESS_THAN(true, true),
LESS_THAN_OR_EQUAL(true, true),
LIKE(true, false),
BETWEEN(true, true),
RANGE(true, true),
IN(true, false),
NOT_IN(true, false),
REGEXP_LIKE(true, false),
IS_NULL(true, false),
IS_NOT_NULL(true, false),
TEXT_MATCH(true, false),
JSON_MATCH(true, false);
private final boolean _isPredicate;
private final boolean _isRange;
FilterKind(boolean isPredicate, boolean isRange) {
_isPredicate = isPredicate;
_isRange = isRange;
}
/**
* Helper method that returns true if the enum maps to a Range.
*
* @return True if the enum is of Range type, false otherwise.
*/
public boolean isRange() {
return _isRange;
}
}
```
this forces the modifier to define whether the new value is a predicate or a range predicate.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r849960934
##########
pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/FilterKind.java:
##########
@@ -48,4 +48,13 @@ public boolean isRange() {
return this == GREATER_THAN || this == GREATER_THAN_OR_EQUAL || this == LESS_THAN || this == LESS_THAN_OR_EQUAL
|| this == BETWEEN || this == RANGE;
}
+
+ /**
+ * Returns true if the filter is a predicate. Returns false otherwise. This logic should mimic FilterContext.Type.
+ *
+ * @return where the filter is a predicate.
+ */
+ public boolean isPredicate() {
+ return this != AND && this != OR && this != NOT;
+ }
Review Comment:
Done.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r852297661
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -79,13 +79,9 @@ public void testCanonicalFunctionName() {
public void testCaseWhenStatements() {
//@formatter:off
PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(
- "SELECT OrderID, Quantity,\n"
- + "CASE\n"
- + " WHEN Quantity > 30 THEN 'The quantity is greater than 30'\n"
- + " WHEN Quantity = 30 THEN 'The quantity is 30'\n"
- + " ELSE 'The quantity is under 30'\n"
- + "END AS QuantityText\n"
- + "FROM OrderDetails");
+ "SELECT OrderID, Quantity,\n" + "CASE\n" + " WHEN Quantity > 30 THEN 'The quantity is greater than 30'\n"
+ + " WHEN Quantity = 30 THEN 'The quantity is 30'\n" + " ELSE 'The quantity is under 30'\n"
+ + "END AS QuantityText\n" + "FROM OrderDetails");
Review Comment:
Done.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r849961945
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -2246,6 +2246,19 @@ public void testFlattenAndOr() {
}
}
Review Comment:
Added the following tests. WHERE clause containing:
1. Only boolean field.
2. Only boolean field (and) boolean field with equals.
3. Only scalar field.
4. Only scalar field (and) scalar field with equals.
5. Only scalar field (and) scalar field with equals and only boolean field (and) boolean field with equals
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r862999137
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -163,94 +165,113 @@ 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();
- FilterKind filterKind = FilterKind.valueOf(thriftFunction.getOperator().toUpperCase());
- List<Expression> operands = thriftFunction.getOperands();
- int numOperands = operands.size();
- switch (filterKind) {
- case AND:
- List<FilterContext> children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- return new FilterContext(FilterContext.Type.AND, children, null);
- case OR:
- children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- 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);
- case EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new EqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case NOT_EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotEqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IN:
- List<String> values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new InPredicate(getExpression(operands.get(0)), values));
- case NOT_IN:
- values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotInPredicate(getExpression(operands.get(0)), values));
- case GREATER_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case GREATER_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case LESS_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, false,
- getStringValue(operands.get(1))));
- case LESS_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, true,
- getStringValue(operands.get(1))));
- case BETWEEN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), true,
- getStringValue(operands.get(2))));
- case RANGE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case REGEXP_LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)),
- RegexpPatternConverterUtils.likeToRegexpLike(getStringValue(operands.get(1)))));
- case TEXT_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new TextMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case JSON_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new JsonMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IS_NULL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new IsNullPredicate(getExpression(operands.get(0))));
- case IS_NOT_NULL:
+ ExpressionType type = thriftExpression.getType();
Review Comment:
Done. Thanks!
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r863226257
##########
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:
I see your point now. The code is much cleaner following this suggestion. Thanks!
##########
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:
Done.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r853410777
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -33,75 +36,138 @@ 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.
+ *
+ *
+ * @param expression current expression in the expression tree
+ * @param parentFunction parent expression
+ * @return re-written expression.
+ */
Review Comment:
This part is duplicated
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r860308738
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -166,91 +168,119 @@ public static FunctionContext getFunction(FunctionCallAstNode astNode) {
* always convert the right-hand side expressions into strings.
*/
public static FilterContext getFilter(Expression thriftExpression) {
- Function thriftFunction = thriftExpression.getFunctionCall();
- FilterKind filterKind = FilterKind.valueOf(thriftFunction.getOperator().toUpperCase());
- List<Expression> operands = thriftFunction.getOperands();
- int numOperands = operands.size();
- switch (filterKind) {
- case AND:
- List<FilterContext> children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- return new FilterContext(FilterContext.Type.AND, children, null);
- case OR:
- children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- 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);
- case EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new EqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case NOT_EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotEqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IN:
- List<String> values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new InPredicate(getExpression(operands.get(0)), values));
- case NOT_IN:
- values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotInPredicate(getExpression(operands.get(0)), values));
- case GREATER_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case GREATER_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case LESS_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, false,
- getStringValue(operands.get(1))));
- case LESS_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, true,
- getStringValue(operands.get(1))));
- case BETWEEN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), true,
- getStringValue(operands.get(2))));
- case RANGE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case REGEXP_LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)),
- RegexpPatternConverterUtils.likeToRegexpLike(getStringValue(operands.get(1)))));
- case TEXT_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new TextMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case JSON_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new JsonMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IS_NULL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new IsNullPredicate(getExpression(operands.get(0))));
- case IS_NOT_NULL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new IsNotNullPredicate(getExpression(operands.get(0))));
- default:
- throw new IllegalStateException();
+ return getFilterHelper(thriftExpression, null);
+ }
+
+ private static FilterContext getFilterHelper(Expression thriftExpression, Function parentFunction) {
Review Comment:
I don't follow the logic here. Why do we need to pass in the `parentFunction` here? We should never call this method when the parent is a predicate.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r863005987
##########
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),
+ OR(false, false),
+ NOT(false, false),
+ EQUALS(true, false),
+ NOT_EQUALS(true, false),
+ GREATER_THAN(true, true),
+ GREATER_THAN_OR_EQUAL(true, true),
+ LESS_THAN(true, true),
+ LESS_THAN_OR_EQUAL(true, true),
+ LIKE(true, false),
+ BETWEEN(true, true),
+ RANGE(true, true),
+ IN(true, false),
+ NOT_IN(true, false),
+ REGEXP_LIKE(true, false),
+ IS_NULL(true, false),
+ IS_NOT_NULL(true, false),
+ TEXT_MATCH(true, false),
+ JSON_MATCH(true, false);
- /**
- * Helper method that returns true if the enum maps to a Range.
- *
- * @return True if the enum is of Range type, false otherwise.
- */
+ private final boolean _isPredicate;
+ private final boolean _isRange;
+
+ FilterKind(boolean isPredicate, boolean isRange) {
+ _isPredicate = isPredicate;
+ _isRange = isRange;
+ }
+
+ /** @return true if the enum is of Range type, false otherwise. */
public boolean isRange() {
- return this == GREATER_THAN || this == GREATER_THAN_OR_EQUAL || this == LESS_THAN || this == LESS_THAN_OR_EQUAL
- || this == BETWEEN || this == RANGE;
+ return _isRange;
+ }
+
+ /** @return true if the filter is a predicate. This logic should mimic FilterContext.Type. */
+ public boolean isPredicate() {
Review Comment:
Please see comment above.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r863005499
##########
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:
Yes, we use FilterKind in PredicateComparisonRewriter. It's used to determine we need to convert to EQUALS expression. For example:
`WHERE col1 and col2` should be converted
`WHERE col1 BETWEEN 10 AND 20` should not be converted.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia merged pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
siddharthteotia merged PR #8518:
URL: https://github.com/apache/pinot/pull/8518
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r849107128
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -42,6 +44,73 @@ public PinotQuery rewrite(PinotQuery pinotQuery) {
return pinotQuery;
}
+ /**
+ * Updates boolean predicates 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 parent expression
+ * @return re-written expression.
+ */
+ private static Expression updateBooleanPredicates(Expression expression, Function parentFunction) {
+ ExpressionType type = expression.getType();
+ if (type == ExpressionType.FUNCTION) {
+ Function function = expression.getFunctionCall();
+
+ // If the function is not of FilterKind, we might have to rewrite the function if parentFunction is AND, OR or
+ // NOT. 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;".
+ if (!EnumUtils.isValidEnum(FilterKind.class, function.getOperator())) {
+
+ // Rewrite non FilterKind functions if one of the two conditions are satified:
+ // 1. parent function is empty. Example: "select * from table where startsWith(col, 'myStr');
+ // 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ // Example: "select * from table where startsWith(col, 'myStr') AND col2 > 10";
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class, parentFunction.getOperator())
+ && !FilterKind.valueOf(parentFunction.getOperator()).isPredicate())) {
+ Expression currExpression = expression.deepCopy();
+ expression = RequestUtils.getFunctionExpression(FilterKind.EQUALS.name());
+ List<Expression> operands = new ArrayList<>();
+ operands.add(currExpression);
+ operands.add(RequestUtils.getLiteralExpression(true));
+ expression.getFunctionCall().setOperands(operands);
+
+ return expression;
+ }
+ }
+
+ List<Expression> operands = expression.getFunctionCall().getOperands();
+
+ if (operands.size() > 0) {
+ // unset operands only within the "if" condition because 'no argument' functions expect operand list to
+ // be initialized.
+ expression.getFunctionCall().unsetOperands();
+
+ for (Expression exp : operands) {
+ expression.getFunctionCall().addToOperands(updateBooleanPredicates(exp, expression.getFunctionCall()));
+ }
+ }
+ } else if (type == ExpressionType.IDENTIFIER) {
+
+ // Rewrite identifiers if one of the two conditions are satified:
+ // 1. parent function is empty. Example: "select * from table where col1";
+ // 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ // Example: "select * from table where col1 AND col2 > 10";
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class, parentFunction.getOperator())
Review Comment:
If the particular node is an identifier under the WHERE tree then does it matter if it has a parent or not ?
In both the above examples, as you traverse down the filter tree, you will reach `WHERE col1`, and enter this else branch. We just need to rewrite it . Why should we check that parent should be null or one of AND,. OR, NOT ?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r852301842
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -18,21 +18,27 @@
*/
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;
import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
import org.apache.pinot.sql.parsers.SqlCompilationException;
-
public class PredicateComparisonRewriter implements QueryRewriter {
@Override
public PinotQuery rewrite(PinotQuery pinotQuery) {
Expression filterExpression = pinotQuery.getFilterExpression();
if (filterExpression != null) {
+ filterExpression = updateBooleanPredicates(filterExpression, null);
Review Comment:
Done.
A question:
- Based on my understanding, we cannot have predicates that need to be converted to boolean expression in a HAVING clause - because it'll always work on an aggregation (eg: HAVING sum(field) > 10). Am I missing something?
- That's the reason I decided to not make these changes within `updateComparisonPredicate` earlier.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r860320909
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -166,91 +168,119 @@ public static FunctionContext getFunction(FunctionCallAstNode astNode) {
* always convert the right-hand side expressions into strings.
*/
public static FilterContext getFilter(Expression thriftExpression) {
- Function thriftFunction = thriftExpression.getFunctionCall();
- FilterKind filterKind = FilterKind.valueOf(thriftFunction.getOperator().toUpperCase());
- List<Expression> operands = thriftFunction.getOperands();
- int numOperands = operands.size();
- switch (filterKind) {
- case AND:
- List<FilterContext> children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- return new FilterContext(FilterContext.Type.AND, children, null);
- case OR:
- children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- 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);
- case EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new EqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case NOT_EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotEqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IN:
- List<String> values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new InPredicate(getExpression(operands.get(0)), values));
- case NOT_IN:
- values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotInPredicate(getExpression(operands.get(0)), values));
- case GREATER_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case GREATER_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case LESS_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, false,
- getStringValue(operands.get(1))));
- case LESS_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, true,
- getStringValue(operands.get(1))));
- case BETWEEN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), true,
- getStringValue(operands.get(2))));
- case RANGE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case REGEXP_LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)),
- RegexpPatternConverterUtils.likeToRegexpLike(getStringValue(operands.get(1)))));
- case TEXT_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new TextMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case JSON_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new JsonMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IS_NULL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new IsNullPredicate(getExpression(operands.get(0))));
- case IS_NOT_NULL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new IsNotNullPredicate(getExpression(operands.get(0))));
- default:
- throw new IllegalStateException();
+ return getFilterHelper(thriftExpression, null);
+ }
+
+ private static FilterContext getFilterHelper(Expression thriftExpression, Function parentFunction) {
Review Comment:
I added it as a safeguard for future-proofing. It is true that we never call when parentFunction is a predicate. Hence I throw an exception when parentFunction is a predicate.
If it's not needed, I can keep the existing function definition as it is currently and make the changes. Thoughts?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r849076687
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -2246,6 +2246,19 @@ public void testFlattenAndOr() {
}
}
+ {
+ String query = "SELECT * FROM foo WHERE col1 > 0 AND (col2 AND col3 > 0) AND startsWith(col4, 'myStr')";
+ PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(query);
+ Function functionCall = pinotQuery.getFilterExpression().getFunctionCall();
+ Assert.assertEquals(functionCall.getOperator(), FilterKind.AND.name());
+ List<Expression> operands = functionCall.getOperands();
+ Assert.assertEquals(operands.size(), 4);
+ Assert.assertEquals(operands.get(0).getFunctionCall().getOperator(), FilterKind.GREATER_THAN.name());
+ Assert.assertEquals(operands.get(1).getFunctionCall().getOperator(), FilterKind.EQUALS.name());
+ Assert.assertEquals(operands.get(2).getFunctionCall().getOperator(), FilterKind.GREATER_THAN.name());
+ Assert.assertEquals(operands.get(3).getFunctionCall().getOperator(), FilterKind.EQUALS.name());
Review Comment:
(nit) for the part that is being rewritten `startsWith(col4, 'myStr')`, we may want to assert for both LHS and RHS (esp RHS)
So in above test, for the 4th operand we are only checking for operator EQUALS. Would be good to check LHS is startsWith and RHS is true
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r849963166
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -42,6 +44,73 @@ public PinotQuery rewrite(PinotQuery pinotQuery) {
return pinotQuery;
}
+ /**
+ * Updates boolean predicates 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 parent expression
+ * @return re-written expression.
+ */
+ private static Expression updateBooleanPredicates(Expression expression, Function parentFunction) {
+ ExpressionType type = expression.getType();
+ if (type == ExpressionType.FUNCTION) {
+ Function function = expression.getFunctionCall();
+
+ // If the function is not of FilterKind, we might have to rewrite the function if parentFunction is AND, OR or
+ // NOT. 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;".
+ if (!EnumUtils.isValidEnum(FilterKind.class, function.getOperator())) {
+
+ // Rewrite non FilterKind functions if one of the two conditions are satified:
+ // 1. parent function is empty. Example: "select * from table where startsWith(col, 'myStr');
+ // 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ // Example: "select * from table where startsWith(col, 'myStr') AND col2 > 10";
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class, parentFunction.getOperator())
+ && !FilterKind.valueOf(parentFunction.getOperator()).isPredicate())) {
+ Expression currExpression = expression.deepCopy();
+ expression = RequestUtils.getFunctionExpression(FilterKind.EQUALS.name());
+ List<Expression> operands = new ArrayList<>();
+ operands.add(currExpression);
+ operands.add(RequestUtils.getLiteralExpression(true));
+ expression.getFunctionCall().setOperands(operands);
+
+ return expression;
+ }
+ }
+
+ List<Expression> operands = expression.getFunctionCall().getOperands();
+
+ if (operands.size() > 0) {
+ // unset operands only within the "if" condition because 'no argument' functions expect operand list to
+ // be initialized.
+ expression.getFunctionCall().unsetOperands();
+
+ for (Expression exp : operands) {
+ expression.getFunctionCall().addToOperands(updateBooleanPredicates(exp, expression.getFunctionCall()));
+ }
+ }
+ } else if (type == ExpressionType.IDENTIFIER) {
+
+ // Rewrite identifiers if one of the two conditions are satified:
+ // 1. parent function is empty. Example: "select * from table where col1";
+ // 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ // Example: "select * from table where col1 AND col2 > 10";
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class, parentFunction.getOperator())
Review Comment:
In both the above examples, when we reach `WHERE col1` we have to rewrite the query.
But consider the example query `WHERE concat(col1, "str")`. In this case we have to not rewrite the query.
So the logic is that, literals will be re-written only if at least one of the conditions is satisfied:
1. Parent function is null.
2. Parent function is one of AND, OR, NOT.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r849092454
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -42,6 +44,73 @@ public PinotQuery rewrite(PinotQuery pinotQuery) {
return pinotQuery;
}
+ /**
+ * Updates boolean predicates 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 parent expression
+ * @return re-written expression.
+ */
+ private static Expression updateBooleanPredicates(Expression expression, Function parentFunction) {
+ ExpressionType type = expression.getType();
+ if (type == ExpressionType.FUNCTION) {
+ Function function = expression.getFunctionCall();
+
+ // If the function is not of FilterKind, we might have to rewrite the function if parentFunction is AND, OR or
+ // NOT. 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;".
+ if (!EnumUtils.isValidEnum(FilterKind.class, function.getOperator())) {
+
+ // Rewrite non FilterKind functions if one of the two conditions are satified:
+ // 1. parent function is empty. Example: "select * from table where startsWith(col, 'myStr');
+ // 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ // Example: "select * from table where startsWith(col, 'myStr') AND col2 > 10";
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class, parentFunction.getOperator())
+ && !FilterKind.valueOf(parentFunction.getOperator()).isPredicate())) {
+ Expression currExpression = expression.deepCopy();
+ expression = RequestUtils.getFunctionExpression(FilterKind.EQUALS.name());
+ List<Expression> operands = new ArrayList<>();
+ operands.add(currExpression);
+ operands.add(RequestUtils.getLiteralExpression(true));
+ expression.getFunctionCall().setOperands(operands);
+
+ return expression;
+ }
+ }
+
+ List<Expression> operands = expression.getFunctionCall().getOperands();
+
+ if (operands.size() > 0) {
+ // unset operands only within the "if" condition because 'no argument' functions expect operand list to
+ // be initialized.
+ expression.getFunctionCall().unsetOperands();
+
+ for (Expression exp : operands) {
+ expression.getFunctionCall().addToOperands(updateBooleanPredicates(exp, expression.getFunctionCall()));
+ }
+ }
+ } else if (type == ExpressionType.IDENTIFIER) {
+
+ // Rewrite identifiers if one of the two conditions are satified:
+ // 1. parent function is empty. Example: "select * from table where col1";
+ // 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ // Example: "select * from table where col1 AND col2 > 10";
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class, parentFunction.getOperator())
+ && !FilterKind.valueOf(parentFunction.getOperator()).isPredicate())) {
+ Expression currExpression = expression.deepCopy();
Review Comment:
In-place modification / rewrite not possible ?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r849107128
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -42,6 +44,73 @@ public PinotQuery rewrite(PinotQuery pinotQuery) {
return pinotQuery;
}
+ /**
+ * Updates boolean predicates 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 parent expression
+ * @return re-written expression.
+ */
+ private static Expression updateBooleanPredicates(Expression expression, Function parentFunction) {
+ ExpressionType type = expression.getType();
+ if (type == ExpressionType.FUNCTION) {
+ Function function = expression.getFunctionCall();
+
+ // If the function is not of FilterKind, we might have to rewrite the function if parentFunction is AND, OR or
+ // NOT. 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;".
+ if (!EnumUtils.isValidEnum(FilterKind.class, function.getOperator())) {
+
+ // Rewrite non FilterKind functions if one of the two conditions are satified:
+ // 1. parent function is empty. Example: "select * from table where startsWith(col, 'myStr');
+ // 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ // Example: "select * from table where startsWith(col, 'myStr') AND col2 > 10";
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class, parentFunction.getOperator())
+ && !FilterKind.valueOf(parentFunction.getOperator()).isPredicate())) {
+ Expression currExpression = expression.deepCopy();
+ expression = RequestUtils.getFunctionExpression(FilterKind.EQUALS.name());
+ List<Expression> operands = new ArrayList<>();
+ operands.add(currExpression);
+ operands.add(RequestUtils.getLiteralExpression(true));
+ expression.getFunctionCall().setOperands(operands);
+
+ return expression;
+ }
+ }
+
+ List<Expression> operands = expression.getFunctionCall().getOperands();
+
+ if (operands.size() > 0) {
+ // unset operands only within the "if" condition because 'no argument' functions expect operand list to
+ // be initialized.
+ expression.getFunctionCall().unsetOperands();
+
+ for (Expression exp : operands) {
+ expression.getFunctionCall().addToOperands(updateBooleanPredicates(exp, expression.getFunctionCall()));
+ }
+ }
+ } else if (type == ExpressionType.IDENTIFIER) {
+
+ // Rewrite identifiers if one of the two conditions are satified:
+ // 1. parent function is empty. Example: "select * from table where col1";
+ // 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ // Example: "select * from table where col1 AND col2 > 10";
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class, parentFunction.getOperator())
Review Comment:
If the particular node is an identifier under the WHERE tree then does it matter if it has a parent or not ?
In both the above examples, as you traverse down the filter tree, you will reach `WHERE col1`, and enter this else branch. We just need to rewrite it . Why should we check that parent should be null or one of `AND, OR, NOT` ?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r849096065
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -42,6 +44,73 @@ public PinotQuery rewrite(PinotQuery pinotQuery) {
return pinotQuery;
}
+ /**
+ * Updates boolean predicates 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 parent expression
+ * @return re-written expression.
+ */
+ private static Expression updateBooleanPredicates(Expression expression, Function parentFunction) {
+ ExpressionType type = expression.getType();
+ if (type == ExpressionType.FUNCTION) {
+ Function function = expression.getFunctionCall();
+
+ // If the function is not of FilterKind, we might have to rewrite the function if parentFunction is AND, OR or
+ // NOT. 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;".
+ if (!EnumUtils.isValidEnum(FilterKind.class, function.getOperator())) {
+
+ // Rewrite non FilterKind functions if one of the two conditions are satified:
+ // 1. parent function is empty. Example: "select * from table where startsWith(col, 'myStr');
+ // 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ // Example: "select * from table where startsWith(col, 'myStr') AND col2 > 10";
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class, parentFunction.getOperator())
+ && !FilterKind.valueOf(parentFunction.getOperator()).isPredicate())) {
+ Expression currExpression = expression.deepCopy();
+ expression = RequestUtils.getFunctionExpression(FilterKind.EQUALS.name());
+ List<Expression> operands = new ArrayList<>();
+ operands.add(currExpression);
+ operands.add(RequestUtils.getLiteralExpression(true));
+ expression.getFunctionCall().setOperands(operands);
+
+ return expression;
+ }
+ }
+
+ List<Expression> operands = expression.getFunctionCall().getOperands();
+
+ if (operands.size() > 0) {
+ // unset operands only within the "if" condition because 'no argument' functions expect operand list to
+ // be initialized.
+ expression.getFunctionCall().unsetOperands();
+
+ for (Expression exp : operands) {
+ expression.getFunctionCall().addToOperands(updateBooleanPredicates(exp, expression.getFunctionCall()));
Review Comment:
(nit) for shorter / cleaner code: do expression.getFunctionCall() once ?
`Expression parent = expression.getFunctionCall()`
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] amrishlal commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
amrishlal commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r850680318
##########
pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/FilterKind.java:
##########
@@ -39,13 +41,26 @@ public enum FilterKind {
TEXT_MATCH,
JSON_MATCH;
+ private static final EnumSet<FilterKind> RANGE_FILTERS = EnumSet.of(GREATER_THAN, GREATER_THAN_OR_EQUAL, LESS_THAN,
Review Comment:
It is easy to forget updating the EnumSet when one updates the FilterKind. One option may be to move LIKE after BETWEEN and then do:
public boolean isRange() {
return this >= GREATER_THAN && this <= RANGE;
}
##########
pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/FilterKind.java:
##########
@@ -39,13 +41,26 @@ public enum FilterKind {
TEXT_MATCH,
JSON_MATCH;
+ private static final EnumSet<FilterKind> RANGE_FILTERS = EnumSet.of(GREATER_THAN, GREATER_THAN_OR_EQUAL, LESS_THAN,
+ LESS_THAN_OR_EQUAL, BETWEEN, RANGE);
+ private static final EnumSet<FilterKind> PREDICATE_FILTERS = EnumSet.of(EQUALS, NOT_EQUALS, GREATER_THAN,
Review Comment:
Same here, it's easy to forget updating PREDICATE_FILTERS when one updates FilterKind. Maybe the ordinal positions of enum in FilterKind can be fixed and ordinal positions used? That way we only have to maintain a single data structure.
##########
pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/FilterKind.java:
##########
@@ -39,13 +41,26 @@ public enum FilterKind {
TEXT_MATCH,
JSON_MATCH;
+ private static final EnumSet<FilterKind> RANGE_FILTERS = EnumSet.of(GREATER_THAN, GREATER_THAN_OR_EQUAL, LESS_THAN,
+ LESS_THAN_OR_EQUAL, BETWEEN, RANGE);
+ private static final EnumSet<FilterKind> PREDICATE_FILTERS = EnumSet.of(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);
/**
* Helper method that returns true if the enum maps to a Range.
*
* @return True if the enum is of Range type, false otherwise.
*/
public boolean isRange() {
- return this == GREATER_THAN || this == GREATER_THAN_OR_EQUAL || this == LESS_THAN || this == LESS_THAN_OR_EQUAL
- || this == BETWEEN || this == RANGE;
+ return RANGE_FILTERS.contains(this);
+ }
+
+ /**
+ * Returns true if the filter is a predicate. Returns false otherwise. This logic should mimic FilterContext.Type.
+ *
+ * @return where the filter is a predicate.
+ */
Review Comment:
A single line comment should be sufficient here :-) Same with comment on isRange function.
/** @return true if the filter is a predicate; otherwise, false. Logic should mimic FilterContext.Type. */
Can the logic be moved to a common place to avoid keeping the two logic in sync?
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -79,13 +79,9 @@ public void testCanonicalFunctionName() {
public void testCaseWhenStatements() {
//@formatter:off
PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(
- "SELECT OrderID, Quantity,\n"
- + "CASE\n"
- + " WHEN Quantity > 30 THEN 'The quantity is greater than 30'\n"
- + " WHEN Quantity = 30 THEN 'The quantity is 30'\n"
- + " ELSE 'The quantity is under 30'\n"
- + "END AS QuantityText\n"
- + "FROM OrderDetails");
+ "SELECT OrderID, Quantity,\n" + "CASE\n" + " WHEN Quantity > 30 THEN 'The quantity is greater than 30'\n"
+ + " WHEN Quantity = 30 THEN 'The quantity is 30'\n" + " ELSE 'The quantity is under 30'\n"
+ + "END AS QuantityText\n" + "FROM OrderDetails");
Review Comment:
Can we avoid unrelated formatting changes from this PR, so that it is easier to see what has changed or not changed?
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -191,45 +178,148 @@ public void testQuotedStrings() {
@Test
public void testFilterClauses() {
- PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where a > 1.5");
- Function func = pinotQuery.getFilterExpression().getFunctionCall();
- Assert.assertEquals(func.getOperator(), FilterKind.GREATER_THAN.name());
- Assert.assertEquals(func.getOperands().get(0).getIdentifier().getName(), "a");
- Assert.assertEquals(func.getOperands().get(1).getLiteral().getDoubleValue(), 1.5);
- pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where b < 100");
- func = pinotQuery.getFilterExpression().getFunctionCall();
- Assert.assertEquals(func.getOperator(), FilterKind.LESS_THAN.name());
- Assert.assertEquals(func.getOperands().get(0).getIdentifier().getName(), "b");
- Assert.assertEquals(func.getOperands().get(1).getLiteral().getLongValue(), 100L);
- pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where c >= 10");
- func = pinotQuery.getFilterExpression().getFunctionCall();
- Assert.assertEquals(func.getOperator(), FilterKind.GREATER_THAN_OR_EQUAL.name());
- Assert.assertEquals(func.getOperands().get(0).getIdentifier().getName(), "c");
- Assert.assertEquals(func.getOperands().get(1).getLiteral().getLongValue(), 10L);
- pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where d <= 50");
- func = pinotQuery.getFilterExpression().getFunctionCall();
- Assert.assertEquals(func.getOperator(), FilterKind.LESS_THAN_OR_EQUAL.name());
- Assert.assertEquals(func.getOperands().get(0).getIdentifier().getName(), "d");
- Assert.assertEquals(func.getOperands().get(1).getLiteral().getLongValue(), 50L);
- pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where e BETWEEN 70 AND 80");
- func = pinotQuery.getFilterExpression().getFunctionCall();
- Assert.assertEquals(func.getOperator(), FilterKind.BETWEEN.name());
- Assert.assertEquals(func.getOperands().get(0).getIdentifier().getName(), "e");
- Assert.assertEquals(func.getOperands().get(1).getLiteral().getLongValue(), 70L);
- Assert.assertEquals(func.getOperands().get(2).getLiteral().getLongValue(), 80L);
- pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where regexp_like(E, '^U.*')");
- func = pinotQuery.getFilterExpression().getFunctionCall();
- Assert.assertEquals(func.getOperator(), "REGEXP_LIKE");
- Assert.assertEquals(func.getOperands().get(0).getIdentifier().getName(), "E");
- Assert.assertEquals(func.getOperands().get(1).getLiteral().getStringValue(), "^U.*");
- pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where g IN (12, 13, 15.2, 17)");
- func = pinotQuery.getFilterExpression().getFunctionCall();
- Assert.assertEquals(func.getOperator(), FilterKind.IN.name());
- Assert.assertEquals(func.getOperands().get(0).getIdentifier().getName(), "g");
- Assert.assertEquals(func.getOperands().get(1).getLiteral().getLongValue(), 12L);
- Assert.assertEquals(func.getOperands().get(2).getLiteral().getLongValue(), 13L);
- Assert.assertEquals(func.getOperands().get(3).getLiteral().getDoubleValue(), 15.2);
- Assert.assertEquals(func.getOperands().get(4).getLiteral().getLongValue(), 17L);
+ {
Review Comment:
Same here. Can we avoid this formatting changes?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r860323002
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -166,91 +168,119 @@ public static FunctionContext getFunction(FunctionCallAstNode astNode) {
* always convert the right-hand side expressions into strings.
*/
public static FilterContext getFilter(Expression thriftExpression) {
- Function thriftFunction = thriftExpression.getFunctionCall();
- FilterKind filterKind = FilterKind.valueOf(thriftFunction.getOperator().toUpperCase());
- List<Expression> operands = thriftFunction.getOperands();
- int numOperands = operands.size();
- switch (filterKind) {
- case AND:
- List<FilterContext> children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- return new FilterContext(FilterContext.Type.AND, children, null);
- case OR:
- children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- 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);
- case EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new EqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case NOT_EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotEqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IN:
- List<String> values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new InPredicate(getExpression(operands.get(0)), values));
- case NOT_IN:
- values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotInPredicate(getExpression(operands.get(0)), values));
- case GREATER_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case GREATER_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case LESS_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, false,
- getStringValue(operands.get(1))));
- case LESS_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, true,
- getStringValue(operands.get(1))));
- case BETWEEN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), true,
- getStringValue(operands.get(2))));
- case RANGE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case REGEXP_LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)),
- RegexpPatternConverterUtils.likeToRegexpLike(getStringValue(operands.get(1)))));
- case TEXT_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new TextMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case JSON_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new JsonMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IS_NULL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new IsNullPredicate(getExpression(operands.get(0))));
- case IS_NOT_NULL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new IsNotNullPredicate(getExpression(operands.get(0))));
- default:
- throw new IllegalStateException();
+ return getFilterHelper(thriftExpression, null);
+ }
+
+ private static FilterContext getFilterHelper(Expression thriftExpression, Function parentFunction) {
Review Comment:
I'd suggest not checking the parent function for the boolean rewrite. Logically, the parsing should not rely on the parent function
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r852297879
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -191,45 +178,148 @@ public void testQuotedStrings() {
@Test
public void testFilterClauses() {
- PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where a > 1.5");
- Function func = pinotQuery.getFilterExpression().getFunctionCall();
- Assert.assertEquals(func.getOperator(), FilterKind.GREATER_THAN.name());
- Assert.assertEquals(func.getOperands().get(0).getIdentifier().getName(), "a");
- Assert.assertEquals(func.getOperands().get(1).getLiteral().getDoubleValue(), 1.5);
- pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where b < 100");
- func = pinotQuery.getFilterExpression().getFunctionCall();
- Assert.assertEquals(func.getOperator(), FilterKind.LESS_THAN.name());
- Assert.assertEquals(func.getOperands().get(0).getIdentifier().getName(), "b");
- Assert.assertEquals(func.getOperands().get(1).getLiteral().getLongValue(), 100L);
- pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where c >= 10");
- func = pinotQuery.getFilterExpression().getFunctionCall();
- Assert.assertEquals(func.getOperator(), FilterKind.GREATER_THAN_OR_EQUAL.name());
- Assert.assertEquals(func.getOperands().get(0).getIdentifier().getName(), "c");
- Assert.assertEquals(func.getOperands().get(1).getLiteral().getLongValue(), 10L);
- pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where d <= 50");
- func = pinotQuery.getFilterExpression().getFunctionCall();
- Assert.assertEquals(func.getOperator(), FilterKind.LESS_THAN_OR_EQUAL.name());
- Assert.assertEquals(func.getOperands().get(0).getIdentifier().getName(), "d");
- Assert.assertEquals(func.getOperands().get(1).getLiteral().getLongValue(), 50L);
- pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where e BETWEEN 70 AND 80");
- func = pinotQuery.getFilterExpression().getFunctionCall();
- Assert.assertEquals(func.getOperator(), FilterKind.BETWEEN.name());
- Assert.assertEquals(func.getOperands().get(0).getIdentifier().getName(), "e");
- Assert.assertEquals(func.getOperands().get(1).getLiteral().getLongValue(), 70L);
- Assert.assertEquals(func.getOperands().get(2).getLiteral().getLongValue(), 80L);
- pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where regexp_like(E, '^U.*')");
- func = pinotQuery.getFilterExpression().getFunctionCall();
- Assert.assertEquals(func.getOperator(), "REGEXP_LIKE");
- Assert.assertEquals(func.getOperands().get(0).getIdentifier().getName(), "E");
- Assert.assertEquals(func.getOperands().get(1).getLiteral().getStringValue(), "^U.*");
- pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where g IN (12, 13, 15.2, 17)");
- func = pinotQuery.getFilterExpression().getFunctionCall();
- Assert.assertEquals(func.getOperator(), FilterKind.IN.name());
- Assert.assertEquals(func.getOperands().get(0).getIdentifier().getName(), "g");
- Assert.assertEquals(func.getOperands().get(1).getLiteral().getLongValue(), 12L);
- Assert.assertEquals(func.getOperands().get(2).getLiteral().getLongValue(), 13L);
- Assert.assertEquals(func.getOperands().get(3).getLiteral().getDoubleValue(), 15.2);
- Assert.assertEquals(func.getOperands().get(4).getLiteral().getLongValue(), 17L);
+ {
Review Comment:
I'd prefer to keep this change. The test is cleaner this way.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r851566824
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -18,21 +18,27 @@
*/
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;
import org.apache.pinot.pql.parsers.pql2.ast.FilterKind;
import org.apache.pinot.sql.parsers.SqlCompilationException;
-
public class PredicateComparisonRewriter implements QueryRewriter {
@Override
public PinotQuery rewrite(PinotQuery pinotQuery) {
Expression filterExpression = pinotQuery.getFilterExpression();
if (filterExpression != null) {
+ filterExpression = updateBooleanPredicates(filterExpression, null);
Review Comment:
I'd do it within the `updateComparisonPredicate` (you may rename it to `updatePredicate`) so that it can be applied to both filter and having, and we just need to traverse the tree once
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r849963166
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -42,6 +44,73 @@ public PinotQuery rewrite(PinotQuery pinotQuery) {
return pinotQuery;
}
+ /**
+ * Updates boolean predicates 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 parent expression
+ * @return re-written expression.
+ */
+ private static Expression updateBooleanPredicates(Expression expression, Function parentFunction) {
+ ExpressionType type = expression.getType();
+ if (type == ExpressionType.FUNCTION) {
+ Function function = expression.getFunctionCall();
+
+ // If the function is not of FilterKind, we might have to rewrite the function if parentFunction is AND, OR or
+ // NOT. 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;".
+ if (!EnumUtils.isValidEnum(FilterKind.class, function.getOperator())) {
+
+ // Rewrite non FilterKind functions if one of the two conditions are satified:
+ // 1. parent function is empty. Example: "select * from table where startsWith(col, 'myStr');
+ // 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ // Example: "select * from table where startsWith(col, 'myStr') AND col2 > 10";
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class, parentFunction.getOperator())
+ && !FilterKind.valueOf(parentFunction.getOperator()).isPredicate())) {
+ Expression currExpression = expression.deepCopy();
+ expression = RequestUtils.getFunctionExpression(FilterKind.EQUALS.name());
+ List<Expression> operands = new ArrayList<>();
+ operands.add(currExpression);
+ operands.add(RequestUtils.getLiteralExpression(true));
+ expression.getFunctionCall().setOperands(operands);
+
+ return expression;
+ }
+ }
+
+ List<Expression> operands = expression.getFunctionCall().getOperands();
+
+ if (operands.size() > 0) {
+ // unset operands only within the "if" condition because 'no argument' functions expect operand list to
+ // be initialized.
+ expression.getFunctionCall().unsetOperands();
+
+ for (Expression exp : operands) {
+ expression.getFunctionCall().addToOperands(updateBooleanPredicates(exp, expression.getFunctionCall()));
+ }
+ }
+ } else if (type == ExpressionType.IDENTIFIER) {
+
+ // Rewrite identifiers if one of the two conditions are satified:
+ // 1. parent function is empty. Example: "select * from table where col1";
+ // 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ // Example: "select * from table where col1 AND col2 > 10";
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class, parentFunction.getOperator())
Review Comment:
In both the above example, when we reach `WHERE col1` we have to rewrite the query.
But consider the example query `WHERE concat(col1, "str")`. In this case we have to not rewrite the query.
So the logic is that, literals will be re-written only if at least one of the conditions is satisfied:
1. Parent function is null.
2. Parent function is one of AND, OR, NOT.
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -2246,6 +2246,19 @@ public void testFlattenAndOr() {
}
}
+ {
+ String query = "SELECT * FROM foo WHERE col1 > 0 AND (col2 AND col3 > 0) AND startsWith(col4, 'myStr')";
+ PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(query);
+ Function functionCall = pinotQuery.getFilterExpression().getFunctionCall();
+ Assert.assertEquals(functionCall.getOperator(), FilterKind.AND.name());
+ List<Expression> operands = functionCall.getOperands();
+ Assert.assertEquals(operands.size(), 4);
+ Assert.assertEquals(operands.get(0).getFunctionCall().getOperator(), FilterKind.GREATER_THAN.name());
+ Assert.assertEquals(operands.get(1).getFunctionCall().getOperator(), FilterKind.EQUALS.name());
+ Assert.assertEquals(operands.get(2).getFunctionCall().getOperator(), FilterKind.GREATER_THAN.name());
+ Assert.assertEquals(operands.get(3).getFunctionCall().getOperator(), FilterKind.EQUALS.name());
Review Comment:
Done.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r849961179
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/PredicateComparisonRewriter.java:
##########
@@ -42,6 +44,73 @@ public PinotQuery rewrite(PinotQuery pinotQuery) {
return pinotQuery;
}
+ /**
+ * Updates boolean predicates 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 parent expression
+ * @return re-written expression.
+ */
+ private static Expression updateBooleanPredicates(Expression expression, Function parentFunction) {
+ ExpressionType type = expression.getType();
+ if (type == ExpressionType.FUNCTION) {
+ Function function = expression.getFunctionCall();
+
+ // If the function is not of FilterKind, we might have to rewrite the function if parentFunction is AND, OR or
+ // NOT. 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;".
+ if (!EnumUtils.isValidEnum(FilterKind.class, function.getOperator())) {
+
+ // Rewrite non FilterKind functions if one of the two conditions are satified:
+ // 1. parent function is empty. Example: "select * from table where startsWith(col, 'myStr');
+ // 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ // Example: "select * from table where startsWith(col, 'myStr') AND col2 > 10";
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class, parentFunction.getOperator())
+ && !FilterKind.valueOf(parentFunction.getOperator()).isPredicate())) {
+ Expression currExpression = expression.deepCopy();
+ expression = RequestUtils.getFunctionExpression(FilterKind.EQUALS.name());
+ List<Expression> operands = new ArrayList<>();
+ operands.add(currExpression);
+ operands.add(RequestUtils.getLiteralExpression(true));
+ expression.getFunctionCall().setOperands(operands);
+
+ return expression;
+ }
+ }
+
+ List<Expression> operands = expression.getFunctionCall().getOperands();
+
+ if (operands.size() > 0) {
+ // unset operands only within the "if" condition because 'no argument' functions expect operand list to
+ // be initialized.
+ expression.getFunctionCall().unsetOperands();
+
+ for (Expression exp : operands) {
+ expression.getFunctionCall().addToOperands(updateBooleanPredicates(exp, expression.getFunctionCall()));
+ }
+ }
+ } else if (type == ExpressionType.IDENTIFIER) {
+
+ // Rewrite identifiers if one of the two conditions are satified:
+ // 1. parent function is empty. Example: "select * from table where col1";
+ // 2. Separator is not a predicate (non predicates are AND, OR, NOT).
+ // Example: "select * from table where col1 AND col2 > 10";
+ if (parentFunction == null || (EnumUtils.isValidEnum(FilterKind.class, parentFunction.getOperator())
+ && !FilterKind.valueOf(parentFunction.getOperator()).isPredicate())) {
+ Expression currExpression = expression.deepCopy();
Review Comment:
I've refactored code and modified in-place. Please take a look.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] amrishlal commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
amrishlal commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r850793054
##########
pinot-common/src/main/java/org/apache/pinot/pql/parsers/pql2/ast/FilterKind.java:
##########
@@ -39,13 +41,26 @@ public enum FilterKind {
TEXT_MATCH,
JSON_MATCH;
+ private static final EnumSet<FilterKind> RANGE_FILTERS = EnumSet.of(GREATER_THAN, GREATER_THAN_OR_EQUAL, LESS_THAN,
Review Comment:
Yes, this approach looks good. Sorry I missed your earlier comment.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r863007035
##########
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:
Can we add support for literals as a separate PR - asking because adding support and test for literals will make this PR larger? I'll send out a PR just for literals soon.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r863014083
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -163,94 +165,113 @@ 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();
- FilterKind filterKind = FilterKind.valueOf(thriftFunction.getOperator().toUpperCase());
- List<Expression> operands = thriftFunction.getOperands();
- int numOperands = operands.size();
- switch (filterKind) {
- case AND:
- List<FilterContext> children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- return new FilterContext(FilterContext.Type.AND, children, null);
- case OR:
- children = new ArrayList<>(numOperands);
- for (Expression operand : operands) {
- children.add(getFilter(operand));
- }
- 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);
- case EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new EqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case NOT_EQUALS:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotEqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IN:
- List<String> values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new InPredicate(getExpression(operands.get(0)), values));
- case NOT_IN:
- values = new ArrayList<>(numOperands - 1);
- for (int i = 1; i < numOperands; i++) {
- values.add(getStringValue(operands.get(i)));
- }
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new NotInPredicate(getExpression(operands.get(0)), values));
- case GREATER_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case GREATER_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), false,
- RangePredicate.UNBOUNDED));
- case LESS_THAN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, false,
- getStringValue(operands.get(1))));
- case LESS_THAN_OR_EQUAL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, true,
- getStringValue(operands.get(1))));
- case BETWEEN:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), true,
- getStringValue(operands.get(2))));
- case RANGE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RangePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case REGEXP_LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case LIKE:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new RegexpLikePredicate(getExpression(operands.get(0)),
- RegexpPatternConverterUtils.likeToRegexpLike(getStringValue(operands.get(1)))));
- case TEXT_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new TextMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case JSON_MATCH:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new JsonMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
- case IS_NULL:
- return new FilterContext(FilterContext.Type.PREDICATE, null,
- new IsNullPredicate(getExpression(operands.get(0))));
- case IS_NOT_NULL:
+ ExpressionType type = thriftExpression.getType();
+ if (type == ExpressionType.FUNCTION) {
+ Function thriftFunction = thriftExpression.getFunctionCall();
+ 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 IsNotNullPredicate(getExpression(operands.get(0))));
- default:
- throw new IllegalStateException();
+ new EqPredicate(getExpression(thriftExpression), getStringValue(RequestUtils.getLiteralExpression(true))));
+ }
+
+ FilterKind filterKind = FilterKind.valueOf(thriftFunction.getOperator().toUpperCase());
+ List<Expression> operands = thriftFunction.getOperands();
+ int numOperands = operands.size();
+ switch (filterKind) {
+ case AND:
+ List<FilterContext> children = new ArrayList<>(numOperands);
+ for (Expression operand : operands) {
+ children.add(getFilter(operand));
+ }
+ return new FilterContext(FilterContext.Type.AND, children, null);
+ case OR:
+ children = new ArrayList<>(numOperands);
+ for (Expression operand : operands) {
+ children.add(getFilter(operand));
+ }
+ 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);
+ case EQUALS:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new EqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
+ case NOT_EQUALS:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new NotEqPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
+ case IN:
+ List<String> values = new ArrayList<>(numOperands - 1);
+ for (int i = 1; i < numOperands; i++) {
+ values.add(getStringValue(operands.get(i)));
+ }
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new InPredicate(getExpression(operands.get(0)), values));
+ case NOT_IN:
+ values = new ArrayList<>(numOperands - 1);
+ for (int i = 1; i < numOperands; i++) {
+ values.add(getStringValue(operands.get(i)));
+ }
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new NotInPredicate(getExpression(operands.get(0)), values));
+ case GREATER_THAN:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RangePredicate(getExpression(operands.get(0)), false, getStringValue(operands.get(1)), false,
+ RangePredicate.UNBOUNDED));
+ case GREATER_THAN_OR_EQUAL:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), false,
+ RangePredicate.UNBOUNDED));
+ case LESS_THAN:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, false,
+ getStringValue(operands.get(1))));
+ case LESS_THAN_OR_EQUAL:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RangePredicate(getExpression(operands.get(0)), false, RangePredicate.UNBOUNDED, true,
+ getStringValue(operands.get(1))));
+ case BETWEEN:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RangePredicate(getExpression(operands.get(0)), true, getStringValue(operands.get(1)), true,
+ getStringValue(operands.get(2))));
+ case RANGE:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RangePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
+ case REGEXP_LIKE:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RegexpLikePredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
+ case LIKE:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new RegexpLikePredicate(getExpression(operands.get(0)),
+ RegexpPatternConverterUtils.likeToRegexpLike(getStringValue(operands.get(1)))));
+ case TEXT_MATCH:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new TextMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
+ case JSON_MATCH:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new JsonMatchPredicate(getExpression(operands.get(0)), getStringValue(operands.get(1))));
+ case IS_NULL:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new IsNullPredicate(getExpression(operands.get(0))));
+ case IS_NOT_NULL:
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new IsNotNullPredicate(getExpression(operands.get(0))));
+ default:
+ throw new IllegalStateException();
+ }
+ } else if (type == ExpressionType.IDENTIFIER) {
+ // Convert "WHERE a" to "WHERE a = true"
+ return new FilterContext(FilterContext.Type.PREDICATE, null,
+ new EqPredicate(getExpression(thriftExpression), getStringValue(RequestUtils.getLiteralExpression(true))));
}
+
+ throw new IllegalArgumentException();
Review Comment:
Can we add support for literals as a separate PR? (asking because adding support and test for literals will make this PR larger). I'll send out a PR just for literals soon.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #8518: [WIP] Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8518:
URL: https://github.com/apache/pinot/pull/8518#issuecomment-1097265336
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8518?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8518](https://codecov.io/gh/apache/pinot/pull/8518?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (197a7e9) into [master](https://codecov.io/gh/apache/pinot/commit/5e56b9a155b9f5d7ba3cd7214c15b3f1c697a351?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5e56b9a) will **decrease** coverage by `56.61%`.
> The diff coverage is `1.14%`.
> :exclamation: Current head 197a7e9 differs from pull request most recent head 9ca1489. Consider uploading reports for the commit 9ca1489 to get more accurate results
```diff
@@ Coverage Diff @@
## master #8518 +/- ##
=============================================
- Coverage 70.70% 14.09% -56.61%
+ Complexity 4285 84 -4201
=============================================
Files 1664 1629 -35
Lines 87342 85741 -1601
Branches 13227 13076 -151
=============================================
- Hits 61757 12087 -49670
- Misses 21296 72742 +51446
+ Partials 4289 912 -3377
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.09% <1.14%> (-0.08%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8518?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/8518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `22.83% <0.00%> (-29.23%)` | :arrow_down: |
| [...pache/pinot/common/config/provider/TableCache.java](https://codecov.io/gh/apache/pinot/pull/8518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3Byb3ZpZGVyL1RhYmxlQ2FjaGUuamF2YQ==) | `0.00% <0.00%> (-76.06%)` | :arrow_down: |
| [...inot/common/function/scalar/DateTimeFunctions.java](https://codecov.io/gh/apache/pinot/pull/8518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL0RhdGVUaW1lRnVuY3Rpb25zLmphdmE=) | `0.00% <ø> (-95.24%)` | :arrow_down: |
| [...java/org/apache/pinot/common/utils/DataSchema.java](https://codecov.io/gh/apache/pinot/pull/8518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvRGF0YVNjaGVtYS5qYXZh) | `0.00% <0.00%> (-79.33%)` | :arrow_down: |
| [.../java/org/apache/pinot/common/utils/DataTable.java](https://codecov.io/gh/apache/pinot/pull/8518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvRGF0YVRhYmxlLmphdmE=) | `0.00% <0.00%> (-89.48%)` | :arrow_down: |
| [.../apache/pinot/pql/parsers/pql2/ast/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/8518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9wcWwvcGFyc2Vycy9wcWwyL2FzdC9GaWx0ZXJLaW5kLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../parsers/rewriter/PredicateComparisonRewriter.java](https://codecov.io/gh/apache/pinot/pull/8518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9QcmVkaWNhdGVDb21wYXJpc29uUmV3cml0ZXIuamF2YQ==) | `0.00% <0.00%> (-83.73%)` | :arrow_down: |
| [...roller/api/resources/PinotTaskRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90VGFza1Jlc3RsZXRSZXNvdXJjZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...lix/core/minion/PinotHelixTaskResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9QaW5vdEhlbGl4VGFza1Jlc291cmNlTWFuYWdlci5qYXZh) | `2.38% <0.00%> (-41.81%)` | :arrow_down: |
| [...che/pinot/controller/util/FileIngestionHelper.java](https://codecov.io/gh/apache/pinot/pull/8518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci91dGlsL0ZpbGVJbmdlc3Rpb25IZWxwZXIuamF2YQ==) | `91.02% <ø> (ø)` | |
| ... and [1363 more](https://codecov.io/gh/apache/pinot/pull/8518/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8518?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8518?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5e56b9a...9ca1489](https://codecov.io/gh/apache/pinot/pull/8518?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8518:
URL: https://github.com/apache/pinot/pull/8518#discussion_r849076944
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -2254,9 +2267,9 @@ public void testFlattenAndOr() {
List<Expression> operands = functionCall.getOperands();
Assert.assertEquals(operands.size(), 4);
Assert.assertEquals(operands.get(0).getFunctionCall().getOperator(), FilterKind.GREATER_THAN.name());
- Assert.assertEquals(operands.get(1).getIdentifier().getName(), "col2");
+ Assert.assertEquals(operands.get(1).getFunctionCall().getOperator(), FilterKind.EQUALS.name());
Assert.assertEquals(operands.get(2).getFunctionCall().getOperator(), FilterKind.GREATER_THAN.name());
- Assert.assertEquals(operands.get(3).getIdentifier().getName(), "col4");
+ Assert.assertEquals(operands.get(3).getFunctionCall().getOperator(), FilterKind.EQUALS.name());
Review Comment:
Same 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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org