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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
