[GitHub] [druid] rohangarg commented on a diff in pull request #13206: SQL: Use timestamp_floor when granularity is not safe.

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

rohangarg commented on code in PR #13206:
URL: https://github.com/apache/druid/pull/13206#discussion_r992484540


##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java:
##########
@@ -1219,10 +1280,22 @@ private GroupByQuery toGroupByQuery()
         }
         queryGranularity = granularity;
         int timestampDimensionIndexInDimensions = grouping.getDimensions().indexOf(dimensionExpression);
-        // these settings will only affect the most inner query sent to the down streaming compute nodes
-        theContext.put(GroupByQuery.CTX_TIMESTAMP_RESULT_FIELD, dimensionExpression.getOutputName());
-        theContext.put(GroupByQuery.CTX_TIMESTAMP_RESULT_FIELD_INDEX, timestampDimensionIndexInDimensions);
-        theContext.put(GroupByQuery.CTX_TIMESTAMP_RESULT_FIELD_GRANULARITY, queryGranularity);
+
+        if (canUseQueryGranularity(dataSource, filtration, queryGranularity)) {

Review Comment:
   maybe we can use this on L1272 with `if(granularity == null)` check? because this sets the queryGranularity to skip other grouping columns but may not set the context.



##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java:
##########
@@ -774,6 +784,53 @@ private static Filtration toFiltration(DimFilter filter, VirtualColumnRegistry v
     return Filtration.create(filter).optimize(virtualColumnRegistry.getFullRowSignature());
   }
 
+  /**
+   * Whether the provided combination of dataSource, filtration, and queryGranularity is safe to use in queries.
+   *
+   * Necessary because some combinations are unsafe, mainly because they would lead to the creation of too many
+   * time-granular buckets during query processing.
+   */
+  private static boolean canUseQueryGranularity(
+      final DataSource dataSource,
+      final Filtration filtration,
+      final Granularity queryGranularity
+  )
+  {
+    if (Granularities.ALL.equals(queryGranularity)) {
+      // Always OK: no storage adapter has problem with ALL.
+      return true;
+    }
+
+    if (DataSourceAnalysis.forDataSource(dataSource).isConcreteTableBased()) {
+      // Always OK: queries on concrete tables (regular Druid datasources) use segment-based storage adapters
+      // (IncrementalIndex or QueryableIndex). These clip query interval to data interval, making wide query
+      // intervals safer. They do not have special checks for granularity and interval safety.
+      return true;
+    }
+
+    // Query is against something other than a regular Druid table. Apply additional checks, because we can't
+    // count on interval-clipping to save us.
+
+    for (final Interval filtrationInterval : filtration.getIntervals()) {
+      // Query may be using RowBasedStorageAdapter. We don't know for sure, so check
+      // RowBasedStorageAdapter#isQueryGranularityAllowed to be safe.
+      if (!RowBasedStorageAdapter.isQueryGranularityAllowed(filtrationInterval, queryGranularity)) {
+        return false;
+      }
+
+      // Validate the interval against MAX_TIME_GRAINS_NON_DRUID_TABLE.
+      // Estimate based on the size of the first bucket, to avoid computing them all. (That's what we're
+      // trying to avoid!)
+      final Interval firstBucket = queryGranularity.bucket(filtrationInterval.getStart());

Review Comment:
   maybe the Granularity object should have a duration method as well - but that can be a follow-up later.



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