You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/05/04 20:11:34 UTC
[GitHub] [incubator-pinot] Jackie-Jiang opened a new pull request #5328: Fix the bug introduced in #5132 where ScanBasedDocIdIterator.isMatch() is still required
Jackie-Jiang opened a new pull request #5328:
URL: https://github.com/apache/incubator-pinot/pull/5328
ScanBasedDocIdIterator.isMatch() is still required when the query is in the following format:
SELECT ... WHERE (filterA OR filterB) AND filterC
And filterC is working on a scan-based column (column without inverted index)
Enhance the HybridClusterIntegrationTest to catch this issue
----------------------------------------------------------------
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.
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] [incubator-pinot] codecov-io commented on pull request #5328: Fix the bug introduced in #5132 where ScanBasedDocIdIterator.isMatch() is still required
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #5328:
URL: https://github.com/apache/incubator-pinot/pull/5328#issuecomment-623726703
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5328?src=pr&el=h1) Report
> Merging [#5328](https://codecov.io/gh/apache/incubator-pinot/pull/5328?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/c87be271bfe4c923b59f4e68b85a51e518112e54&el=desc) will **decrease** coverage by `9.39%`.
> The diff coverage is `1.78%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/5328?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #5328 +/- ##
==========================================
- Coverage 66.21% 56.82% -9.40%
==========================================
Files 1072 1072
Lines 54552 54578 +26
Branches 8137 8141 +4
==========================================
- Hits 36123 31012 -5111
- Misses 15801 21130 +5329
+ Partials 2628 2436 -192
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5328?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...core/operator/dociditerators/AndDocIdIterator.java](https://codecov.io/gh/apache/incubator-pinot/pull/5328/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9BbmREb2NJZEl0ZXJhdG9yLmphdmE=) | `54.38% <0.00%> (-10.32%)` | :arrow_down: |
| [...or/dociditerators/ExpressionScanDocIdIterator.java](https://codecov.io/gh/apache/incubator-pinot/pull/5328/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9FeHByZXNzaW9uU2NhbkRvY0lkSXRlcmF0b3IuamF2YQ==) | `0.00% <0.00%> (-49.70%)` | :arrow_down: |
| [...e/operator/dociditerators/SVScanDocIdIterator.java](https://codecov.io/gh/apache/incubator-pinot/pull/5328/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9TVlNjYW5Eb2NJZEl0ZXJhdG9yLmphdmE=) | `49.42% <0.00%> (-20.09%)` | :arrow_down: |
| [...e/operator/dociditerators/MVScanDocIdIterator.java](https://codecov.io/gh/apache/incubator-pinot/pull/5328/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9kb2NpZGl0ZXJhdG9ycy9NVlNjYW5Eb2NJZEl0ZXJhdG9yLmphdmE=) | `40.62% <14.28%> (-28.87%)` | :arrow_down: |
| [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5328/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/5328/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/5328/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...he/pinot/core/query/reduce/ComparisonFunction.java](https://codecov.io/gh/apache/incubator-pinot/pull/5328/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvQ29tcGFyaXNvbkZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/5328/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ot/minion/events/EventObserverFactoryRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/5328/diff?src=pr&el=tree#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXZlbnRzL0V2ZW50T2JzZXJ2ZXJGYWN0b3J5UmVnaXN0cnkuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [323 more](https://codecov.io/gh/apache/incubator-pinot/pull/5328/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5328?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5328?src=pr&el=footer). Last update [edef309...6cdf280](https://codecov.io/gh/apache/incubator-pinot/pull/5328?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5328: Fix the bug introduced in #5132 where ScanBasedDocIdIterator.isMatch() is still required
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5328:
URL: https://github.com/apache/incubator-pinot/pull/5328#discussion_r419900186
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java
##########
@@ -137,6 +137,9 @@ public void testHardcodedQueries()
query = "SELECT MAX(ArrTime), MIN(ArrTime) FROM mytable WHERE DaysSinceEpoch >= 16312";
testQuery(query, Arrays.asList("SELECT MAX(ArrTime) FROM mytable WHERE DaysSinceEpoch >= 15312",
"SELECT MIN(ArrTime) FROM mytable WHERE DaysSinceEpoch >= 15312"));
+ query =
+ "SELECT SUM(TotalAddGTime) FROM mytable WHERE DivArrDelay NOT IN (67, 260) AND Carrier IN ('F9', 'B6') OR DepTime BETWEEN 2144 AND 1926";
Review comment:
@jackjlli Yes, for hybrid use case, there will be an implicit time filter appended. This is the query that failed before.
Do we have an issue opened for this?
----------------------------------------------------------------
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.
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] [incubator-pinot] jackjlli commented on a change in pull request #5328: Fix the bug introduced in #5132 where ScanBasedDocIdIterator.isMatch() is still required
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #5328:
URL: https://github.com/apache/incubator-pinot/pull/5328#discussion_r419862270
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java
##########
@@ -137,6 +137,9 @@ public void testHardcodedQueries()
query = "SELECT MAX(ArrTime), MIN(ArrTime) FROM mytable WHERE DaysSinceEpoch >= 16312";
testQuery(query, Arrays.asList("SELECT MAX(ArrTime) FROM mytable WHERE DaysSinceEpoch >= 15312",
"SELECT MIN(ArrTime) FROM mytable WHERE DaysSinceEpoch >= 15312"));
+ query =
+ "SELECT SUM(TotalAddGTime) FROM mytable WHERE DivArrDelay NOT IN (67, 260) AND Carrier IN ('F9', 'B6') OR DepTime BETWEEN 2144 AND 1926";
Review comment:
Is it the test case mentioned in the description section? It doesn't seem matched with `SELECT ... WHERE (filterA OR filterB) AND filterC` though.
----------------------------------------------------------------
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.
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] [incubator-pinot] mcvsubbu commented on a change in pull request #5328: Fix the bug introduced in #5132 where ScanBasedDocIdIterator.isMatch() is still required
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5328:
URL: https://github.com/apache/incubator-pinot/pull/5328#discussion_r419747204
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/AndDocIdIterator.java
##########
@@ -102,6 +102,17 @@ public int next() {
i = -1;
}
}
+ if (hasScanBasedIterators && i == docIdIterators.length - 1) {
+ // this means we found the docId common to all nonScanBased iterators, now we need to ensure
+ // that its also found in scanBasedIterator, if not matched, we restart the intersection
+ for (ScanBasedDocIdIterator iterator : scanBasedDocIdIterators) {
+ if (!iterator.isMatch(currentMax)) {
+ i = -1;
Review comment:
Can you restructure the (outer) loop so that it is a while with some condition rather than a `for(i = ..)` in which we change the loop counter? I understand it was already that way.
----------------------------------------------------------------
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.
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] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5328: Fix the bug introduced in #5132 where ScanBasedDocIdIterator.isMatch() is still required
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5328:
URL: https://github.com/apache/incubator-pinot/pull/5328#discussion_r419758565
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/dociditerators/AndDocIdIterator.java
##########
@@ -102,6 +102,17 @@ public int next() {
i = -1;
}
}
+ if (hasScanBasedIterators && i == docIdIterators.length - 1) {
+ // this means we found the docId common to all nonScanBased iterators, now we need to ensure
+ // that its also found in scanBasedIterator, if not matched, we restart the intersection
+ for (ScanBasedDocIdIterator iterator : scanBasedDocIdIterators) {
+ if (!iterator.isMatch(currentMax)) {
+ i = -1;
Review comment:
@mcvsubbu Planning to do some clean-up in the following pr. Wanted to keep this pr small and focus on the bug fix.
----------------------------------------------------------------
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.
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