Jackie-Jiang commented on code in PR #19440:
URL: https://github.com/apache/pinot/pull/19440#discussion_r3919882205
##########
pinot-core/src/main/java/org/apache/pinot/core/startree/StarTreeUtils.java:
##########
@@ -88,9 +89,14 @@ public static AggregationFunctionColumnPair[]
extractAggregationFunctionPairs(
/// the list are implicitly ANDed together. Any OR and NOT predicates are
nested within a CompositePredicate.
///
/// A map from predicates to their evaluators is passed in to accelerate the
computation.
+ ///
+ /// A predicate that is always true over a column's values is left out of
the map, unless null handling is enabled
+ /// and the column holds nulls: a null row is UNKNOWN rather than true, so
the predicate is kept, and the column stays
+ /// visible to the null checks that decide whether a star-tree can serve the
query.
@Nullable
public static Map<String, List<CompositePredicateEvaluator>>
extractPredicateEvaluatorsMap(IndexSegment indexSegment,
Review Comment:
Adding the required `nullHandlingEnabled` argument removes the existing
public three-argument method, breaking compiled and source callers. Please keep
a three-argument overload that delegates with the legacy two-valued behavior,
while this overload carries the explicit null-handling contract.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/predicate/PredicateEvaluatorProvider.java:
##########
@@ -103,7 +103,7 @@ private static PredicateEvaluator buildEvaluator(Predicate
predicate, @Nullable
queryContext);
}
default:
- throw new UnsupportedOperationException("Unsupported predicate
type: " + predicate.getType());
+ throw new IllegalStateException("Unsupported predicate type: " +
predicate.getType());
Review Comment:
This exception-type change is unrelated to the three-valued filter shortcut
fix and is not needed by its tests. Please drop it from this PR or move it to a
separate cleanup with its own rationale.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/SortedIndexBasedFilterOperator.java:
##########
@@ -137,6 +137,9 @@ public boolean canOptimizeCount() {
@Override
public int getNumMatchingDocs() {
+ if (getNullBitmap() != null) {
Review Comment:
Any non-empty null vector now sends `getNumMatchingDocs()` through
`getBitmaps()`. For sorted indexes this replaces range arithmetic with a newly
materialized `MutableRoaringBitmap`; multi-value `IN` on single-value inverted
indexes similarly loses the disjoint-bitmap summation fast path. Could we
subtract the relevant null cardinality (or use intersection cardinality)
without materializing all matches? A nullable sorted/inverted filtered-COUNT
benchmark would help guard this path.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/AndFilterOperator.java:
##########
@@ -101,6 +102,16 @@ protected BlockDocIdSet getFalses() {
return new NotDocIdSet(new AndDocIdSet(blockDocIdSets, _queryOptions),
_numDocs);
}
+ /// A conjunction is UNKNOWN where it is neither true nor false: where no
child is false and some child is UNKNOWN.
+ @Override
+ protected BlockDocIdSet getNulls() {
+ if (!mayHaveNulls()) {
+ return EmptyDocIdSet.getInstance();
+ }
+ return new AndDocIdSet(
Review Comment:
`getNulls()` independently calls `getTrues()` and `getFalses()`, while
`getFalses()` walks each child's trues and nulls again; `OrFilterOperator` has
the same pattern. In nested mixed AND/OR trees this can create multiple
independent scan doc sets for the same leaves and repeat work as nesting grows.
Can we cache/reuse each child's ternary result or derive true/false/unknown in
one traversal?
--
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]
