Jackie-Jiang commented on a change in pull request #5653:
URL: https://github.com/apache/incubator-pinot/pull/5653#discussion_r449200057
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/indexsegment/mutable/MutableSegmentImpl.java
##########
@@ -116,15 +116,14 @@
private final Map<String, InvertedIndexReader> _invertedIndexMap = new
HashMap<>();
private final Map<String, InvertedIndexReader> _rangeIndexMap = new
HashMap<>();
private final Map<String, BloomFilterReader> _bloomFilterMap = new
HashMap<>();
+ private final Map<String, Comparable> _minValueMap = new HashMap<>();
Review comment:
This has potential thread safety issue (expanding hashMap while
reading). You can prevent that by putting `null` in the constructor.
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/RealtimeClusterIntegrationTest.java
##########
@@ -19,8 +19,10 @@
package org.apache.pinot.integration.tests;
import java.io.File;
+import java.util.Arrays;
Review comment:
Revert
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/RealtimeNoDictionaryTimeColumnClusterIntegrationTest.java
##########
@@ -0,0 +1,42 @@
+/**
+ * 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.integration.tests;
+
+import java.io.File;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import org.apache.commons.io.FileUtils;
+import org.apache.pinot.util.TestUtils;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+
+/**
+ * Extends RealtimeClusterIntegrationTest and configure time column to be a
non-dictionary column.
+ */
+public class RealtimeNoDictionaryTimeColumnClusterIntegrationTest extends
RealtimeClusterIntegrationTest {
Review comment:
One way to merge these 2 tests is by using a random boolean to decide
the NoDictionaryColumns
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentColumnarIndexCreator.java
##########
@@ -491,7 +491,6 @@ public static void
addColumnMetadataInfo(PropertiesConfiguration properties, Str
String.valueOf(columnIndexCreationInfo.getTotalNumberOfEntries()));
properties.setProperty(V1Constants.MetadataKeys.Column.getKeyFor(column,
IS_AUTO_GENERATED),
String.valueOf(columnIndexCreationInfo.isAutoGenerated()));
-
Review comment:
(nit) revert?
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/index/datasource/MutableDataSource.java
##########
@@ -39,11 +39,12 @@
private static final String OPERATOR_NAME_PREFIX = "MutableDataSource:";
public MutableDataSource(FieldSpec fieldSpec, int numDocs, int numValues,
int maxNumValuesPerMVEntry,
- @Nullable PartitionFunction partitionFunction, int partitionId,
DataFileReader forwardIndex,
- @Nullable Dictionary dictionary, @Nullable InvertedIndexReader
invertedIndex, @Nullable InvertedIndexReader rangeIndex,
- @Nullable BloomFilterReader bloomFilter, @Nullable NullValueVectorReader
nullValueVector) {
+ @Nullable PartitionFunction partitionFunction, int partitionId,
Comparable minValue, Comparable maxValue,
Review comment:
Annotate them as nullable
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/segment/index/datasource/MutableDataSource.java
##########
@@ -54,9 +55,11 @@ public MutableDataSource(FieldSpec fieldSpec, int numDocs,
int numValues, int ma
final int _maxNumValuesPerMVEntry;
final PartitionFunction _partitionFunction;
final Set<Integer> _partitions;
+ final Comparable _minValue;
+ final Comparable _maxValue;
MutableDataSourceMetadata(FieldSpec fieldSpec, int numDocs, int numValues,
int maxNumValuesPerMVEntry,
- @Nullable PartitionFunction partitionFunction, int partitionId) {
+ @Nullable PartitionFunction partitionFunction, int partitionId,
Comparable minValue, Comparable maxValue) {
Review comment:
Annotate them as nullable
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/indexsegment/mutable/MutableSegmentImpl.java
##########
@@ -514,6 +518,32 @@ private void addForwardIndex(GenericRow row, int docId,
Map<String, Object> dict
numValuesInfo.updateMVEntry(dictIds.length);
}
+
+ // Update min/max value for columns
+ BaseMutableDictionary dictionary = _dictionaryMap.get(column);
+ if (dictionary != null) {
+ _minValueMap.put(column, dictionary.getMinVal());
+ _maxValueMap.put(column, dictionary.getMaxVal());
+ continue;
+ }
+ if (value == null) {
+ continue;
+ }
+ if (!(value instanceof Comparable)) {
+ continue;
+ }
+ Comparable comparableValue = (Comparable) value;
+ if (!_minValueMap.containsKey(column)) {
Review comment:
Cache `_minValueMap.get(column)` here to save 2 extra map lookups
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/indexsegment/mutable/MutableSegmentImpl.java
##########
@@ -514,6 +518,32 @@ private void addForwardIndex(GenericRow row, int docId,
Map<String, Object> dict
numValuesInfo.updateMVEntry(dictIds.length);
}
+
+ // Update min/max value for columns
+ BaseMutableDictionary dictionary = _dictionaryMap.get(column);
+ if (dictionary != null) {
+ _minValueMap.put(column, dictionary.getMinVal());
+ _maxValueMap.put(column, dictionary.getMaxVal());
+ continue;
+ }
+ if (value == null) {
Review comment:
This check is redundant (1. value cannot be null; 2. already handled in
the instanceof check)
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
