[GitHub] [druid] clintropolis commented on a change in pull request #11930: Scan: Add "orderBy" parameter.

[GitBox <gi...@apache.org>]

clintropolis commented on a change in pull request #11930:
URL: https://github.com/apache/druid/pull/11930#discussion_r752849195



##########
File path: processing/src/main/java/org/apache/druid/query/scan/ScanQueryEngine.java
##########
@@ -131,8 +131,8 @@
                     intervals.get(0),
                     query.getVirtualColumns(),
                     Granularities.ALL,
-                    query.getOrder().equals(ScanQuery.Order.DESCENDING) ||
-                    (query.getOrder().equals(ScanQuery.Order.NONE) && query.isDescending()),
+                    query.getTimeOrder().equals(ScanQuery.Order.DESCENDING) ||
+                    (query.getTimeOrder().equals(ScanQuery.Order.NONE) && query.isDescending()),

Review comment:
       I know this isn't new, but unless i'm missing something it looks like the scan query constructor hard codes `descending` to false, so the 2nd clause should never be true?

##########
File path: processing/src/main/java/org/apache/druid/query/scan/ScanQuery.java
##########
@@ -417,10 +542,92 @@ public String toString()
            ", limit=" + scanRowsLimit +
            ", dimFilter=" + dimFilter +
            ", columns=" + columns +
-           ", legacy=" + legacy +
+           (orderBys.isEmpty() ? "" : ", orderBy=" + orderBys) +
+           (legacy == null ? "" : ", legacy=" + legacy) +
+           ", context=" + getContext() +
            '}';
   }
 
+  /**
+   * Verify and reconcile the two ways of specifying ordering: "orderBy", which can refer to any column, and
+   * "order", which refers to the __time column.
+   *
+   * If only "order" is provided, it is returned as-is, along with an equivalent "orderBy".
+   *
+   * If only "orderBy" is provided, it is returned as-is. If it can be converted into an equivalent "order", then that
+   * equivalent "order" is also returned. Otherwise, "orderBy" is returned as-is and "order" is returned as NONE.
+   *
+   * If both "orderBy" and "order" are provided, this returns them as-is if they are compatible, or throws an
+   * exception if they are incompatible.
+   *
+   * @param orderByFromUser "orderBy" specified by the user (can refer to any column)
+   * @param orderFromUser   "order" specified by the user (refers to time order)
+   */
+  private static Pair<List<OrderBy>, Order> verifyAndReconcileOrdering(
+      @Nullable final List<OrderBy> orderByFromUser,
+      @Nullable final Order orderFromUser
+  )
+  {
+    final List<OrderBy> orderByRetVal;
+    final Order orderRetVal;
+
+    // Compute the returned orderBy.
+    if (orderByFromUser != null) {
+      orderByRetVal = orderByFromUser;
+    } else if (orderFromUser == null || orderFromUser == Order.NONE) {
+      orderByRetVal = Collections.emptyList();
+    } else {
+      orderByRetVal = Collections.singletonList(new OrderBy(ColumnHolder.TIME_COLUMN_NAME, orderFromUser));
+    }
+
+    // Compute the returned order.
+    orderRetVal = computeTimeOrderFromOrderBys(orderByRetVal);

Review comment:
       nit: it seems slightly funny to do this here, but also seems ok since there is no implementation yet of order by other than time. Mostly just wondering if it should be pushed down a bit further. I guess you didn't want to change the scan engine / query runner factory to work on `OrderBy` instead of `Order` and just do the checking there?




-- 
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@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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