Skip to content

Commit

Permalink
Continue treating IS NULL as an exclusive predicate
Browse files Browse the repository at this point in the history
  • Loading branch information
@yashmayya
yashmayya committed May 16, 2024
1 parent fc6493f commit c6c54a6
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 65 deletions.
70 changes: 39 additions & 31 deletions ...src/main/java/org/apache/pinot/segment/local/realtime/impl/json/MutableJsonIndexImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,16 +135,34 @@ public MutableRoaringBitmap getMatchingDocIds(String filterString) {

_readLock.lock();
try {
RoaringBitmap matchingFlattenedDocIds = getMatchingFlattenedDocIds(filter);
MutableRoaringBitmap matchingDocIds = new MutableRoaringBitmap();
matchingFlattenedDocIds.forEach(
(IntConsumer) flattenedDocId -> matchingDocIds.add(_docIdMapping.getInt(flattenedDocId)));
return matchingDocIds;
if (filter.getType() == FilterContext.Type.PREDICATE && isExclusive(filter.getPredicate().getType())) {
// Handle exclusive predicate separately because the flip can only be applied to the unflattened doc ids in
// order to get the correct result, and it cannot be nested
RoaringBitmap matchingFlattenedDocIds = getMatchingFlattenedDocIds(filter.getPredicate());
MutableRoaringBitmap matchingDocIds = new MutableRoaringBitmap();
matchingFlattenedDocIds.forEach(
(IntConsumer) flattenedDocId -> matchingDocIds.add(_docIdMapping.getInt(flattenedDocId)));
matchingDocIds.flip(0, (long) _nextDocId);
return matchingDocIds;
} else {
RoaringBitmap matchingFlattenedDocIds = getMatchingFlattenedDocIds(filter);
MutableRoaringBitmap matchingDocIds = new MutableRoaringBitmap();
matchingFlattenedDocIds.forEach(
(IntConsumer) flattenedDocId -> matchingDocIds.add(_docIdMapping.getInt(flattenedDocId)));
return matchingDocIds;
}
} finally {
_readLock.unlock();
}
}

/**
* Returns {@code true} if the given predicate type is exclusive for json_match calculation, {@code false} otherwise.
*/
private boolean isExclusive(Predicate.Type predicateType) {
return predicateType == Predicate.Type.IS_NULL;
}

