[GitHub] [incubator-pinot] jtao15 commented on a change in pull request #6259: Broker time range pruning(#6189)

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

jtao15 commented on a change in pull request #6259:
URL: https://github.com/apache/incubator-pinot/pull/6259#discussion_r522523961



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/SegmentPrunerFactory.java
##########
@@ -97,4 +105,44 @@ private static PartitionSegmentPruner getPartitionSegmentPruner(TableConfig tabl
       return new PartitionSegmentPruner(tableNameWithType, partitionColumn, propertyStore);
     }
   }
+
+  @Nullable
+  private static TimeRangeSegmentPruner getTimeRangeSegmentPruner(TableConfig tableConfig,
+      ZkHelixPropertyStore<ZNRecord> propertyStore) {
+    String tableNameWithType = tableConfig.getTableName();
+    SegmentsValidationAndRetentionConfig validationConfig = tableConfig.getValidationConfig();
+    if (validationConfig == null) {
+      LOGGER.warn("Cannot enable time range pruning without validation config for table: {}",
+          tableNameWithType);
+      return null;
+    }
+    String timeColumn = validationConfig.getTimeColumnName();
+    if (timeColumn == null) {
+      LOGGER.warn("Cannot enable time range pruning without time column for table: {}",
+          tableNameWithType);
+      return null;
+    }
+
+    LOGGER.info("Using TimeRangePruner on time column: {} for table: {}", timeColumn,
+        tableNameWithType);
+    return new TimeRangeSegmentPruner(tableConfig, propertyStore);
+  }
+
+  private static List<SegmentPruner> sortSegmentPruners(List<SegmentPruner> pruners) {
+    // If there's multiple pruners, move time range pruners to the front。
+    // Partition pruner run time is proportional to input # of segments while time range pruner is not,
+    // Prune based on time range first will have a smaller input size for partition pruners, so have better performance.
+    List<SegmentPruner> sortedPruners = new ArrayList<>();
+    for (SegmentPruner pruner : pruners) {
+      if (pruner instanceof TimeRangeSegmentPruner) {
+        sortedPruners.add(pruner);
+      }
+    }
+    for (SegmentPruner pruner: pruners) {
+      if (!(pruner instanceof TimeRangeSegmentPruner)) {

Review comment:
       The first for loop will add `TimeRangeSegmentPruner` to the sorted result, this loop are adding non-timeRange pruners. To avoid duplicated pruners, I think this is still needed.




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