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

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

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


##########
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:
   This logic is here because not all Granularities have a fixed duration. There is DurationGranularity, which does, but also PeriodGranularity, which does not. For example, P1Y changes durations on leap years and P1D changes durations for daylight savings time.



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