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