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/10/17 21:25:44 UTC

[GitHub] [incubator-pinot] fx19880617 opened a new pull request #6152: Support using ordinals in GROUP BY and ORDER BY clause

fx19880617 opened a new pull request #6152:
URL: https://github.com/apache/incubator-pinot/pull/6152


   ## Description
   Support Ordinal position in the query for GROUP BY and ORDER BY clause.
   
   So we can support queries like:
   ```
   SELECT foo, bar, count(*) FROM myTable GROUP BY 1, 2 ORDER BY 1, 2 DESC
   ```
   
   
   ## Release Notes
   Support Ordinal position in the query for GROUP BY and ORDER BY 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.

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] [incubator-pinot] fx19880617 commented on a change in pull request #6152: Support using ordinals in GROUP BY and ORDER BY clause

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6152:
URL: https://github.com/apache/incubator-pinot/pull/6152#discussion_r506990820



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -364,6 +367,42 @@ private static void queryRewrite(PinotQuery pinotQuery) {
     validate(aliasMap, pinotQuery);
   }
 
+  private static void applyOrdinals(PinotQuery pinotQuery) {
+    // handle GROUP BY clause
+    for (int i = 0; i < pinotQuery.getGroupByListSize(); i++) {
+      final Expression groupByExpr = pinotQuery.getGroupByList().get(i);
+      if (groupByExpr.isSetLiteral() && groupByExpr.getLiteral().isSetLongValue()) {
+        final int ordinal = (int) groupByExpr.getLiteral().getLongValue();
+        pinotQuery.getGroupByList().set(i, getExpressionFromOrdinal(pinotQuery.getSelectList(), ordinal));
+      }
+    }
+
+    // handle ORDER BY clause
+    for (int i = 0; i < pinotQuery.getOrderByListSize(); i++) {
+      final Expression orderByExpr = pinotQuery.getOrderByList().get(i).getFunctionCall().getOperands().get(0);
+      if (orderByExpr.isSetLiteral() && orderByExpr.getLiteral().isSetLongValue()) {
+        final int ordinal = (int) orderByExpr.getLiteral().getLongValue();
+        pinotQuery.getOrderByList().get(i).getFunctionCall()
+            .setOperands(Arrays.asList(getExpressionFromOrdinal(pinotQuery.getSelectList(), ordinal)));
+      }
+    }

Review comment:
       added a test for this as well.




----------------------------------------------------------------
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



---------------------------------------------------------------------
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 #6152: Support using ordinals in GROUP BY and ORDER BY clause

Posted by GitBox <gi...@apache.org>.
fx19880617 merged pull request #6152:
URL: https://github.com/apache/incubator-pinot/pull/6152


   


----------------------------------------------------------------
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



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

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6152: Support using ordinals in GROUP BY and ORDER BY clause

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6152:
URL: https://github.com/apache/incubator-pinot/pull/6152#discussion_r506986716



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -364,6 +367,42 @@ private static void queryRewrite(PinotQuery pinotQuery) {
     validate(aliasMap, pinotQuery);
   }
 
+  private static void applyOrdinals(PinotQuery pinotQuery) {
+    // handle GROUP BY clause
+    for (int i = 0; i < pinotQuery.getGroupByListSize(); i++) {
+      final Expression groupByExpr = pinotQuery.getGroupByList().get(i);
+      if (groupByExpr.isSetLiteral() && groupByExpr.getLiteral().isSetLongValue()) {
+        final int ordinal = (int) groupByExpr.getLiteral().getLongValue();
+        pinotQuery.getGroupByList().set(i, getExpressionFromOrdinal(pinotQuery.getSelectList(), ordinal));
+      }
+    }
+
+    // handle ORDER BY clause
+    for (int i = 0; i < pinotQuery.getOrderByListSize(); i++) {
+      final Expression orderByExpr = pinotQuery.getOrderByList().get(i).getFunctionCall().getOperands().get(0);
+      if (orderByExpr.isSetLiteral() && orderByExpr.getLiteral().isSetLongValue()) {
+        final int ordinal = (int) orderByExpr.getLiteral().getLongValue();
+        pinotQuery.getOrderByList().get(i).getFunctionCall()
+            .setOperands(Arrays.asList(getExpressionFromOrdinal(pinotQuery.getSelectList(), ordinal)));
+      }
+    }

Review comment:
       Would it be possible to make it all or nothing? For example, when you go over the group by expression list and to get the long literal for ordinal position, if the first group by expression doesn't happen to be ordinal then we simply break out of this function immediately rather than going over the rest of the expressions.
   
   In other words, if the group by expression is not an ordinal reference, then remaining one's aren't too




----------------------------------------------------------------
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



---------------------------------------------------------------------
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 pull request #6152: Support using ordinals in GROUP BY and ORDER BY clause

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6152:
URL: https://github.com/apache/incubator-pinot/pull/6152#issuecomment-711086760


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6152?src=pr&el=h1) Report
   > Merging [#6152](https://codecov.io/gh/apache/incubator-pinot/pull/6152?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) will **decrease** coverage by `21.08%`.
   > The diff coverage is `49.14%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6152/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6152?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           master    #6152       +/-   ##
   ===========================================
   - Coverage   66.44%   45.36%   -21.09%     
   ===========================================
     Files        1075     1231      +156     
     Lines       54773    58102     +3329     
     Branches     8168     8577      +409     
   ===========================================
   - Hits        36396    26359    -10037     
   - Misses      15700    29579    +13879     
   + Partials     2677     2164      -513     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #integration | `45.36% <49.14%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6152?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
   | [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `26.66% <0.00%> (-30.48%)` | :arrow_down: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `22.22% <0.00%> (-26.62%)` | :arrow_down: |
   | [.../org/apache/pinot/client/ResultTableResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Jlc3VsdFRhYmxlUmVzdWx0U2V0LmphdmE=) | `24.00% <0.00%> (-10.29%)` | :arrow_down: |
   | [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `64.28% <ø> (-8.89%)` | :arrow_down: |
   | [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
   | ... and [1210 more](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6152?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/6152?src=pr&el=footer). Last update [1750548...b854209](https://codecov.io/gh/apache/incubator-pinot/pull/6152?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



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

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6152: Support using ordinals in GROUP BY and ORDER BY clause

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6152:
URL: https://github.com/apache/incubator-pinot/pull/6152#discussion_r506986845



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -364,6 +367,42 @@ private static void queryRewrite(PinotQuery pinotQuery) {
     validate(aliasMap, pinotQuery);
   }
 
+  private static void applyOrdinals(PinotQuery pinotQuery) {
+    // handle GROUP BY clause
+    for (int i = 0; i < pinotQuery.getGroupByListSize(); i++) {
+      final Expression groupByExpr = pinotQuery.getGroupByList().get(i);
+      if (groupByExpr.isSetLiteral() && groupByExpr.getLiteral().isSetLongValue()) {
+        final int ordinal = (int) groupByExpr.getLiteral().getLongValue();
+        pinotQuery.getGroupByList().set(i, getExpressionFromOrdinal(pinotQuery.getSelectList(), ordinal));
+      }
+    }
+
+    // handle ORDER BY clause
+    for (int i = 0; i < pinotQuery.getOrderByListSize(); i++) {
+      final Expression orderByExpr = pinotQuery.getOrderByList().get(i).getFunctionCall().getOperands().get(0);
+      if (orderByExpr.isSetLiteral() && orderByExpr.getLiteral().isSetLongValue()) {
+        final int ordinal = (int) orderByExpr.getLiteral().getLongValue();
+        pinotQuery.getOrderByList().get(i).getFunctionCall()
+            .setOperands(Arrays.asList(getExpressionFromOrdinal(pinotQuery.getSelectList(), ordinal)));
+      }
+    }

Review comment:
       I am just trying to see how to fail fast (for some cases) out of these query rewrite methods since most of them are iterating on select, group by or order by expressions




----------------------------------------------------------------
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



---------------------------------------------------------------------
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 #6152: Support using ordinals in GROUP BY and ORDER BY clause

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6152:
URL: https://github.com/apache/incubator-pinot/pull/6152#discussion_r507007311



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -364,6 +367,42 @@ private static void queryRewrite(PinotQuery pinotQuery) {
     validate(aliasMap, pinotQuery);
   }
 
+  private static void applyOrdinals(PinotQuery pinotQuery) {
+    // handle GROUP BY clause
+    for (int i = 0; i < pinotQuery.getGroupByListSize(); i++) {
+      final Expression groupByExpr = pinotQuery.getGroupByList().get(i);
+      if (groupByExpr.isSetLiteral() && groupByExpr.getLiteral().isSetLongValue()) {
+        final int ordinal = (int) groupByExpr.getLiteral().getLongValue();
+        pinotQuery.getGroupByList().set(i, getExpressionFromOrdinal(pinotQuery.getSelectList(), ordinal));
+      }
+    }
+
+    // handle ORDER BY clause
+    for (int i = 0; i < pinotQuery.getOrderByListSize(); i++) {
+      final Expression orderByExpr = pinotQuery.getOrderByList().get(i).getFunctionCall().getOperands().get(0);
+      if (orderByExpr.isSetLiteral() && orderByExpr.getLiteral().isSetLongValue()) {
+        final int ordinal = (int) orderByExpr.getLiteral().getLongValue();
+        pinotQuery.getOrderByList().get(i).getFunctionCall()
+            .setOperands(Arrays.asList(getExpressionFromOrdinal(pinotQuery.getSelectList(), ordinal)));
+      }
+    }

Review comment:
       Below is an example of query and results from mysql:
   ![image](https://user-images.githubusercontent.com/1202120/96357306-0a1e2100-10af-11eb-9793-d509141b6659.png)
   




----------------------------------------------------------------
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



---------------------------------------------------------------------
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 #6152: Support using ordinals in GROUP BY and ORDER BY clause

Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6152:
URL: https://github.com/apache/incubator-pinot/pull/6152#discussion_r506989630



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -364,6 +367,42 @@ private static void queryRewrite(PinotQuery pinotQuery) {
     validate(aliasMap, pinotQuery);
   }
 
+  private static void applyOrdinals(PinotQuery pinotQuery) {
+    // handle GROUP BY clause
+    for (int i = 0; i < pinotQuery.getGroupByListSize(); i++) {
+      final Expression groupByExpr = pinotQuery.getGroupByList().get(i);
+      if (groupByExpr.isSetLiteral() && groupByExpr.getLiteral().isSetLongValue()) {
+        final int ordinal = (int) groupByExpr.getLiteral().getLongValue();
+        pinotQuery.getGroupByList().set(i, getExpressionFromOrdinal(pinotQuery.getSelectList(), ordinal));
+      }
+    }
+
+    // handle ORDER BY clause
+    for (int i = 0; i < pinotQuery.getOrderByListSize(); i++) {
+      final Expression orderByExpr = pinotQuery.getOrderByList().get(i).getFunctionCall().getOperands().get(0);
+      if (orderByExpr.isSetLiteral() && orderByExpr.getLiteral().isSetLongValue()) {
+        final int ordinal = (int) orderByExpr.getLiteral().getLongValue();
+        pinotQuery.getOrderByList().get(i).getFunctionCall()
+            .setOperands(Arrays.asList(getExpressionFromOrdinal(pinotQuery.getSelectList(), ordinal)));
+      }
+    }

Review comment:
       I thought we should support the case of mixing column name/ordinal/alias.
   E.g. 
   ```
   select a, b + 2, func(c) as func_c, count(*) from data group by a, 2, func_c;
   ```




----------------------------------------------------------------
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



---------------------------------------------------------------------
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 pull request #6152: Support using ordinals in GROUP BY and ORDER BY clause

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6152:
URL: https://github.com/apache/incubator-pinot/pull/6152#issuecomment-711086760


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6152?src=pr&el=h1) Report
   > Merging [#6152](https://codecov.io/gh/apache/incubator-pinot/pull/6152?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) will **increase** coverage by `6.35%`.
   > The diff coverage is `60.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6152/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6152?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6152      +/-   ##
   ==========================================
   + Coverage   66.44%   72.79%   +6.35%     
   ==========================================
     Files        1075     1231     +156     
     Lines       54773    58091    +3318     
     Branches     8168     8577     +409     
   ==========================================
   + Hits        36396    42290    +5894     
   + Misses      15700    13013    -2687     
   - Partials     2677     2788     +111     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #integration | `45.23% <49.14%> (?)` | |
   | #unittests | `63.94% <38.09%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6152?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `53.33% <0.00%> (-3.81%)` | :arrow_down: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `44.44% <0.00%> (-4.40%)` | :arrow_down: |
   | [.../org/apache/pinot/client/ResultTableResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Jlc3VsdFRhYmxlUmVzdWx0U2V0LmphdmE=) | `24.00% <0.00%> (-10.29%)` | :arrow_down: |
   | [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `78.57% <ø> (+5.40%)` | :arrow_up: |
   | [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
   | [...pinot/common/function/AggregationFunctionType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
   | [.../pinot/common/function/DateTimePatternHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRGF0ZVRpbWVQYXR0ZXJuSGFuZGxlci5qYXZh) | `83.33% <ø> (ø)` | |
   | [...ot/common/function/FunctionDefinitionRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25EZWZpbml0aW9uUmVnaXN0cnkuamF2YQ==) | `88.88% <ø> (+44.44%)` | :arrow_up: |
   | ... and [983 more](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6152?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/6152?src=pr&el=footer). Last update [1750548...3ba0d7f](https://codecov.io/gh/apache/incubator-pinot/pull/6152?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



---------------------------------------------------------------------
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 pull request #6152: Support using ordinals in GROUP BY and ORDER BY clause

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6152:
URL: https://github.com/apache/incubator-pinot/pull/6152#issuecomment-711086760


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6152?src=pr&el=h1) Report
   > Merging [#6152](https://codecov.io/gh/apache/incubator-pinot/pull/6152?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) will **increase** coverage by `6.35%`.
   > The diff coverage is `60.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6152/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/6152?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #6152      +/-   ##
   ==========================================
   + Coverage   66.44%   72.80%   +6.35%     
   ==========================================
     Files        1075     1231     +156     
     Lines       54773    58102    +3329     
     Branches     8168     8577     +409     
   ==========================================
   + Hits        36396    42300    +5904     
   + Misses      15700    13013    -2687     
   - Partials     2677     2789     +112     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #integration | `45.36% <49.14%> (?)` | |
   | #unittests | `63.91% <38.09%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6152?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
   | [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
   | [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `53.33% <0.00%> (-3.81%)` | :arrow_down: |
   | [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `44.44% <0.00%> (-4.40%)` | :arrow_down: |
   | [.../org/apache/pinot/client/ResultTableResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Jlc3VsdFRhYmxlUmVzdWx0U2V0LmphdmE=) | `24.00% <0.00%> (-10.29%)` | :arrow_down: |
   | [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `78.57% <ø> (+5.40%)` | :arrow_up: |
   | [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
   | [...pinot/common/function/AggregationFunctionType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
   | [.../pinot/common/function/DateTimePatternHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRGF0ZVRpbWVQYXR0ZXJuSGFuZGxlci5qYXZh) | `83.33% <ø> (ø)` | |
   | [...ot/common/function/FunctionDefinitionRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25EZWZpbml0aW9uUmVnaXN0cnkuamF2YQ==) | `88.88% <ø> (+44.44%)` | :arrow_up: |
   | ... and [983 more](https://codecov.io/gh/apache/incubator-pinot/pull/6152/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6152?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/6152?src=pr&el=footer). Last update [1750548...b854209](https://codecov.io/gh/apache/incubator-pinot/pull/6152?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



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

[GitHub] [incubator-pinot] KKcorps commented on pull request #6152: Support using ordinals in GROUP BY and ORDER BY clause

Posted by GitBox <gi...@apache.org>.
KKcorps commented on pull request #6152:
URL: https://github.com/apache/incubator-pinot/pull/6152#issuecomment-712305364


   LGTM!


----------------------------------------------------------------
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



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

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6152: Support using ordinals in GROUP BY and ORDER BY clause

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6152:
URL: https://github.com/apache/incubator-pinot/pull/6152#discussion_r506986716



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -364,6 +367,42 @@ private static void queryRewrite(PinotQuery pinotQuery) {
     validate(aliasMap, pinotQuery);
   }
 
+  private static void applyOrdinals(PinotQuery pinotQuery) {
+    // handle GROUP BY clause
+    for (int i = 0; i < pinotQuery.getGroupByListSize(); i++) {
+      final Expression groupByExpr = pinotQuery.getGroupByList().get(i);
+      if (groupByExpr.isSetLiteral() && groupByExpr.getLiteral().isSetLongValue()) {
+        final int ordinal = (int) groupByExpr.getLiteral().getLongValue();
+        pinotQuery.getGroupByList().set(i, getExpressionFromOrdinal(pinotQuery.getSelectList(), ordinal));
+      }
+    }
+
+    // handle ORDER BY clause
+    for (int i = 0; i < pinotQuery.getOrderByListSize(); i++) {
+      final Expression orderByExpr = pinotQuery.getOrderByList().get(i).getFunctionCall().getOperands().get(0);
+      if (orderByExpr.isSetLiteral() && orderByExpr.getLiteral().isSetLongValue()) {
+        final int ordinal = (int) orderByExpr.getLiteral().getLongValue();
+        pinotQuery.getOrderByList().get(i).getFunctionCall()
+            .setOperands(Arrays.asList(getExpressionFromOrdinal(pinotQuery.getSelectList(), ordinal)));
+      }
+    }

Review comment:
       Would it be possible to make it all or nothing? For example, when you go over the group by expression list and to get the long literal for ordinal position, if the first group by expression doesn't happen to be ordinal then we simply break out of this function immediately rather than going over the rest of the expressions.
   
   In other words, if the first group by expression is not an ordinal reference, then remaining one's aren't too




----------------------------------------------------------------
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



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

[GitHub] [incubator-pinot] kishoreg commented on a change in pull request #6152: Support using ordinals in GROUP BY and ORDER BY clause

Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #6152:
URL: https://github.com/apache/incubator-pinot/pull/6152#discussion_r506988077



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -364,6 +367,42 @@ private static void queryRewrite(PinotQuery pinotQuery) {
     validate(aliasMap, pinotQuery);
   }
 
+  private static void applyOrdinals(PinotQuery pinotQuery) {
+    // handle GROUP BY clause
+    for (int i = 0; i < pinotQuery.getGroupByListSize(); i++) {
+      final Expression groupByExpr = pinotQuery.getGroupByList().get(i);
+      if (groupByExpr.isSetLiteral() && groupByExpr.getLiteral().isSetLongValue()) {
+        final int ordinal = (int) groupByExpr.getLiteral().getLongValue();
+        pinotQuery.getGroupByList().set(i, getExpressionFromOrdinal(pinotQuery.getSelectList(), ordinal));
+      }
+    }
+
+    // handle ORDER BY clause
+    for (int i = 0; i < pinotQuery.getOrderByListSize(); i++) {
+      final Expression orderByExpr = pinotQuery.getOrderByList().get(i).getFunctionCall().getOperands().get(0);
+      if (orderByExpr.isSetLiteral() && orderByExpr.getLiteral().isSetLongValue()) {
+        final int ordinal = (int) orderByExpr.getLiteral().getLongValue();
+        pinotQuery.getOrderByList().get(i).getFunctionCall()
+            .setOperands(Arrays.asList(getExpressionFromOrdinal(pinotQuery.getSelectList(), ordinal)));
+      }
+    }

Review comment:
       Do you know if sql allows a combination of ordinal and alias or actual expression itself in order/ group by




----------------------------------------------------------------
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



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