Jackie-Jiang commented on code in PR #15526:
URL: https://github.com/apache/pinot/pull/15526#discussion_r2100960553
##########
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:
Any specific reason why we want to reduce the scale of this test?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/JsonMatchFilterOperator.java:
##########
@@ -41,18 +42,36 @@ public class JsonMatchFilterOperator extends
BaseFilterOperator {
private final JsonIndexReader _jsonIndex;
private final JsonMatchPredicate _predicate;
+ private final FilterContext _filterContext;
- public JsonMatchFilterOperator(JsonIndexReader jsonIndex, JsonMatchPredicate
predicate,
- int numDocs) {
+ /**
+ * Constructor that takes a Json Predicate
+ */
+ public JsonMatchFilterOperator(JsonIndexReader jsonIndex, JsonMatchPredicate
predicate, int numDocs) {
super(numDocs, false);
_jsonIndex = jsonIndex;
_predicate = predicate;
+ _filterContext = null;
+ }
+
+ /**
+ * Constructor that takes a FilterContext
+ */
+ public JsonMatchFilterOperator(JsonIndexReader jsonIndex, FilterContext
filterContext, int numDocs) {
+ super(numDocs, false);
+ _jsonIndex = jsonIndex;
+ _filterContext = filterContext;
+ _predicate = null;
}
@Override
protected BlockDocIdSet getTrues() {
- ImmutableRoaringBitmap bitmap =
- _jsonIndex.getMatchingDocIds(_predicate.getValue(),
_predicate.getCountPredicate());
+ ImmutableRoaringBitmap bitmap;
+ if (_predicate != null) {
+ bitmap = _jsonIndex.getMatchingDocIds(_predicate.getValue(),
_predicate.getCountPredicate());
+ } else {
+ bitmap = _jsonIndex.getMatchingDocIds(_filterContext);
+ }
Review Comment:
(minor) Create a helper method for this
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/MapFieldTypeTest.java:
##########
@@ -242,9 +282,172 @@ public void testQueries(boolean useMultiStageQueryEngine)
}
}
+ @Test(dataProvider = "useBothQueryEngines")
+ public void testNotEqPredicate(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+
+ // Test NOT_EQ predicate with string map
+ String query = "SELECT stringMap['k2'] FROM " + getTableName() + " WHERE
stringMap['k1'] != 'v_25'";
+ JsonNode pinotResponse = postQuery(query);
+ assertEquals(pinotResponse.get("exceptions").size(), 0);
+ JsonNode rows = pinotResponse.get("resultTable").get("rows");
+ // All records except the one with k1 = 'v_25' should be returned
+ // Verify that none of the returned rows have k1 = 'v_25'
+ for (int i = 0; i < rows.size(); i++) {
+ assertNotEquals(rows.get(i).get(0).textValue(), "v_25");
+ }
+
+ // Test NOT_EQ predicate with int map
+ query = "SELECT intMap['k2'] FROM " + getTableName() + " WHERE
intMap['k1'] != 25";
+ pinotResponse = postQuery(query);
+ assertEquals(pinotResponse.get("exceptions").size(), 0);
+ rows = pinotResponse.get("resultTable").get("rows");
+ // All records except the one with k1 = 25 should be returned
+ // Verify that none of the returned rows have k1 = 25
+ for (int i = 0; i < rows.size(); i++) {
+ assertNotEquals(rows.get(i).get(0).textValue(), "v_25");
+ }
+
+ // Test NOT_EQ predicate with non-existing key
+ query = "SELECT stringMap['k2'] FROM " + getTableName() + " WHERE
stringMap['kk'] != 'v_25'";
+ pinotResponse = postQuery(query);
+ assertEquals(pinotResponse.get("exceptions").size(), 0);
+ rows = pinotResponse.get("resultTable").get("rows");
+ // All records should be returned since the key doesn't exist
+ // assertEquals(rows.size(), 0);
+ }
+
+ @Test(dataProvider = "useBothQueryEngines")
+ public void testInPredicate(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+
+ // Test IN predicate with string map
+ String query = "SELECT stringMap['k2'] FROM " + getTableName() + " WHERE
stringMap['k1'] IN ('v_25', 'v_26')";
+ JsonNode pinotResponse = postQuery(query);
+ assertEquals(pinotResponse.get("exceptions").size(), 0);
+ JsonNode rows = pinotResponse.get("resultTable").get("rows");
+ // Only records with k1 = 'v_25' or 'v_26' should be returned
+ assertEquals(rows.size(), 2);
+
+ // Verify the returned values
+ for (int i = 0; i < rows.size(); i++) {
+ String value = rows.get(i).get(0).textValue();
+ assert (value.equals("v_25") || value.equals("v_26"));
+ }
+
+ // Test IN predicate with int map
+ query = "SELECT intMap['k2'] FROM " + getTableName() + " WHERE
intMap['k1'] IN (25, 26)";
+ pinotResponse = postQuery(query);
+ assertEquals(pinotResponse.get("exceptions").size(), 0);
+ rows = pinotResponse.get("resultTable").get("rows");
+ // Only records with k1 = 25 or 26 should be returned
+ assertEquals(rows.size(), 2);
+
+ // Verify the returned values
+ for (int i = 0; i < rows.size(); i++) {
+ int value = rows.get(i).get(0).intValue();
+ assert (value == 25 || value == 26);
+ }
+
+ // Test IN predicate with non-existing key
+ query = "SELECT stringMap['k2'] FROM " + getTableName() + " WHERE
stringMap['kk'] IN ('v_25', 'v_26')";
+ pinotResponse = postQuery(query);
+ assertEquals(pinotResponse.get("exceptions").size(), 0);
+ rows = pinotResponse.get("resultTable").get("rows");
+ // No records should be returned since the key doesn't exist
+ assertEquals(rows.size(), 0);
+ }
+
+ @Test(dataProvider = "useBothQueryEngines")
+ public void testNotInPredicate(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+
+ // Test NOT IN predicate with string map
+ String query = "SELECT stringMap['k2'] FROM " + getTableName() + " WHERE
stringMap['k1'] NOT IN ('v_25', 'v_26')";
+ JsonNode pinotResponse = postQuery(query);
+ assertEquals(pinotResponse.get("exceptions").size(), 0);
+ JsonNode rows = pinotResponse.get("resultTable").get("rows");
+
+ // Verify the returned values
+ for (int i = 0; i < rows.size(); i++) {
+ String value = rows.get(i).get(0).textValue();
+ assert (!value.equals("v_25") && !value.equals("v_26"));
+ }
+
+ // Test NOT IN predicate with int map
+ query = "SELECT intMap['k2'] FROM " + getTableName() + " WHERE
intMap['k1'] NOT IN (25, 26)";
+ pinotResponse = postQuery(query);
+ assertEquals(pinotResponse.get("exceptions").size(), 0);
+ rows = pinotResponse.get("resultTable").get("rows");
+
+ // Verify the returned values
+ for (int i = 0; i < rows.size(); i++) {
+ int value = rows.get(i).get(0).intValue();
+ assert (value != 25 && value != 26);
+ }
+ }
+
+ @Test(dataProvider = "useBothQueryEngines")
+ public void testIsNullPredicate(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+
+ // Test IS_NULL predicate with string map
+ String query = "SELECT stringMap['k2'] FROM " + getTableName() + " WHERE
stringMap['k1'] IS NULL";
+ JsonNode pinotResponse = postQuery(query);
+ assertEquals(pinotResponse.get("exceptions").size(), 0);
+ JsonNode rows = pinotResponse.get("resultTable").get("rows");
+ assertEquals(rows.size(), 0);
+
+ // Test IS_NULL predicate with non-existing key
+ query = "SELECT stringMap['k2'] FROM " + getTableName() + " WHERE
stringMap['kk'] IS NULL";
+ pinotResponse = postQuery(query);
+ assertEquals(pinotResponse.get("exceptions").size(), 0);
+ rows = pinotResponse.get("resultTable").get("rows");
+ assertEquals(rows.size(), 0);
+
+ // Test IS_NOT_NULL predicate with string map
+ query = "SELECT stringMap['k2'] FROM " + getTableName() + " WHERE
stringMap['k1'] IS NOT NULL";
+ pinotResponse = postQuery(query);
+ assertEquals(pinotResponse.get("exceptions").size(), 0);
+ rows = pinotResponse.get("resultTable").get("rows");
+ // All records should be returned since all records have k1 defined
+ if (useMultiStageQueryEngine) {
+ assertEquals(rows.size(), getSelectionDefaultDocCount());
+ } else {
+ //First Two rows are null for k1
+ assertEquals(rows.size(), getSelectionDefaultDocCount());
+ }
+ }
+
+ @Test(dataProvider = "useBothQueryEngines")
+ public void testStringWithQuotes(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+
+ // Test string with single quote in map value
+ String query = "SELECT stringMap['k2'] FROM " + getTableName() + " WHERE
stringMap['k1'] = 'v_25''s value'";
+ JsonNode pinotResponse = postQuery(query);
+ assertEquals(pinotResponse.get("exceptions").size(), 0);
+
+ // Test string with multiple single quotes
+ query = "SELECT stringMap['k2'] FROM " + getTableName() + " WHERE
stringMap['k1'] = 'v_25''s ''quoted'' value'";
+ pinotResponse = postQuery(query);
+ assertEquals(pinotResponse.get("exceptions").size(), 0);
+
+ // Test IN predicate with quoted strings
+ query = "SELECT stringMap['k2'] FROM " + getTableName()
+ + " WHERE stringMap['k1'] IN ('v_25''s value', 'v_26''s value')";
+ pinotResponse = postQuery(query);
+ assertEquals(pinotResponse.get("exceptions").size(), 0);
+ }
+
@Override
protected void setUseMultiStageQueryEngine(boolean useMultiStageQueryEngine)
{
super.setUseMultiStageQueryEngine(useMultiStageQueryEngine);
- _setSelectionDefaultDocCount = useMultiStageQueryEngine ? 1000 : 10;
+ _setSelectionDefaultDocCount = useMultiStageQueryEngine ? 100 : 10;
Review Comment:
Use constant (`NUM_DOCS`) instead of magic number
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -234,6 +305,12 @@ public DictIdCompressionType getDictIdCompressionType() {
return _dictIdCompressionType;
}
+ @JsonIgnore
Review Comment:
Why ignoring it?
##########
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:
Given `MapIndexType` is deleted, consider removing all related classes and
methods
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/MapFieldTypeTest.java:
##########
@@ -84,9 +87,50 @@ public Schema createSchema() {
@Override
public TableConfig createOfflineTableConfig() {
- IngestionConfig ingestionConfig = new IngestionConfig();
- return new
TableConfigBuilder(TableType.OFFLINE).setTableName(getTableName()).setIngestionConfig(ingestionConfig)
- .build();
+ // Create table config with field configs
+ TableConfig config =
+ new
TableConfigBuilder(TableType.OFFLINE).setTableName(getTableName()).setIngestionConfig(new
IngestionConfig())
+ .build();
+ // Get indexing config
+ List<FieldConfig> fieldConfigList = createFieldConfigs();
+
+ config.setFieldConfigList(fieldConfigList);
+ LOGGER.info("Table config: {}", config);
Review Comment:
(minor) We don't usually log in test
##########
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:
Is `MapIndexReader` still in use?
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -97,7 +98,10 @@ public static ForwardIndexConfig getDisabled() {
private final ChunkCompressionType _chunkCompressionType;
@Nullable
private final DictIdCompressionType _dictIdCompressionType;
+ @Nullable
+ private final Map<String, Object> _configs;
+ @Deprecated
public ForwardIndexConfig(@Nullable Boolean disabled, @Nullable
CompressionCodec compressionCodec,
Review Comment:
Just make it call the new constructor. No need to duplicate the code
##########
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:
(minor) Store `context.getFieldSpec().getDataType().getStoredType()` in a
local variable, same for other repeated access
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/MapFieldTypeTest.java:
##########
@@ -84,9 +87,50 @@ public Schema createSchema() {
@Override
public TableConfig createOfflineTableConfig() {
- IngestionConfig ingestionConfig = new IngestionConfig();
- return new
TableConfigBuilder(TableType.OFFLINE).setTableName(getTableName()).setIngestionConfig(ingestionConfig)
- .build();
+ // Create table config with field configs
+ TableConfig config =
+ new
TableConfigBuilder(TableType.OFFLINE).setTableName(getTableName()).setIngestionConfig(new
IngestionConfig())
+ .build();
+ // Get indexing config
+ List<FieldConfig> fieldConfigList = createFieldConfigs();
+
+ config.setFieldConfigList(fieldConfigList);
Review Comment:
```suggestion
TableConfig config = new
TableConfigBuilder(TableType.OFFLINE).setTableName(getTableName())
.setFieldConfigList(createFieldConfigs())
.build();
```
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -110,7 +114,60 @@ public ForwardIndexConfig(@Nullable Boolean disabled,
@Nullable CompressionCodec
_targetMaxChunkSizeBytes =
targetMaxChunkSize == null ? _defaultTargetMaxChunkSizeBytes : (int)
DataSizeUtils.toBytes(targetMaxChunkSize);
_targetDocsPerChunk = targetDocsPerChunk == null ?
_defaultTargetDocsPerChunk : targetDocsPerChunk;
+ if (compressionCodec != null) {
+ switch (compressionCodec) {
+ case PASS_THROUGH:
+ case CLP:
+ case CLPV2:
+ case CLPV2_ZSTD:
+ case CLPV2_LZ4:
+ _chunkCompressionType = ChunkCompressionType.PASS_THROUGH;
+ _dictIdCompressionType = null;
+ break;
+ case SNAPPY:
+ _chunkCompressionType = ChunkCompressionType.SNAPPY;
+ _dictIdCompressionType = null;
+ break;
+ case ZSTANDARD:
+ _chunkCompressionType = ChunkCompressionType.ZSTANDARD;
+ _dictIdCompressionType = null;
+ break;
+ case LZ4:
+ _chunkCompressionType = ChunkCompressionType.LZ4;
+ _dictIdCompressionType = null;
+ break;
+ case GZIP:
+ _chunkCompressionType = ChunkCompressionType.GZIP;
+ _dictIdCompressionType = null;
+ break;
+ case MV_ENTRY_DICT:
+ _dictIdCompressionType = DictIdCompressionType.MV_ENTRY_DICT;
+ _chunkCompressionType = null;
+ break;
+ default:
+ throw new IllegalStateException("Unsupported compression codec: " +
compressionCodec);
+ }
+ } else {
+ _dictIdCompressionType = null;
+ _chunkCompressionType = null;
+ }
+ _configs = null;
+ }
+ public ForwardIndexConfig(@Nullable Boolean disabled, @Nullable
CompressionCodec compressionCodec,
+ @Nullable Boolean deriveNumDocsPerChunk, @Nullable Integer
rawIndexWriterVersion,
+ @Nullable String targetMaxChunkSize, @Nullable Integer
targetDocsPerChunk,
+ @JsonProperty("configs") @Nullable Map<String, Object> configs) {
Review Comment:
No need to annotate `@JsonProperty`
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -150,7 +207,7 @@ public ForwardIndexConfig(@Nullable Boolean disabled,
@Nullable CompressionCodec
}
}
- @JsonCreator
+ @Deprecated
Review Comment:
I think you can directly remove this constructor. If you want to keep it,
given it is no longer than `@JsonCreator`, remove all `@JsonProperty`
--
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]
