Jackie-Jiang commented on code in PR #15526:
URL: https://github.com/apache/pinot/pull/15526#discussion_r2101379881


##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/map/MutableMapDataSource.java:
##########
@@ -55,15 +55,13 @@ public MutableMapDataSource(FieldSpec fieldSpec, int 
numDocs, int numValues, int
             partitionFunction, partitions, minValue, maxValue, 
maxRowLengthInBytes),
         new 
ColumnIndexContainer.FromMap.Builder().withAll(mutableIndexes).build());
     _mutableIndexes = mutableIndexes;
-    MapIndexReader mapIndexReader = getMapIndex();
-    if (mapIndexReader == null) {
-      // Fallback to use forward index
-      ForwardIndexReader<?> forwardIndex = getForwardIndex();
-      if (forwardIndex instanceof MapIndexReader) {
-        mapIndexReader = (MapIndexReader) forwardIndex;
-      } else {
-        mapIndexReader = new MapIndexReaderWrapper(forwardIndex, 
getFieldSpec());
-      }
+    MapIndexReader mapIndexReader;
+    // Fallback to use forward index
+    ForwardIndexReader<?> forwardIndex = getForwardIndex();
+    if (forwardIndex instanceof MapIndexReader) {
+      mapIndexReader = (MapIndexReader) forwardIndex;
+    } else {
+      mapIndexReader = new MapIndexReaderWrapper(forwardIndex, getFieldSpec());

Review Comment:
   Given map index is forward index, do we still need this wrapper?



##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/MapFieldTypeTest.java:
##########
@@ -38,14 +40,15 @@
 import org.testng.annotations.Test;
 
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotEquals;
 
 
 @Test(suiteName = "CustomClusterIntegrationTest")
 public class MapFieldTypeTest extends CustomDataQueryClusterIntegrationTest {
 
   // Default settings
   protected static final String DEFAULT_TABLE_NAME = "MapFieldTypeTest";
-  private static final int NUM_DOCS = 1000;
+  private static final int NUM_DOCS = 100;

Review Comment:
   How long does it take to process 1000 docs? If we observe long testing time 
for only 1000 docs, the performance is not acceptable



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/json/JsonIndexType.java:
##########
@@ -95,7 +95,8 @@ public JsonIndexCreator 
createIndexCreator(IndexCreationContext context, JsonInd
       throws IOException {
     Preconditions.checkState(context.getFieldSpec().isSingleValueField(),
         "Json index is currently only supported on single-value columns");
-    
Preconditions.checkState(context.getFieldSpec().getDataType().getStoredType() 
== FieldSpec.DataType.STRING,
+    
Preconditions.checkState(context.getFieldSpec().getDataType().getStoredType() 
== FieldSpec.DataType.STRING

Review Comment:
   ^^



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/datasource/BaseDataSource.java:
##########
@@ -127,6 +127,7 @@ public VectorIndexReader getVectorIndex() {
     return getIndex(StandardIndexes.vector());
   }
 
+  @Deprecated

Review Comment:
   ^^



##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/map/ImmutableMapDataSource.java:
##########
@@ -41,15 +41,12 @@ public class ImmutableMapDataSource extends 
BaseMapDataSource {
 
   public ImmutableMapDataSource(ColumnMetadata columnMetadata, 
ColumnIndexContainer columnIndexContainer) {
     super(new ImmutableMapDataSourceMetadata(columnMetadata), 
columnIndexContainer);
-    MapIndexReader mapIndexReader = getMapIndex();
-    if (mapIndexReader == null) {
-      // Fallback to use forward index
-      ForwardIndexReader<?> forwardIndex = getForwardIndex();
-      if (forwardIndex instanceof MapIndexReader) {
-        mapIndexReader = (MapIndexReader) forwardIndex;
-      } else {
-        mapIndexReader = new MapIndexReaderWrapper(forwardIndex, 
getFieldSpec());
-      }
+    MapIndexReader mapIndexReader;
+    ForwardIndexReader<?> forwardIndex = getForwardIndex();
+    if (forwardIndex instanceof MapIndexReader) {

Review Comment:
   I don't see any usage of `ImmutableMapIndexReader` or `MutableMapIndex`, 
thus wondering how are they wired up



-- 
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-unsubscr...@pinot.apache.org

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

Reply via email to