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

[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