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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
