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

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

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



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentpruner/TimeSegmentPruner.java
##########
@@ -0,0 +1,346 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.broker.routing.segmentpruner;
+
+import com.google.common.base.Preconditions;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nullable;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.broker.routing.segmentpruner.interval.Interval;
+import org.apache.pinot.broker.routing.segmentpruner.interval.IntervalTree;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.common.utils.request.FilterQueryTree;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.data.DateTimeFieldSpec;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;
+import org.apache.pinot.spi.data.Schema;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * The {@code TimeSegmentPruner} prunes segments based on their time column start & end time metadata stored in ZK. The pruner
+ * supports queries with filter (or nested filter) of EQUALITY and RANGE predicates.
+ */
+public class TimeSegmentPruner implements SegmentPruner {
+  private static final Logger LOGGER = LoggerFactory.getLogger(TimeSegmentPruner.class);
+  private static final long MIN_START_TIME = 0;
+  private static final long MAX_END_TIME = Long.MAX_VALUE;
+  private static final Interval DEFAULT_INTERVAL = new Interval(MIN_START_TIME, MAX_END_TIME);
+  private static final char DELIMITER = '\0';
+  private static final String LEGACY_DELIMITER = "\t\t";
+  private static final char START_INCLUSIVE = '(';
+  private static final char END_INCLUSIVE = ')';
+  private static final String UNBOUNDED = "*";
+
+  private final String _tableNameWithType;
+  private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+  private final String _segmentZKMetadataPathPrefix;
+  private final String _timeColumn;
+  private final DateTimeFormatSpec _timeFormatSpec;
+
+  private volatile IntervalTree<String> _intervalTree;
+  private final Map<String, Interval> _intervalMap = new HashMap<>();
+
+  public TimeSegmentPruner(TableConfig tableConfig, ZkHelixPropertyStore<ZNRecord> propertyStore) {
+    _tableNameWithType = tableConfig.getTableName();
+    _propertyStore = propertyStore;
+    _segmentZKMetadataPathPrefix = ZKMetadataProvider.constructPropertyStorePathForResource(_tableNameWithType) + "/";
+    _timeColumn = tableConfig.getValidationConfig().getTimeColumnName();
+    Preconditions
+        .checkNotNull(_timeColumn, "Time column must be configured in table config for table: %s", _tableNameWithType);
+
+    Schema schema = ZKMetadataProvider.getTableSchema(_propertyStore, _tableNameWithType);
+    Preconditions.checkNotNull(schema, "Failed to find schema for table: %s", _tableNameWithType);
+    DateTimeFieldSpec dateTimeSpec = schema.getSpecForTimeColumn(_timeColumn);
+    Preconditions.checkNotNull(dateTimeSpec, "Field spec must be specified in schema for time column: %s of table: %s",
+        _timeColumn, _tableNameWithType);
+    _timeFormatSpec = new DateTimeFormatSpec(dateTimeSpec.getFormat());
+  }
+
+  @Override
+  public void init(ExternalView externalView, IdealState idealState, Set<String> onlineSegments) {
+    // Bulk load time info for all online segments
+    int numSegments = onlineSegments.size();
+    List<String> segments = new ArrayList<>(numSegments);
+    List<String> segmentZKMetadataPaths = new ArrayList<>(numSegments);
+    for (String segment : segments) {

Review comment:
       `segments` is always going to be empty here. Shouldn't this be `onlineSegments` ?
   The call on line 104 will always result in index out of bounds.
   Am I missing something here? Shouldn't the tests have caught this?




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