/**
* Returns the matching flattened doc ids for the given filter.
*/
Expand All @@ -169,7 +187,10 @@ private RoaringBitmap getMatchingFlattenedDocIds(FilterContext filter) {
return matchingDocIds;
}
case PREDICATE: {
return getMatchingFlattenedDocIds(filter.getPredicate());
Predicate predicate = filter.getPredicate();
Preconditions.checkArgument(!isExclusive(predicate.getType()), "Exclusive predicate: %s cannot be nested",
predicate);
return getMatchingFlattenedDocIds(predicate);
}
default:
throw new IllegalStateException();
Expand All @@ -178,6 +199,8 @@ private RoaringBitmap getMatchingFlattenedDocIds(FilterContext filter) {

/**
* Returns the matching flattened doc ids for the given predicate.
* <p>Exclusive predicate is handled as the inclusive predicate, and the caller should flip the unflattened doc ids in
* order to get the correct exclusive predicate result.
*/
private RoaringBitmap getMatchingFlattenedDocIds(Predicate predicate) {
ExpressionContext lhs = predicate.getLhs();
Expand Down Expand Up @@ -299,7 +322,8 @@ private RoaringBitmap getMatchingFlattenedDocIds(Predicate predicate) {
}
}

case IS_NOT_NULL: {
case IS_NOT_NULL:
case IS_NULL: {
RoaringBitmap matchingDocIdsForKey = _postingListMap.get(key);
if (matchingDocIdsForKey != null) {
if (matchingDocIds == null) {
Expand All @@ -313,29 +337,6 @@ private RoaringBitmap getMatchingFlattenedDocIds(Predicate predicate) {
}
}

case IS_NULL: {
RoaringBitmap matchingDocIdsForKey = _postingListMap.get(key);

if (matchingDocIdsForKey != null) {
MutableRoaringBitmap result = matchingDocIdsForKey.toMutableRoaringBitmap();
result.flip(0, (long) _nextFlattenedDocId);
if (matchingDocIds == null) {
return result.toRoaringBitmap();
} else {
matchingDocIds.and(result.toRoaringBitmap());
return matchingDocIds;
}
} else {
if (matchingDocIds != null) {
return matchingDocIds;
} else {
MutableRoaringBitmap result = new MutableRoaringBitmap();
result.flip(0, (long) _nextFlattenedDocId);
return result.toRoaringBitmap();
}
}
}

case REGEXP_LIKE: {
Map<String, RoaringBitmap> subMap = getMatchingKeysMap(key);
if (subMap.isEmpty()) {
Expand Down Expand Up @@ -447,7 +448,14 @@ public Map<String, RoaringBitmap> getMatchingFlattenedDocsMap(String jsonPathKey
if (filterString != null) {
filter = RequestContextUtils.getFilter(CalciteSqlParser.compileToExpression(filterString));
Preconditions.checkArgument(!filter.isConstant(), "Invalid json match filter: " + filterString);
filteredFlattenedDocIds = getMatchingFlattenedDocIds(filter);
if (filter.getType() == FilterContext.Type.PREDICATE && isExclusive(filter.getPredicate().getType())) {
// Handle exclusive predicate separately because the flip can only be applied to the
// unflattened doc ids in order to get the correct result, and it cannot be nested
filteredFlattenedDocIds = getMatchingFlattenedDocIds(filter.getPredicate());
filteredFlattenedDocIds.flip(0, (long) _nextFlattenedDocId);
} else {
filteredFlattenedDocIds = getMatchingFlattenedDocIds(filter);
}
}
// Support 2 formats:
// - JSONPath format (e.g. "$.a[1].b"='abc', "$[0]"=1, "$"='abc')
Expand Down
67 changes: 37 additions & 30 deletions ...a/org/apache/pinot/segment/local/segment/index/readers/json/ImmutableJsonIndexReader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,27 @@ public MutableRoaringBitmap getMatchingDocIds(String filterString) {
throw new BadQueryRequestException("Invalid json match filter: " + filterString);
}

MutableRoaringBitmap matchingFlattenedDocIds = getMatchingFlattenedDocIds(filter);
MutableRoaringBitmap matchingDocIds = new MutableRoaringBitmap();
matchingFlattenedDocIds.forEach((IntConsumer) flattenedDocId -> matchingDocIds.add(getDocId(flattenedDocId)));
return matchingDocIds;
if (filter.getType() == FilterContext.Type.PREDICATE && isExclusive(filter.getPredicate().getType())) {
// Handle exclusive predicate separately because the flip can only be applied to the unflattened doc ids in order
// to get the correct result, and it cannot be nested
MutableRoaringBitmap matchingFlattenedDocIds = getMatchingFlattenedDocIds(filter.getPredicate());
MutableRoaringBitmap matchingDocIds = new MutableRoaringBitmap();
matchingFlattenedDocIds.forEach((IntConsumer) flattenedDocId -> matchingDocIds.add(getDocId(flattenedDocId)));
matchingDocIds.flip(0, _numDocs);
return matchingDocIds;
} else {
MutableRoaringBitmap matchingFlattenedDocIds = getMatchingFlattenedDocIds(filter);
MutableRoaringBitmap matchingDocIds = new MutableRoaringBitmap();
matchingFlattenedDocIds.forEach((IntConsumer) flattenedDocId -> matchingDocIds.add(getDocId(flattenedDocId)));
return matchingDocIds;
}
}

/**
* Returns {@code true} if the given predicate type is exclusive for json_match calculation, {@code false} otherwise.
*/
private boolean isExclusive(Predicate.Type predicateType) {
return predicateType == Predicate.Type.IS_NULL;
}

/**
Expand Down Expand Up @@ -135,7 +152,10 @@ private MutableRoaringBitmap getMatchingFlattenedDocIds(FilterContext filter) {
return matchingDocIds;
}
case PREDICATE: {
return getMatchingFlattenedDocIds(filter.getPredicate());
Predicate predicate = filter.getPredicate();
Preconditions
.checkArgument(!isExclusive(predicate.getType()), "Exclusive predicate: %s cannot be nested", predicate);
return getMatchingFlattenedDocIds(predicate);
}
default:
throw new IllegalStateException();
Expand All @@ -144,6 +164,8 @@ private MutableRoaringBitmap getMatchingFlattenedDocIds(FilterContext filter) {

/**
* Returns the matching flattened doc ids for the given predicate.
* <p>Exclusive predicate is handled as the inclusive predicate, and the caller should flip the unflattened doc ids in
* order to get the correct exclusive predicate result.
*/
private MutableRoaringBitmap getMatchingFlattenedDocIds(Predicate predicate) {
ExpressionContext lhs = predicate.getLhs();
Expand Down Expand Up @@ -266,7 +288,8 @@ private MutableRoaringBitmap getMatchingFlattenedDocIds(Predicate predicate) {
}
}

case IS_NOT_NULL: {
case IS_NOT_NULL:
case IS_NULL: {
int dictId = _dictionary.indexOf(key);
if (dictId >= 0) {
ImmutableRoaringBitmap matchingDocIdsForKey = _invertedIndex.getDocIds(dictId);
Expand All @@ -281,29 +304,6 @@ private MutableRoaringBitmap getMatchingFlattenedDocIds(Predicate predicate) {
}
}

case IS_NULL: {
int dictId = _dictionary.indexOf(key);
if (dictId >= 0) {
MutableRoaringBitmap matchingDocIdsForKey = _invertedIndex.getDocIds(dictId).toMutableRoaringBitmap();
matchingDocIdsForKey.flip(0, _numFlattenedDocs);

if (matchingDocIds == null) {
matchingDocIds = matchingDocIdsForKey;
} else {
matchingDocIds.and(matchingDocIdsForKey);
}
return matchingDocIds;
} else {
if (matchingDocIds != null) {
return matchingDocIds;
} else {
MutableRoaringBitmap result = new MutableRoaringBitmap();
result.flip(0, _numFlattenedDocs);
return result;
}
}
}

case REGEXP_LIKE: {
Pattern pattern = ((RegexpLikePredicate) predicate).getPattern();
int[] dictIds = getDictIdRangeForKey(key);
Expand Down Expand Up @@ -409,7 +409,14 @@ public Map<String, RoaringBitmap> getMatchingFlattenedDocsMap(String jsonPathKey
} catch (Exception e) {
throw new BadQueryRequestException("Invalid json match filter: " + filterString);
}
filteredFlattenedDocIds = getMatchingFlattenedDocIds(filter).toRoaringBitmap();
if (filter.getType() == FilterContext.Type.PREDICATE && isExclusive(filter.getPredicate().getType())) {
// Handle exclusive predicate separately because the flip can only be applied to the
// unflattened doc ids in order to get the correct result, and it cannot be nested
filteredFlattenedDocIds = getMatchingFlattenedDocIds(filter.getPredicate()).toRoaringBitmap();
filteredFlattenedDocIds.flip(0, _numFlattenedDocs);
} else {
filteredFlattenedDocIds = getMatchingFlattenedDocIds(filter).toRoaringBitmap();
}
}
// Support 2 formats:
// - JSONPath format (e.g. "$.a[1].b"='abc', "$[0]"=1, "$"='abc')
Expand Down
5 changes: 1 addition & 4 deletions ...gment-local/src/test/java/org/apache/pinot/segment/local/segment/index/JsonIndexTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,14 +178,11 @@ public void testSmallIndex()
Assert.assertEquals(matchingDocIds.toArray(), new int[0]);

matchingDocIds = getMatchingDocIds(indexReader, "\"addresses[*].types[*]\" IS NULL");
Assert.assertEquals(matchingDocIds.toArray(), new int[]{0, 1, 2, 3});
Assert.assertEquals(matchingDocIds.toArray(), new int[]{0, 1, 2});

matchingDocIds = getMatchingDocIds(indexReader, "\"addresses[*].types[*]\" IS NOT NULL");
Assert.assertEquals(matchingDocIds.toArray(), new int[]{3});

matchingDocIds = getMatchingDocIds(indexReader, "\"addresses[1].types[*]\" IS NULL");
Assert.assertEquals(matchingDocIds.toArray(), new int[]{0, 1, 2, 3});

matchingDocIds = getMatchingDocIds(indexReader, "\"addresses[1].types[*]\" IS NOT NULL");
Assert.assertEquals(matchingDocIds.toArray(), new int[0]);

Expand Down

0 comments on commit c6c54a6

Please sign in to comment.