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 2020/02/14 09:53:40 UTC

[GitHub] [incubator-pinot] fx19880617 opened a new pull request #5070: Handle predicate right side expression

fx19880617 opened a new pull request #5070: Handle predicate right side expression
URL: https://github.com/apache/incubator-pinot/pull/5070
 
 
   For operators:  =,<>, >, >=, <, <=, we used to only support literal only on right side.
   This change overwrites the predicate by moving right side to left side with minus operator.
   E.g., 
   `WHERE a > b` will be rewrote to `WHERE a - b > 0`
   `WHERE a = b + 10` will be rewrote to `WHERE a - (b + 10) = 0`

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #5070: Support non-literal expressions for right-side operand in predicate comparison

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #5070: Support non-literal expressions for right-side operand in predicate comparison
URL: https://github.com/apache/incubator-pinot/pull/5070#discussion_r384165432
 
 

 ##########
 File path: pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java
 ##########
 @@ -118,6 +118,56 @@ public void testFilterCaluses() {
     Assert.assertEquals(func.getOperands().get(4).getLiteral().getLongValue(), 17L);
   }
 
+  @Test
+  public void testFilterCalusesWithRightExpression() {
+    PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where a > b");
+    Function func = pinotQuery.getFilterExpression().getFunctionCall();
+    Assert.assertEquals(func.getOperator(), SqlKind.GREATER_THAN.name());
+    Assert.assertEquals(func.getOperands().get(0).getFunctionCall().getOperator(), "MINUS");
+    Assert.assertEquals(func.getOperands().get(0).getFunctionCall().getOperands().get(0).getIdentifier().getName(), "a");
+    Assert.assertEquals(func.getOperands().get(0).getFunctionCall().getOperands().get(1).getIdentifier().getName(), "b");
+    Assert.assertEquals(func.getOperands().get(1).getLiteral().getLongValue(), 0L);
+    pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where 0 < a-b");
+    func = pinotQuery.getFilterExpression().getFunctionCall();
+    Assert.assertEquals(func.getOperator(), SqlKind.GREATER_THAN.name());
+    Assert.assertEquals(func.getOperands().get(0).getFunctionCall().getOperator(), "MINUS");
+    Assert.assertEquals(func.getOperands().get(0).getFunctionCall().getOperands().get(0).getIdentifier().getName(), "a");
+    Assert.assertEquals(func.getOperands().get(0).getFunctionCall().getOperands().get(1).getIdentifier().getName(), "b");
+    Assert.assertEquals(func.getOperands().get(1).getLiteral().getLongValue(), 0L);
+
+
+    pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where b < 100 + c");
 
 Review comment:
   This will be supported as `foo1(bar1(a-b)) - foo2(bar2(c/d-e)) <= 0`.
   Will add a test case for this.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #5070: Support non-literal expressions for right-side operand in predicate comparison

Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5070: Support non-literal expressions for right-side operand in predicate comparison
URL: https://github.com/apache/incubator-pinot/pull/5070#discussion_r384117155
 
 

 ##########
 File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
 ##########
 @@ -289,6 +289,91 @@ private static PinotQuery compileCalciteSqlToPinotQuery(String sql) {
     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) {
 
 Review comment:
   Probably better to model this as a query re-write phase, so all re-writes can be done in a standardized 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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 merged pull request #5070: Support non-literal expressions for right-side operand in predicate comparison

Posted by GitBox <gi...@apache.org>.
fx19880617 merged pull request #5070: Support non-literal expressions for right-side operand in predicate comparison
URL: https://github.com/apache/incubator-pinot/pull/5070
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on issue #5070: Support non-literal expressions for right-side operand in predicate comparison

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on issue #5070: Support non-literal expressions for right-side operand in predicate comparison
URL: https://github.com/apache/incubator-pinot/pull/5070#issuecomment-592245251
 
 
   @fx19880617 Not related to this PR but these 2 queries does not work and make the test flaky:
   `SELECT COUNT(WeatherDelay) FROM mytable WHERE ArrDelay > CarrierDelay LIMIT 26`
   `SELECT COUNT(WeatherDelay) FROM mytable WHERE ArrDelay - CarrierDelay > 0 LIMIT 26`
   
   I think there are some bugs in the ExpressionFilterOperator

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io edited a comment on issue #5070: Handle predicate right side expression

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #5070: Handle predicate right side expression
URL: https://github.com/apache/incubator-pinot/pull/5070#issuecomment-586260624
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=h1) Report
   > Merging [#5070](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/344badeec1f2afe8ef74cd893deedb255be18c77?src=pr&el=desc) will **increase** coverage by `19.51%`.
   > The diff coverage is `74.57%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5070/graphs/tree.svg?width=650&token=4ibza2ugkz&height=150&src=pr)](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=tree)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #5070       +/-   ##
   =============================================
   + Coverage     39.01%   58.53%   +19.51%     
   - Complexity        0       12       +12     
   =============================================
     Files          1197     1208       +11     
     Lines         64058    64569      +511     
     Branches       9439     9489       +50     
   =============================================
   + Hits          24993    37794    +12801     
   + Misses        36920    24025    -12895     
   - Partials       2145     2750      +605
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...pache/pinot/common/utils/request/RequestUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvcmVxdWVzdC9SZXF1ZXN0VXRpbHMuamF2YQ==) | `83.14% <63.63%> (+30.16%)` | `0 <0> (ø)` | :arrow_down: |
   | [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `89.59% <77.08%> (+23.46%)` | `0 <0> (ø)` | :arrow_down: |
   | [...er/validation/BrokerResourceValidationManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci92YWxpZGF0aW9uL0Jyb2tlclJlc291cmNlVmFsaWRhdGlvbk1hbmFnZXIuamF2YQ==) | `25% <0%> (-56.25%)` | `0% <0%> (ø)` | |
   | [...a/manager/realtime/RealtimeSegmentDataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVTZWdtZW50RGF0YU1hbmFnZXIuamF2YQ==) | `50% <0%> (-25%)` | `0% <0%> (ø)` | |
   | [...e/impl/dictionary/LongOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvTG9uZ09uSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `63.41% <0%> (-4.88%)` | `0% <0%> (ø)` | |
   | [...impl/dictionary/DoubleOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT25IZWFwTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | `54.87% <0%> (-3.66%)` | `0% <0%> (ø)` | |
   | [.../impl/dictionary/FloatOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRmxvYXRPbkhlYXBNdXRhYmxlRGljdGlvbmFyeS5qYXZh) | `70.73% <0%> (-3.66%)` | `0% <0%> (ø)` | |
   | [...e/operator/dociditerators/BitmapDocIdIterator.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9CaXRtYXBEb2NJZEl0ZXJhdG9yLmphdmE=) | `60.71% <0%> (-3.58%)` | `0% <0%> (ø)` | |
   | [...me/impl/dictionary/IntOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvSW50T25IZWFwTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | `78.04% <0%> (-2.44%)` | `0% <0%> (ø)` | |
   | [...e/operator/dociditerators/SVScanDocIdIterator.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9TVlNjYW5Eb2NJZEl0ZXJhdG9yLmphdmE=) | `63.21% <0%> (-2.3%)` | `0% <0%> (ø)` | |
   | ... and [597 more](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=footer). Last update [344bade...557ad7c](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #5070: Support non-literal expressions for right-side operand in predicate comparison

Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5070: Support non-literal expressions for right-side operand in predicate comparison
URL: https://github.com/apache/incubator-pinot/pull/5070#discussion_r384123795
 
 

 ##########
 File path: pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java
 ##########
 @@ -118,6 +118,56 @@ public void testFilterCaluses() {
     Assert.assertEquals(func.getOperands().get(4).getLiteral().getLongValue(), 17L);
   }
 
+  @Test
+  public void testFilterCalusesWithRightExpression() {
+    PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where a > b");
+    Function func = pinotQuery.getFilterExpression().getFunctionCall();
+    Assert.assertEquals(func.getOperator(), SqlKind.GREATER_THAN.name());
+    Assert.assertEquals(func.getOperands().get(0).getFunctionCall().getOperator(), "MINUS");
+    Assert.assertEquals(func.getOperands().get(0).getFunctionCall().getOperands().get(0).getIdentifier().getName(), "a");
+    Assert.assertEquals(func.getOperands().get(0).getFunctionCall().getOperands().get(1).getIdentifier().getName(), "b");
+    Assert.assertEquals(func.getOperands().get(1).getLiteral().getLongValue(), 0L);
+    pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where 0 < a-b");
+    func = pinotQuery.getFilterExpression().getFunctionCall();
+    Assert.assertEquals(func.getOperator(), SqlKind.GREATER_THAN.name());
+    Assert.assertEquals(func.getOperands().get(0).getFunctionCall().getOperator(), "MINUS");
+    Assert.assertEquals(func.getOperands().get(0).getFunctionCall().getOperands().get(0).getIdentifier().getName(), "a");
+    Assert.assertEquals(func.getOperands().get(0).getFunctionCall().getOperands().get(1).getIdentifier().getName(), "b");
+    Assert.assertEquals(func.getOperands().get(1).getLiteral().getLongValue(), 0L);
+
+
+    pinotQuery = CalciteSqlParser.compileToPinotQuery("select * from vegetables where b < 100 + c");
 
 Review comment:
   Does the code handle more complex expressions such as `foo1(bar1(a-b)) <= foo2(bar2(c/d-e)`? If so, let's add tests for those. If not, what's the plan there? Would it ever support those cases? 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #5070: Support non-literal expressions for right-side operand in predicate comparison

Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #5070: Support non-literal expressions for right-side operand in predicate comparison
URL: https://github.com/apache/incubator-pinot/pull/5070#discussion_r384116619
 
 

 ##########
 File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
 ##########
 @@ -289,6 +289,91 @@ private static PinotQuery compileCalciteSqlToPinotQuery(String sql) {
     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 functionCall = expression.getFunctionCall();
+    if (functionCall != null) {
+      SqlKind sqlKind = SqlKind.OTHER_FUNCTION;
+      try {
+        sqlKind = SqlKind.valueOf(functionCall.getOperator().toUpperCase());
+      } catch (Exception e) {
+        // Do nothing
+      }
+      switch (sqlKind) {
+        case EQUALS:
+        case NOT_EQUALS:
+        case GREATER_THAN:
+        case GREATER_THAN_OR_EQUAL:
+        case LESS_THAN:
+        case LESS_THAN_OR_EQUAL:
+          // Handle predicate like 'WHERE 10=a'
+          if (functionCall.getOperands().get(0).getLiteral() != null) {
+            functionCall.setOperator(getOppositeOperator(functionCall.getOperator()));
+            List<Expression> oldOperands = functionCall.getOperands();
+            Expression tempExpr = oldOperands.get(0);
+            oldOperands.set(0, oldOperands.get(1));
+            oldOperands.set(1, tempExpr);
+          }
+          if (functionCall.getOperands().get(1).getLiteral() != null) {
+            return expression;
+          }
+          Expression comparisonFunction = RequestUtils.getFunctionExpression(functionCall.getOperator());
+          List<Expression> exprList = new ArrayList<>();
+          exprList.add(getLeftOperand(functionCall));
+          exprList.add(RequestUtils.getLiteralExpression(0));
+          comparisonFunction.getFunctionCall().setOperands(exprList);
+          return comparisonFunction;
+        default:
+          List<Expression> operands = functionCall.getOperands();
+          List<Expression> newOperands = new ArrayList<>();
+          for (int i = 0; i < operands.size(); i++) {
+            newOperands.add(updateComparisonPredicate(operands.get(i)));
+          }
+          functionCall.setOperands(newOperands);
+      }
+    }
+    return expression;
+  }
+
+  private static String getOppositeOperator(String operator) {
+    switch (operator.toUpperCase()) {
+      case "GREATER_THAN":
 
 Review comment:
   Should opposite of GREATER_THAN be LESS_THAN_OR_EQUAL? 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #5070: Support non-literal expressions for right-side operand in predicate comparison

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #5070: Support non-literal expressions for right-side operand in predicate comparison
URL: https://github.com/apache/incubator-pinot/pull/5070#discussion_r384159457
 
 

 ##########
 File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
 ##########
 @@ -289,6 +289,91 @@ private static PinotQuery compileCalciteSqlToPinotQuery(String sql) {
     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 functionCall = expression.getFunctionCall();
+    if (functionCall != null) {
+      SqlKind sqlKind = SqlKind.OTHER_FUNCTION;
+      try {
+        sqlKind = SqlKind.valueOf(functionCall.getOperator().toUpperCase());
+      } catch (Exception e) {
+        // Do nothing
+      }
+      switch (sqlKind) {
+        case EQUALS:
+        case NOT_EQUALS:
+        case GREATER_THAN:
+        case GREATER_THAN_OR_EQUAL:
+        case LESS_THAN:
+        case LESS_THAN_OR_EQUAL:
+          // Handle predicate like 'WHERE 10=a'
+          if (functionCall.getOperands().get(0).getLiteral() != null) {
+            functionCall.setOperator(getOppositeOperator(functionCall.getOperator()));
+            List<Expression> oldOperands = functionCall.getOperands();
+            Expression tempExpr = oldOperands.get(0);
+            oldOperands.set(0, oldOperands.get(1));
+            oldOperands.set(1, tempExpr);
+          }
+          if (functionCall.getOperands().get(1).getLiteral() != null) {
+            return expression;
+          }
+          Expression comparisonFunction = RequestUtils.getFunctionExpression(functionCall.getOperator());
+          List<Expression> exprList = new ArrayList<>();
+          exprList.add(getLeftOperand(functionCall));
+          exprList.add(RequestUtils.getLiteralExpression(0));
+          comparisonFunction.getFunctionCall().setOperands(exprList);
+          return comparisonFunction;
+        default:
+          List<Expression> operands = functionCall.getOperands();
+          List<Expression> newOperands = new ArrayList<>();
+          for (int i = 0; i < operands.size(); i++) {
+            newOperands.add(updateComparisonPredicate(operands.get(i)));
+          }
+          functionCall.setOperands(newOperands);
+      }
+    }
+    return expression;
+  }
+
+  private static String getOppositeOperator(String operator) {
+    switch (operator.toUpperCase()) {
+      case "GREATER_THAN":
 
 Review comment:
   This is to handle expression of `0 < b` to `b > 0`. Will add comments to this.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #5070: Support non-literal expressions for right-side operand in predicate comparison

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #5070: Support non-literal expressions for right-side operand in predicate comparison
URL: https://github.com/apache/incubator-pinot/pull/5070#discussion_r384169237
 
 

 ##########
 File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
 ##########
 @@ -289,6 +289,91 @@ private static PinotQuery compileCalciteSqlToPinotQuery(String sql) {
     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) {
 
 Review comment:
   Adding a queryRewrite method, also moved alias map into it.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io edited a comment on issue #5070: Handle predicate right side expression

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #5070: Handle predicate right side expression
URL: https://github.com/apache/incubator-pinot/pull/5070#issuecomment-586260624
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=h1) Report
   > Merging [#5070](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/344badeec1f2afe8ef74cd893deedb255be18c77?src=pr&el=desc) will **increase** coverage by `19.48%`.
   > The diff coverage is `74.57%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5070/graphs/tree.svg?width=650&token=4ibza2ugkz&height=150&src=pr)](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #5070       +/-   ##
   ============================================
   + Coverage     39.01%   58.5%   +19.48%     
   - Complexity        0      12       +12     
   ============================================
     Files          1197    1208       +11     
     Lines         64058   64569      +511     
     Branches       9439    9489       +50     
   ============================================
   + Hits          24993   37774    +12781     
   + Misses        36920   24048    -12872     
   - Partials       2145    2747      +602
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...pache/pinot/common/utils/request/RequestUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvcmVxdWVzdC9SZXF1ZXN0VXRpbHMuamF2YQ==) | `83.14% <63.63%> (+30.16%)` | `0 <0> (ø)` | :arrow_down: |
   | [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `89.59% <77.08%> (+23.46%)` | `0 <0> (ø)` | :arrow_down: |
   | [...er/validation/BrokerResourceValidationManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci92YWxpZGF0aW9uL0Jyb2tlclJlc291cmNlVmFsaWRhdGlvbk1hbmFnZXIuamF2YQ==) | `25% <0%> (-56.25%)` | `0% <0%> (ø)` | |
   | [...a/manager/realtime/RealtimeSegmentDataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVTZWdtZW50RGF0YU1hbmFnZXIuamF2YQ==) | `50% <0%> (-25%)` | `0% <0%> (ø)` | |
   | [...mpl/dictionary/DoubleOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT2ZmSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `49.46% <0%> (-6.46%)` | `0% <0%> (ø)` | |
   | [...impl/dictionary/DoubleOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT25IZWFwTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | `54.87% <0%> (-3.66%)` | `0% <0%> (ø)` | |
   | [.../impl/dictionary/FloatOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRmxvYXRPbkhlYXBNdXRhYmxlRGljdGlvbmFyeS5qYXZh) | `70.73% <0%> (-3.66%)` | `0% <0%> (ø)` | |
   | [...e/impl/dictionary/LongOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvTG9uZ09uSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `64.63% <0%> (-3.66%)` | `0% <0%> (ø)` | |
   | [...e/operator/dociditerators/BitmapDocIdIterator.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9CaXRtYXBEb2NJZEl0ZXJhdG9yLmphdmE=) | `60.71% <0%> (-3.58%)` | `0% <0%> (ø)` | |
   | [...me/impl/dictionary/IntOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvSW50T25IZWFwTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | `78.04% <0%> (-2.44%)` | `0% <0%> (ø)` | |
   | ... and [597 more](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=footer). Last update [344bade...2c766fe](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io commented on issue #5070: Handle predicate right side expression

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #5070: Handle predicate right side expression
URL: https://github.com/apache/incubator-pinot/pull/5070#issuecomment-586260624
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=h1) Report
   > Merging [#5070](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/344badeec1f2afe8ef74cd893deedb255be18c77?src=pr&el=desc) will **increase** coverage by `19.35%`.
   > The diff coverage is `76.59%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5070/graphs/tree.svg?width=650&token=4ibza2ugkz&height=150&src=pr)](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=tree)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #5070       +/-   ##
   =============================================
   + Coverage     39.01%   58.37%   +19.35%     
   - Complexity        0       12       +12     
   =============================================
     Files          1197     1197               
     Lines         64058    64103       +45     
     Branches       9439     9445        +6     
   =============================================
   + Hits          24993    37417    +12424     
   + Misses        36920    23940    -12980     
   - Partials       2145     2746      +601
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...pache/pinot/common/utils/request/RequestUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvcmVxdWVzdC9SZXF1ZXN0VXRpbHMuamF2YQ==) | `83.14% <63.63%> (+30.16%)` | `0 <0> (ø)` | :arrow_down: |
   | [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `90.55% <80.55%> (+24.42%)` | `0 <0> (ø)` | :arrow_down: |
   | [.../org/apache/pinot/client/PinotClientException.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Bpbm90Q2xpZW50RXhjZXB0aW9uLmphdmE=) | `33.33% <0%> (-33.34%)` | `0% <0%> (ø)` | |
   | [...a/manager/realtime/RealtimeSegmentDataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVTZWdtZW50RGF0YU1hbmFnZXIuamF2YQ==) | `50% <0%> (-25%)` | `0% <0%> (ø)` | |
   | [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `62% <0%> (-14%)` | `0% <0%> (ø)` | |
   | [...impl/dictionary/DoubleOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT25IZWFwTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | `47.56% <0%> (-10.98%)` | `0% <0%> (ø)` | |
   | [...pinot/broker/api/resources/PinotClientRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | `23.4% <0%> (-8.52%)` | `0% <0%> (ø)` | |
   | [...mpl/dictionary/DoubleOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT2ZmSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `50.53% <0%> (-5.38%)` | `0% <0%> (ø)` | |
   | [.../impl/dictionary/FloatOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRmxvYXRPbkhlYXBNdXRhYmxlRGljdGlvbmFyeS5qYXZh) | `70.73% <0%> (-3.66%)` | `0% <0%> (ø)` | |
   | [...e/impl/dictionary/LongOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvTG9uZ09uSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `64.63% <0%> (-3.66%)` | `0% <0%> (ø)` | |
   | ... and [580 more](https://codecov.io/gh/apache/incubator-pinot/pull/5070/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=footer). Last update [344bade...0852fc8](https://codecov.io/gh/apache/incubator-pinot/pull/5070?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] fx19880617 commented on issue #5070: Support non-literal expressions for right-side operand in predicate comparison

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on issue #5070: Support non-literal expressions for right-side operand in predicate comparison
URL: https://github.com/apache/incubator-pinot/pull/5070#issuecomment-592449528
 
 
   Yes. I'm seeing this exception:
   ```
   java.lang.AssertionError: Value: 0 does not match, expected: 95455, got broker value: 95461, got client value:95461
   PQL: SELECT COUNT(*) FROM mytable WHERE ArrDelay > CarrierDelay LIMIT 26
   SQL: [SELECT COUNT(*) FROM mytable WHERE ArrDelay > CarrierDelay LIMIT 26]
   ```
   Will create an issue for this.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org