You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2026/09/02 23:57:30 UTC
[PR] Make filter count and bitmap shortcuts three-valued under null handling [pinot]
Jackie-Jiang opened a new pull request, #19440:
URL: https://github.com/apache/pinot/pull/19440
## Summary
With null handling enabled, a null row is UNKNOWN to a predicate on its column: neither the predicate nor its negation selects it. Several filter paths were still two-valued and let such rows through. All of them are fixed here by making the bitmap shortcut layer three-valued rather than by declining shortcuts, so no null-handling query gets slower and the null-handling-disabled path is unchanged.
### What was wrong
- **Always-true / always-false predicates.** An evaluator's verdict is over the dictionary's real values. The leaf collapsed to `MatchAllFilterOperator` / `EmptyFilterOperator` (or a two-valued bitmap), so `c <> 99999` selected the null rows and `NOT (c = 99999)` selected them too. `StarTreeUtils` dropped such predicates from its map, so the column escaped the null check that decides whether a star-tree may serve the query, and the star-tree answered with the null row folded into the column's default value.
- **Index-based shortcuts.** `InvertedIndexFilterOperator`, `SortedIndexBasedFilterOperator` and `RangeIndexBasedFilterOperator` computed `getNumMatchingDocs()` and `getBitmaps()` from postings alone, while their `getTrues()` excluded the null vector. `COUNT(*)` on its own takes the fast filtered count whenever the filter can count, so `SELECT COUNT(*) FROM t WHERE c <> 1` counted the null rows and `WHERE c = <default>` matched them. Index-based `DISTINCT` and the vector pre-filter consumed the same bitmaps.
- **Negation and composition.** `NotFilterOperator` counted `numDocs - child` and inverted the child's bitmaps as if no document were UNKNOWN, and NOT / AND / OR reported no UNKNOWN documents through `getNulls()`, so a nested boolean tree under a negation treated UNKNOWN rows as false on the iterator path.
### What changes
- `BitmapCollection` gains an optional null bitmap via `excludingNulls`. True documents are the union (or its complement when inverted) minus the nulls; `invert()` keeps the null bitmap since NOT of UNKNOWN is UNKNOWN; `reduce()`, `andCardinality`, `orCardinality` and the new `getCardinality()` honor it. The two-valued fast paths are untouched when no null bitmap is present.
- `BaseColumnFilterOperator` reads the column's null bitmap once at construction, only when null handling is enabled and the vector is non-empty. `getTrues()`, `getNulls()`, `mayHaveNulls()` and the leaves' shortcuts share it, so a consuming segment's vector (which hands out a copy per read) is copied once and every view of the operator sees the same rows.
- The inverted, sorted and range leaves and `BitmapBasedFilterOperator` attach that bitmap to their collection and count through it when nulls exist. The no-null path is the previous code.
- `BaseFilterOperator.mayHaveNulls()` is a metadata-only, possibly pessimistic answer. `NotFilterOperator` counts through the child's inverted bitmaps only when the child may have nulls, and propagates its child's `getNulls()`. `AndFilterOperator` / `OrFilterOperator` compute their own null bitmap when a child has one and implement `getNulls()` as "neither true nor false".
- `FilterOperatorUtils.getLeafFilterOperator` builds a three-valued `BitmapBasedFilterOperator` for an always-true / always-false predicate on a column with nulls, instead of a constant. `StarTreeUtils` keeps such predicates in its map so the null check sees the column. `FilterOperatorUtils.getNullBitmap` / `hasNulls` are the shared helpers.
- `PredicateEvaluatorProvider` throws `IllegalStateException` rather than `UnsupportedOperationException` for an unsupported predicate type.
### Behavior change
Query answers change only with null handling enabled, and only where they were wrong: counts, distinct values and star-tree aggregates no longer include null rows that the predicate or its negation cannot select. Leaves that never model nulls (text, JSON, H3, vector) are unchanged.
### Tests
`BitmapCollectionTest` covers the null cases including inversion. `NotFilterOperatorTest`, `AndFilterOperatorTest` and `OrFilterOperatorTest` check count, bitmaps, `getNulls()` and the iterator path agree under nulls, including nested trees under a negation. `InvertedIndexFilterOperatorTest` mocks a null vector on both sides of exclusive. `NullHandlingEnabledQueriesTest` runs `COUNT(*)` with `<>`, equality on the stored default, `NOT`, `IN` and ranges over inverted, sorted and range-indexed columns, a multi-value column, a consuming segment, and index-based `DISTINCT`. `StarTreeNullHandlingQueriesTest` checks the star-tree is refused for always-true / negated always-false predicates and OR clauses on a nullable column and kept on a non-nullable one.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make filter count and bitmap shortcuts three-valued under null handling [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #19440:
URL: https://github.com/apache/pinot/pull/19440#discussion_r3927838442
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/BitmapCollection.java:
##########
@@ -26,30 +27,75 @@
/// Encapsulates a collection of bitmaps, and allows inversion without modifying the bitmaps.
/// Provides simplified access to efficient cardinality calculation which work regardless of
/// inversion status without computing the complement of the union of the bitmaps.
+///
+/// The collection is two-valued by default: the true documents are the union of the bitmaps, or its complement when
+/// inverted, and every other document is false. [#excludingNulls] attaches the documents the predicate is UNKNOWN
+/// for, which are then neither true nor false: they are left out of the true documents whether or not they fall in
+/// the bitmaps, and they stay UNKNOWN under inversion, since NOT of UNKNOWN is UNKNOWN. Every cardinality and
+/// reduction honors the null bitmap.
public class BitmapCollection {
private final int _numDocs;
private boolean _inverted;
private final ImmutableRoaringBitmap[] _bitmaps;
+ @Nullable
+ private final ImmutableRoaringBitmap _nullBitmap;
public BitmapCollection(int numDocs, boolean inverted, ImmutableRoaringBitmap... bitmaps) {
+ this(numDocs, inverted, bitmaps, null);
+ }
+
+ private BitmapCollection(int numDocs, boolean inverted, ImmutableRoaringBitmap[] bitmaps,
+ @Nullable ImmutableRoaringBitmap nullBitmap) {
_numDocs = numDocs;
_inverted = inverted;
_bitmaps = bitmaps;
+ _nullBitmap = nullBitmap;
+ }
+
+ /// Returns a collection over the same bitmaps that treats the documents in `nullBitmap` as UNKNOWN, or this
+ /// collection when `nullBitmap` is `null` or empty.
+ public BitmapCollection excludingNulls(@Nullable ImmutableRoaringBitmap nullBitmap) {
+ if (nullBitmap == null || nullBitmap.isEmpty()) {
+ return this;
+ }
+ return new BitmapCollection(_numDocs, _inverted, _bitmaps, nullBitmap);
+ }
+
+ /// Returns the documents the predicate is UNKNOWN for, or `null` when there is none.
+ @Nullable
+ public ImmutableRoaringBitmap getNullBitmap() {
+ return _nullBitmap;
}
- /// Inverts the bitmaps in constant time and space.
+ /// Inverts the bitmaps in constant time and space. The null bitmap is kept: NOT of UNKNOWN is UNKNOWN.
/// @return this bitmap collection inverted.
public BitmapCollection invert() {
_inverted = !_inverted;
return this;
}
+ /// Returns the number of true documents.
+ public int getCardinality() {
+ ImmutableRoaringBitmap union = reduceInternal();
+ if (_nullBitmap == null) {
+ return _inverted ? _numDocs - union.getCardinality() : union.getCardinality();
+ }
+ if (_inverted) {
+ return _numDocs - ImmutableRoaringBitmap.orCardinality(union, _nullBitmap);
+ }
+ return union.getCardinality() - ImmutableRoaringBitmap.andCardinality(union, _nullBitmap);
+ }
+
/// Computes the size of the intersection of the bitmaps efficiently regardless of negation, without
- /// needing to invert inputs or materialize an intermediate bitmap.
+ /// needing to invert inputs or materialize an intermediate bitmap. When either collection has a null bitmap, the
+ /// true documents of both are materialized instead.
///
/// @param bitmaps to intersect with
/// @return the size of the intersection of the bitmaps in this collection and in the other collection
public int andCardinality(BitmapCollection bitmaps) {
+ if (_nullBitmap != null || bitmaps._nullBitmap != null) {
+ return ImmutableRoaringBitmap.andCardinality(reduce(), bitmaps.reduce());
Review Comment:
Done. `andCardinality` and `orCardinality` no longer reduce. The intersection is the existing inversion-aware formula minus the intersection restricted to the union of the two null bitmaps, and that restricted term is computed through bitmaps no larger than the null union (`and(X, N)` then one cardinality call, four inversion cases in `andCardinalityWithin`). The union size follows from the two cardinalities and the intersection. The no-null path is byte-for-byte unchanged. `testCardinalitiesMatchMaterializedTrues` brute-forces every inversion / null combination on both sides against the materialized true sets.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RangeIndexBasedFilterOperator.java:
##########
@@ -184,6 +184,9 @@ public boolean canOptimizeCount() {
@Override
public int getNumMatchingDocs() {
+ if (getNullBitmap() != null) {
Review Comment:
Done. The range operator keeps `_rangeIndexReader.getNumMatchingDocs(...)` and subtracts the null vector's cardinality when the column's default null value satisfies the predicate, evaluated once via `applySV` on the default's dictionary id (or the typed default for raw columns). The Javadoc states the invariant this rests on: a null row is stored under the default null value, so the null rows are all inside or all outside the matched set. The query test now runs over both a dictionary and a raw range-indexed column so both branches are exercised.
##########
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:
Correction to the above: a `BlockDocIdSet` is single-use (`AndDocIdSet` / `OrDocIdSet` drop their children after the first `iterator()`), so the AND cannot feed one true-set into both intersections; it asks a child for its trues a second time when both are needed. The latest push also replaces the hand-built "trues plus nulls" set in the default `getFalses()` and both composite `getFalses()` with one `BaseFilterOperator.getNotFalses()`, which the AND `getNulls()` reuses.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make filter count and bitmap shortcuts three-valued under null handling [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19440:
URL: https://github.com/apache/pinot/pull/19440#issuecomment-5675778913
Documentation follow-up: https://github.com/pinot-contrib/pinot-docs/pull/1044 clarifies three-valued filtering with null handling enabled, including indexed shortcuts, filtered COUNT(*), negation, and the star-tree boundary.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make filter count and bitmap shortcuts three-valued under null handling [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #19440:
URL: https://github.com/apache/pinot/pull/19440
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make filter count and bitmap shortcuts three-valued under null handling [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make filter count and bitmap shortcuts three-valued under null handling [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #19440:
URL: https://github.com/apache/pinot/pull/19440#discussion_r3922223498
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/BitmapCollection.java:
##########
@@ -26,30 +27,75 @@
/// Encapsulates a collection of bitmaps, and allows inversion without modifying the bitmaps.
/// Provides simplified access to efficient cardinality calculation which work regardless of
/// inversion status without computing the complement of the union of the bitmaps.
+///
+/// The collection is two-valued by default: the true documents are the union of the bitmaps, or its complement when
+/// inverted, and every other document is false. [#excludingNulls] attaches the documents the predicate is UNKNOWN
+/// for, which are then neither true nor false: they are left out of the true documents whether or not they fall in
+/// the bitmaps, and they stay UNKNOWN under inversion, since NOT of UNKNOWN is UNKNOWN. Every cardinality and
+/// reduction honors the null bitmap.
public class BitmapCollection {
private final int _numDocs;
private boolean _inverted;
private final ImmutableRoaringBitmap[] _bitmaps;
+ @Nullable
+ private final ImmutableRoaringBitmap _nullBitmap;
public BitmapCollection(int numDocs, boolean inverted, ImmutableRoaringBitmap... bitmaps) {
+ this(numDocs, inverted, bitmaps, null);
+ }
+
+ private BitmapCollection(int numDocs, boolean inverted, ImmutableRoaringBitmap[] bitmaps,
+ @Nullable ImmutableRoaringBitmap nullBitmap) {
_numDocs = numDocs;
_inverted = inverted;
_bitmaps = bitmaps;
+ _nullBitmap = nullBitmap;
+ }
+
+ /// Returns a collection over the same bitmaps that treats the documents in `nullBitmap` as UNKNOWN, or this
+ /// collection when `nullBitmap` is `null` or empty.
+ public BitmapCollection excludingNulls(@Nullable ImmutableRoaringBitmap nullBitmap) {
+ if (nullBitmap == null || nullBitmap.isEmpty()) {
+ return this;
+ }
+ return new BitmapCollection(_numDocs, _inverted, _bitmaps, nullBitmap);
+ }
+
+ /// Returns the documents the predicate is UNKNOWN for, or `null` when there is none.
+ @Nullable
+ public ImmutableRoaringBitmap getNullBitmap() {
+ return _nullBitmap;
}
- /// Inverts the bitmaps in constant time and space.
+ /// Inverts the bitmaps in constant time and space. The null bitmap is kept: NOT of UNKNOWN is UNKNOWN.
/// @return this bitmap collection inverted.
public BitmapCollection invert() {
_inverted = !_inverted;
return this;
}
+ /// Returns the number of true documents.
+ public int getCardinality() {
+ ImmutableRoaringBitmap union = reduceInternal();
+ if (_nullBitmap == null) {
+ return _inverted ? _numDocs - union.getCardinality() : union.getCardinality();
+ }
+ if (_inverted) {
+ return _numDocs - ImmutableRoaringBitmap.orCardinality(union, _nullBitmap);
+ }
+ return union.getCardinality() - ImmutableRoaringBitmap.andCardinality(union, _nullBitmap);
+ }
+
/// Computes the size of the intersection of the bitmaps efficiently regardless of negation, without
- /// needing to invert inputs or materialize an intermediate bitmap.
+ /// needing to invert inputs or materialize an intermediate bitmap. When either collection has a null bitmap, the
+ /// true documents of both are materialized instead.
///
/// @param bitmaps to intersect with
/// @return the size of the intersection of the bitmaps in this collection and in the other collection
public int andCardinality(BitmapCollection bitmaps) {
+ if (_nullBitmap != null || bitmaps._nullBitmap != null) {
+ return ImmutableRoaringBitmap.andCardinality(reduce(), bitmaps.reduce());
Review Comment:
When either collection carries nulls, `andCardinality` and `orCardinality` call `reduce()` on both sides. That materializes true-result bitmaps and can also materialize a full-segment complement for an inverted input, replacing the existing cardinality-only shortcut with one or two segment-sized allocations per query segment. Can we extend the inversion-aware cardinality formulas to account for each null bitmap without reducing the collections?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RangeIndexBasedFilterOperator.java:
##########
@@ -184,6 +184,9 @@ public boolean canOptimizeCount() {
@Override
public int getNumMatchingDocs() {
+ if (getNullBitmap() != null) {
Review Comment:
A non-empty null vector redirects every exact range-index count through `getBitmaps()`, which materializes the matching bitmap. This drops the range reader's cardinality-only path. Could we retain `_rangeIndexReader.getNumMatchingDocs(...)` and adjust for null rows—e.g. subtract the null-vector cardinality when the stored default value satisfies the range—similar to the updated sorted-index path?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make filter count and bitmap shortcuts three-valued under null handling [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make filter count and bitmap shortcuts three-valued under null handling [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19440:
URL: https://github.com/apache/pinot/pull/19440#issuecomment-5518492728
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19440?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `87.68116%` with `17 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.81%. Comparing base ([`d3e53e2`](https://app.codecov.io/gh/apache/pinot/commit/d3e53e202107605fb68f6e10aef6f55e25152d29?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`3a7c2ed`](https://app.codecov.io/gh/apache/pinot/commit/3a7c2ed3a660645069417202556fada48e8228a3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/19440?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../org/apache/pinot/core/startree/StarTreeUtils.java](https://app.codecov.io/gh/apache/pinot/pull/19440?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fstartree%2FStarTreeUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9TdGFyVHJlZVV0aWxzLmphdmE=) | 58.33% | [2 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19440?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...e/pinot/core/operator/filter/OrFilterOperator.java](https://app.codecov.io/gh/apache/pinot/pull/19440?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Ffilter%2FOrFilterOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvT3JGaWx0ZXJPcGVyYXRvci5qYXZh) | 84.61% | [1 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19440?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../pinot/core/operator/filter/AndFilterOperator.java](https://app.codecov.io/gh/apache/pinot/pull/19440?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Ffilter%2FAndFilterOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvQW5kRmlsdGVyT3BlcmF0b3IuamF2YQ==) | 89.65% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19440?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://app.codecov.io/gh/apache/pinot/pull/19440?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Ffilter%2FRangeIndexBasedFilterOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | 33.33% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19440?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...r/filter/predicate/PredicateEvaluatorProvider.java](https://app.codecov.io/gh/apache/pinot/pull/19440?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Ffilter%2Fpredicate%2FPredicateEvaluatorProvider.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvcHJlZGljYXRlL1ByZWRpY2F0ZUV2YWx1YXRvclByb3ZpZGVyLmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19440?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../pinot/core/operator/filter/MapFilterOperator.java](https://app.codecov.io/gh/apache/pinot/pull/19440?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Ffilter%2FMapFilterOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvTWFwRmlsdGVyT3BlcmF0b3IuamF2YQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19440?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (d3e53e2) and HEAD (3a7c2ed). Click for more details.
>
> <details><summary>HEAD has 24 uploads less than BASE</summary>
>
>| Flag | BASE (d3e53e2) | HEAD (3a7c2ed) |
>|------|------|------|
>|lane-a|2|0|
>|integration|4|0|
>|java-25|6|1|
>|integration1|2|0|
>|temurin|6|1|
>|lane-b|2|0|
>|unittests|2|1|
>|integration2|2|0|
>|unittests2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19440 +/- ##
============================================
- Coverage 67.62% 57.81% -9.82%
+ Complexity 1430 1 -1429
============================================
Files 3488 2685 -803
Lines 224390 164260 -60130
Branches 35423 26678 -8745
============================================
- Hits 151753 94971 -56782
- Misses 60607 61277 +670
+ Partials 12030 8012 -4018
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19440/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.81% <87.68%> (-9.82%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.81% <87.68%> (-9.82%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.81% <87.68%> (-9.81%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.81% <87.68%> (+0.08%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19440/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19440?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org