gianm commented on code in PR #18235:
URL: https://github.com/apache/druid/pull/18235#discussion_r2211595649


##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/DataServerQueryHandlerUtils.java:
##########
@@ -48,17 +56,65 @@ private DataServerQueryHandlerUtils()
    * Performs necessary transforms to a query destined for data servers. Does 
not update the list of segments; callers
    * should do this themselves using {@link 
Queries#withSpecificSegments(Query, List)}.
    *
-   * @param query      the query
-   * @param dataSource datasource name
+   * @param query          the query
+   * @param dataSourceName datasource name
    */
-  public static <R, T extends Query<R>> Query<R> prepareQuery(final T query, 
final String dataSource)
+  public static <R, T extends Query<R>> Query<R> prepareQuery(
+      final T query,
+      final int inputNumber,
+      final String dataSourceName
+  )
   {
     // MSQ changes the datasource to an inputNumber datasource. This needs to 
be changed back for data servers
     // to understand.
+    return query.withDataSource(transformDatasource(query.getDataSource(), 
inputNumber, dataSourceName));
+  }
 
-    // BUG: This transformation is incorrect; see 
https://github.com/apache/druid/issues/18198. It loses decorations
-    // such as join, unnest, etc.
-    return query.withDataSource(new TableDataSource(dataSource));
+  /**
+   * Transforms {@link InputNumberDataSource} and {@link 
RestrictedInputNumberDataSource}, which are only understood
+   * by MSQ tasks, back into {@link TableDataSource} and {@link 
RestrictedDataSource} recursivly.
+   */
+  static DataSource transformDatasource(
+      final DataSource dataSource,
+      final int inputNumber,
+      final String dataSourceName
+  )

Review Comment:
   I think there is an issue with doing this at SQL planning time, in that the 
query shape is only disallowed if there is realtime data. The shape is allowed 
if all data is historical. So, the check needs to be done at the point that 
this is known. The earliest possible place is at the controller.
   
   I do think it would be good to move this check to the controller, since it 
would satisfy the principle that validations should be done as early as 
possible. However, I'm not off the top of my head able to say exactly where it 
would go. If there is a nice place to put the validation in the controller, 
let's move it there; but if there isn't then I suppose we can keep it here.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to