[GitHub] [druid] gianm commented on a change in pull request #9959: Fix Subquery could not be converted to groupBy query

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

gianm commented on a change in pull request #9959:
URL: https://github.com/apache/druid/pull/9959#discussion_r433381288



##########
File path: processing/src/main/java/org/apache/druid/query/timeseries/TimeseriesQueryQueryToolChest.java
##########
@@ -404,11 +407,16 @@ public boolean isCacheable(TimeseriesQuery query, boolean willMergeRunners)
   @Override
   public RowSignature resultArraySignature(TimeseriesQuery query)
   {
-    return RowSignature.builder()
-                       .addTimeColumn()
-                       .addAggregators(query.getAggregatorSpecs())
-                       .addPostAggregators(query.getPostAggregatorSpecs())
-                       .build();
+
+    RowSignature.Builder rowSignatureBuilder = RowSignature.builder();
+    if (query.getDimensionSpec() != null) {
+      rowSignatureBuilder.addDimensions(Collections.singletonList(query.getDimensionSpec()));

Review comment:
       I think you diagnosed the bug right but the fix is a bit sketchy. If the Timeseries query accepts a DimensionSpec but then only uses it in the array signature, the following problems occur:
   
   1. The input field, extractionFn, and decoration logic of the DimensionSpec are ignored.
   2. The type might not actually be correct here; it will use the type from the DimensionSpec, but that might not match the actual type of the field, because the query engine isn't enforcing it.
   3. The array signature should also match the maps returned from normal map-based responses, but this won't.
   
   I think the idea of a special parameter to the Timeseries query that makes the time column have a different name is a good idea, though. Maybe instead this would work:
   
   - Add an undocumented timeseries context parameter like `timestampResultField` that _adds_ a new field containing the timestamp as a long, with the given name, to both the map and array responses.
   - Modify the SQL layer to generate this context parameter for timeseries queries when there is a time floor dimension.




----------------------------------------------------------------
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@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org