You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "shenyu0127 (via GitHub)" <gi...@apache.org> on 2023/07/29 22:55:14 UTC
[GitHub] [pinot] shenyu0127 opened a new pull request, #11220: ExpressionFilterOperator NULL support.
shenyu0127 opened a new pull request, #11220:
URL: https://github.com/apache/pinot/pull/11220
This PR enables ExpressionFilterOperator NULL support.
We do not override `getNulls` because `getNulls` is called only by `BaseFilterOperator::getFalses` and we override the `getFalses`.
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #11220: ExpressionFilterOperator NULL support.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11220:
URL: https://github.com/apache/pinot/pull/11220#discussion_r1281201506
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/ExpressionScanDocIdIterator.java:
##########
@@ -85,7 +88,7 @@ public int next() {
ProjectionBlock projectionBlock =
new ProjectionOperator(_dataSourceMap, new RangeDocIdSetOperator(blockStartDocId, _blockEndDocId))
.nextBlock();
- RoaringBitmap matchingDocIds = new RoaringBitmap();
+ MutableRoaringBitmap matchingDocIds = new MutableRoaringBitmap();
Review Comment:
`RoaringBitmap` performs better than `MutableRoaringBitmap`. We should change all of them to use `RoaringBitmap`
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/ExpressionScanDocIdIterator.java:
##########
@@ -212,8 +215,27 @@ private void processProjectionBlock(ProjectionBlock projectionBlock, BitmapDataP
default:
throw new IllegalStateException();
}
+ if (_nullHandlingEnabled) {
+ RoaringBitmap nullBitmap = _transformFunction.getNullBitmap(projectionBlock);
+ if (_predicateEvaluationResult == PredicateEvaluationResult.TRUE) {
+ if (nullBitmap != null) {
Review Comment:
Also check if it is empty?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/ExpressionScanDocIdIterator.java:
##########
@@ -212,8 +215,27 @@ private void processProjectionBlock(ProjectionBlock projectionBlock, BitmapDataP
default:
throw new IllegalStateException();
}
+ if (_nullHandlingEnabled) {
+ RoaringBitmap nullBitmap = _transformFunction.getNullBitmap(projectionBlock);
Review Comment:
(MAJOR) What is the offset of this bitmap? Is this the docId within the projection block?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/ExpressionScanDocIdIterator.java:
##########
@@ -212,8 +215,27 @@ private void processProjectionBlock(ProjectionBlock projectionBlock, BitmapDataP
default:
throw new IllegalStateException();
}
+ if (_nullHandlingEnabled) {
+ RoaringBitmap nullBitmap = _transformFunction.getNullBitmap(projectionBlock);
+ if (_predicateEvaluationResult == PredicateEvaluationResult.TRUE) {
+ if (nullBitmap != null) {
+ RoaringBitmap fullBitmap = new RoaringBitmap();
+ fullBitmap.add(0L, (long) numDocs);
+ nullBitmap.xor(fullBitmap);
+ matchingDocIds.and(nullBitmap.toMutableRoaringBitmap());
Review Comment:
It is the same as `andNot()`
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #11220: ExpressionFilterOperator NULL support.
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11220:
URL: https://github.com/apache/pinot/pull/11220#issuecomment-1656957729
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11220?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11220](https://app.codecov.io/gh/apache/pinot/pull/11220?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (ce540ae) into [master](https://app.codecov.io/gh/apache/pinot/commit/834c9707e81dc6b40660f6eb0737f5ca3293a2e2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (834c970) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11220 +/- ##
=========================================
Coverage 0.11% 0.11%
=========================================
Files 2227 2172 -55
Lines 119628 117089 -2539
Branches 18102 17793 -309
=========================================
Hits 137 137
+ Misses 119471 116932 -2539
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration1temurin20 | `?` | |
| integration2temurin11 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `0.11% <0.00%> (-0.01%)` | :arrow_down: |
| unittests2temurin17 | `?` | |
| unittests2temurin20 | `?` | |
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.
| [Files Changed](https://app.codecov.io/gh/apache/pinot/pull/11220?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...or/dociditerators/ExpressionScanDocIdIterator.java](https://app.codecov.io/gh/apache/pinot/pull/11220?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9FeHByZXNzaW9uU2NhbkRvY0lkSXRlcmF0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ot/core/operator/docidsets/ExpressionDocIdSet.java](https://app.codecov.io/gh/apache/pinot/pull/11220?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZHNldHMvRXhwcmVzc2lvbkRvY0lkU2V0LmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...core/operator/filter/ExpressionFilterOperator.java](https://app.codecov.io/gh/apache/pinot/pull/11220?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvRXhwcmVzc2lvbkZpbHRlck9wZXJhdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | |
... and [57 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11220/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [pinot] shenyu0127 commented on a diff in pull request #11220: ExpressionFilterOperator NULL support.
Posted by "shenyu0127 (via GitHub)" <gi...@apache.org>.
shenyu0127 commented on code in PR #11220:
URL: https://github.com/apache/pinot/pull/11220#discussion_r1281246684
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/ExpressionScanDocIdIterator.java:
##########
@@ -212,8 +215,27 @@ private void processProjectionBlock(ProjectionBlock projectionBlock, BitmapDataP
default:
throw new IllegalStateException();
}
+ if (_nullHandlingEnabled) {
+ RoaringBitmap nullBitmap = _transformFunction.getNullBitmap(projectionBlock);
+ if (_predicateEvaluationResult == PredicateEvaluationResult.TRUE) {
+ if (nullBitmap != null) {
Review Comment:
This comment no longer applies. I now have an empty check in the `isNull` function.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/ExpressionScanDocIdIterator.java:
##########
@@ -85,7 +88,7 @@ public int next() {
ProjectionBlock projectionBlock =
new ProjectionOperator(_dataSourceMap, new RangeDocIdSetOperator(blockStartDocId, _blockEndDocId))
.nextBlock();
- RoaringBitmap matchingDocIds = new RoaringBitmap();
+ MutableRoaringBitmap matchingDocIds = new MutableRoaringBitmap();
Review Comment:
Reverted the change.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/ExpressionScanDocIdIterator.java:
##########
@@ -212,8 +215,27 @@ private void processProjectionBlock(ProjectionBlock projectionBlock, BitmapDataP
default:
throw new IllegalStateException();
}
+ if (_nullHandlingEnabled) {
+ RoaringBitmap nullBitmap = _transformFunction.getNullBitmap(projectionBlock);
Review Comment:
Good point. Fixed the bug.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/ExpressionScanDocIdIterator.java:
##########
@@ -212,8 +215,27 @@ private void processProjectionBlock(ProjectionBlock projectionBlock, BitmapDataP
default:
throw new IllegalStateException();
}
+ if (_nullHandlingEnabled) {
+ RoaringBitmap nullBitmap = _transformFunction.getNullBitmap(projectionBlock);
+ if (_predicateEvaluationResult == PredicateEvaluationResult.TRUE) {
+ if (nullBitmap != null) {
+ RoaringBitmap fullBitmap = new RoaringBitmap();
+ fullBitmap.add(0L, (long) numDocs);
+ nullBitmap.xor(fullBitmap);
+ matchingDocIds.and(nullBitmap.toMutableRoaringBitmap());
Review Comment:
This comment no longer applies.
--
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
[GitHub] [pinot] Jackie-Jiang merged pull request #11220: ExpressionFilterOperator NULL support.
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #11220:
URL: https://github.com/apache/pinot/pull/11220
--
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