Jackie-Jiang commented on code in PR #19440:
URL: https://github.com/apache/pinot/pull/19440#discussion_r3922158149
##########
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:
Done. The sorted operator keeps its range arithmetic and subtracts the null
rows per merged range with `rangeCardinality`; the single-value inverted
operator keeps the per-posting summation and subtracts one `andCardinality` per
posting. Both go through a shared `toNumTrueDocs` that handles the exclusive
case (`numDocs - matches - (nulls outside the matches)`). Multi-value inverted
still takes the union it already needed, and the range index and bitmap leaves
were already a single cardinality call on a bitmap they hold. I did not add a
benchmark: with this the null path has the same shape as the no-null path plus
one cardinality per range or posting.
##########
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:
Restructured to a single traversal: each child is asked once for
`getTrues()` and once for `getNulls()`, and the same sets feed both the
not-false and all-true sides (AND) or the any-null and any-true sides (OR),
with the same empty / match-all short-circuits as `getFalses()`. Iterating the
same leaf set twice when it appears in two composite sets is inherent to how
`getFalses()` already works; memoizing leaf results per operator would be a
broader change than this PR, and this path is only reached for a boolean nested
under another boolean under a negation.
##########
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:
Dropped from this PR.
##########
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:
Not added. There is no caller of the three-argument form in this repo, and
none in StarTree either. A delegating overload would silently keep the old
two-valued answer for a caller running with null handling on, which is exactly
the bug this PR fixes, so I'd rather a compile error point them at the new
argument. Failing the binary-compatibility check on this is accepted.
--
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]
