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/14 18:05:12 UTC
[GitHub] [pinot] amrishlal commented on a diff in pull request #8518: Support filtering on bool/scalar fields without evaluator
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