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 2021/09/15 10:50:46 UTC
[GitHub] [pinot] richardstartin opened a new pull request #7435: refactor RangeIndex to make it easier to evolve its implementation
richardstartin opened a new pull request #7435:
URL: https://github.com/apache/pinot/pull/7435
## Description
This PR refactors the interface between the `RangeIndexBasedFilterOperator` and `RangeIndexReader`, the latter of which I plan to submit a proposal to enhance. This work is being done in advance of submitting the enhancement proposal to limit the amount of change necessary.
The pending proposal for a new range index will support fast exact matches without the need to scan, and this PR tries to make it possible to model both exact and inexact indexing with the same interface, allowing the `RangeIndexReaderImpl` to decide the implementation to use based on the version in the index header.
/cc @Jackie-Jiang @mayankshriv
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
<!-- If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release. -->
<!-- If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text.
-->
## Documentation
<!-- If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
-->
--
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] richardstartin commented on a change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r710451160
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/creator/RangeIndexCreatorTest.java
##########
@@ -35,11 +37,12 @@
import org.testng.annotations.Test;
import static org.apache.pinot.segment.spi.V1Constants.Indexes.BITMAP_RANGE_INDEX_FILE_EXTENSION;
+import static org.testng.Assert.*;
public class RangeIndexCreatorTest {
private static final File INDEX_DIR = new File(FileUtils.getTempDirectory(), "RangeIndexCreatorTest");
- private static final Random RANDOM = new Random();
+ private static final Random RANDOM = new Random(42);
Review comment:
tests should be deterministic
--
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 edited a comment on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-919947162
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7435](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (14cf0d9) into [master](https://codecov.io/gh/apache/pinot/commit/ad2e0632fa8d8ab84316d25ce2cd4c91c18b11bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ad2e063) will **decrease** coverage by `2.06%`.
> The diff coverage is `49.27%`.
[](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7435 +/- ##
============================================
- Coverage 71.90% 69.84% -2.07%
+ Complexity 3429 3285 -144
============================================
Files 1517 1123 -394
Lines 75118 53143 -21975
Branches 10950 8012 -2938
============================================
- Hits 54013 37116 -16897
+ Misses 17470 13397 -4073
+ Partials 3635 2630 -1005
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `69.84% <49.27%> (+0.12%)` | :arrow_up: |
| unittests2 | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `0.00% <0.00%> (-44.69%)` | :arrow_down: |
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `58.26% <ø> (ø)` | |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `88.88% <0.00%> (ø)` | |
| [...local/segment/index/datasource/BaseDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvQmFzZURhdGFTb3VyY2UuamF2YQ==) | `95.83% <ø> (ø)` | |
| [...al/segment/index/datasource/MutableDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvTXV0YWJsZURhdGFTb3VyY2UuamF2YQ==) | `69.23% <ø> (ø)` | |
| [...ent/virtualcolumn/VirtualColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `90.47% <ø> (ø)` | |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `87.00% <91.42%> (ø)` | |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `80.63% <100.00%> (+0.08%)` | :arrow_up: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [601 more](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ad2e063...14cf0d9](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] richardstartin commented on a change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r710463113
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/creator/RangeIndexCreatorTest.java
##########
@@ -35,11 +37,12 @@
import org.testng.annotations.Test;
import static org.apache.pinot.segment.spi.V1Constants.Indexes.BITMAP_RANGE_INDEX_FILE_EXTENSION;
+import static org.testng.Assert.*;
public class RangeIndexCreatorTest {
private static final File INDEX_DIR = new File(FileUtils.getTempDirectory(), "RangeIndexCreatorTest");
- private static final Random RANDOM = new Random();
+ private static final Random RANDOM = new Random(42);
Review comment:
https://martinfowler.com/articles/nonDeterminism.html
--
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] klsince commented on a change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
klsince commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r710406660
##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/RangeIndexReader.java
##########
@@ -0,0 +1,100 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.spi.index.reader;
+
+import java.io.Closeable;
+import javax.annotation.Nullable;
+
+/**
+ * Interface for indexed range queries
+ * @param <T>
+ */
+public interface RangeIndexReader<T> extends Closeable {
+ /**
+ * Returns doc ids with a value between min and max, both inclusive.
+ * @param min the inclusive lower bound.
+ * @param max the inclusive upper bound.
+ * @return the matching doc ids.
+ */
+ @Nullable
+ T getMatchingDocIds(int min, int max);
+
+ /**
+ * Returns doc ids with a value between min and max, both inclusive.
+ * @param min the inclusive lower bound.
+ * @param max the inclusive upper bound.
+ * @return the matching doc ids.
+ */
+ @Nullable
+ T getMatchingDocIds(long min, long max);
+
+ /**
+ * Returns doc ids with a value between min and max, both inclusive.
+ * @param min the inclusive lower bound.
+ * @param max the inclusive upper bound.
+ * @return the matching doc ids.
+ */
+ @Nullable
+ T getMatchingDocIds(float min, float max);
+
+ /**
+ * Returns doc ids with a value between min and max, both inclusive.
+ * @param min the inclusive lower bound.
+ * @param max the inclusive upper bound.
+ * @return the matching doc ids.
+ */
+ @Nullable
+ T getMatchingDocIds(double min, double max);
+
+ /**
+ * Returns doc ids with a value between min and max, both inclusive.
+ * @param min the inclusive lower bound.
+ * @param max the inclusive upper bound.
+ * @return the matching doc ids.
+ */
+ @Nullable
Review comment:
looks like the comments are identical for getMatchingDocIds() and getPartiallyMatchingDocIds(). I assume their behavior would be different. If so, perhaps highlight it in the 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.
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 pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-920464640
> The way I see it, this feature is buggy:
>
> * it converted floats to ints
> * it doesn't produce evenly sized ranges
> * it doesn't handle or test rangeIds of -1, which are ambiguous and can mean smaller than the smallest range _or_ larger than the largest range, and `RangeIndexReader.getDocIds` never knew the difference.
>
> If this feature is in use, these bugs are probably acceptable to users. What I'm trying to do here is move some code around so I can eventually replace the feature, I don't want to fix every problem with the existing implementation in the process of doing so.
I don't agree that we should leave the bug as is just because no one complains. It is hard for user to realize the result returned is inaccurate.
It is okay to not fix it in the refactoring PR, but we should at least add a TODO and create an issue to track it so that it can be fixed in a following PR.
Also, it shouldn't be hard to fix:
- Convert float to int - already fixed in this PR
- Not even sized range - not optimal but won't cause wrong result
- RangeId of -1 - We can change the upper bound id to Integer.MAX_VALUE to differentiate them and handle them properly
--
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] richardstartin commented on a change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r709631911
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/RangeIndexReaderImpl.java
##########
@@ -217,4 +272,26 @@ public void close() {
// NOTE: DO NOT close the PinotDataBuffer here because it is tracked by the caller and might be reused later. The
// caller is responsible of closing the PinotDataBuffer.
}
+
+ private ImmutableRoaringBitmap getMatchesInRange(int firstRangeId, int lastRangeId) {
+ if (firstRangeId == lastRangeId) {
+ return null;
+ }
+ MutableRoaringBitmap matching = new MutableRoaringBitmap();
+ for (int rangeId = firstRangeId + 1; rangeId < lastRangeId; ++rangeId) {
Review comment:
Actually, Looking back at `findRangeId`, it is ambiguous. -1 can mean smaller than the lowest range or greater than the highest range. So this was always broken.
This code was lifted straight from `RangeIndexBasedFilterOperator`:
```java
// Ranges in the middle of first and last range are fully matched
for (int rangeId = firstRangeId + 1; rangeId < lastRangeId; rangeId++) {
docIds.or(rangeIndexReader.getDocIds(rangeId));
}
```
I don't want existing bugs/specification flaws to get in the way of redefining this functionality.
--
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 edited a comment on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-919947162
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7435](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (117c064) into [master](https://codecov.io/gh/apache/pinot/commit/ad2e0632fa8d8ab84316d25ce2cd4c91c18b11bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ad2e063) will **decrease** coverage by `41.43%`.
> The diff coverage is `22.38%`.
> :exclamation: Current head 117c064 differs from pull request most recent head 8110c14. Consider uploading reports for the commit 8110c14 to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7435 +/- ##
=============================================
- Coverage 71.90% 30.47% -41.44%
+ Complexity 3429 88 -3341
=============================================
Files 1517 1508 -9
Lines 75118 74800 -318
Branches 10950 10917 -33
=============================================
- Hits 54013 22794 -31219
- Misses 17470 49999 +32529
+ Partials 3635 2007 -1628
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `30.47% <22.38%> (+0.08%)` | :arrow_up: |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `0.00% <ø> (-58.27%)` | :arrow_down: |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `0.00% <0.00%> (-80.55%)` | :arrow_down: |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
| [...local/segment/index/datasource/BaseDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvQmFzZURhdGFTb3VyY2UuamF2YQ==) | `0.00% <ø> (-95.84%)` | :arrow_down: |
| [...al/segment/index/datasource/MutableDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvTXV0YWJsZURhdGFTb3VyY2UuamF2YQ==) | `0.00% <ø> (-69.24%)` | :arrow_down: |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ent/virtualcolumn/VirtualColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `0.00% <ø> (-90.48%)` | :arrow_down: |
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `35.71% <36.58%> (-8.97%)` | :arrow_down: |
| [...c/main/java/org/apache/pinot/common/tier/Tier.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1045 more](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ad2e063...8110c14](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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 #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-919947162
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7435](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8fa6d9e) into [master](https://codecov.io/gh/apache/pinot/commit/f3068bc93165e359cc3d846c40f62312ecc98af3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f3068bc) will **decrease** coverage by `42.74%`.
> The diff coverage is `19.73%`.
> :exclamation: Current head 8fa6d9e differs from pull request most recent head 2b93f0e. Consider uploading reports for the commit 2b93f0e to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7435 +/- ##
=============================================
- Coverage 71.86% 29.11% -42.75%
+ Complexity 3346 85 -3261
=============================================
Files 1519 1508 -11
Lines 75115 74796 -319
Branches 10941 10914 -27
=============================================
- Hits 53981 21778 -32203
- Misses 17504 51046 +33542
+ Partials 3630 1972 -1658
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `29.11% <19.73%> (-0.02%)` | :arrow_down: |
| unittests1 | `?` | |
| unittests2 | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `66.25% <0.00%> (-0.69%)` | :arrow_down: |
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `0.00% <ø> (-58.27%)` | :arrow_down: |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `0.00% <0.00%> (-80.55%)` | :arrow_down: |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
| [...local/segment/index/datasource/BaseDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvQmFzZURhdGFTb3VyY2UuamF2YQ==) | `0.00% <ø> (-95.84%)` | :arrow_down: |
| [...al/segment/index/datasource/MutableDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvTXV0YWJsZURhdGFTb3VyY2UuamF2YQ==) | `0.00% <ø> (-69.24%)` | :arrow_down: |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ent/virtualcolumn/VirtualColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `0.00% <ø> (-90.48%)` | :arrow_down: |
| [...nt/spi/creator/name/FixedSegmentNameGenerator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2NyZWF0b3IvbmFtZS9GaXhlZFNlZ21lbnROYW1lR2VuZXJhdG9yLmphdmE=) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [...eator/name/NormalizedDateSegmentNameGenerator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2NyZWF0b3IvbmFtZS9Ob3JtYWxpemVkRGF0ZVNlZ21lbnROYW1lR2VuZXJhdG9yLmphdmE=) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
| ... and [1063 more](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f3068bc...2b93f0e](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] richardstartin commented on a change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r709221040
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/inv/RangeIndexCreator.java
##########
@@ -539,7 +543,7 @@ public int compare(Number val1, Number val2) {
@Override
public void put(int position, Number value) {
- _dataBuffer.putFloat(position << 2, value.intValue());
+ _dataBuffer.putFloat(position << 2, value.floatValue());
Review comment:
This fixes #7436
--
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 #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #7435:
URL: https://github.com/apache/pinot/pull/7435
--
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] richardstartin commented on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-920452119
I will apply the requested code style changes, but let's look into setting up check style rules to enforce them in another PR, as it's a waste of everybody's time to enforce style guidelines manually during code review.
--
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] richardstartin commented on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-920451358
The way I see it, this feature is buggy:
* it converted floats to ints
* it doesn't produce evenly sized ranges
* it doesn't handle or test rangeIds of -1, which are ambiguous and can mean smaller than the smallest range _or_ larger than the largest range, and `RangeIndexReader.getDocIds` never knew the difference.
If this feature is in use, these bugs are probably acceptable to users. What I'm trying to do here is move some code around so I can eventually replace the feature, I don't want to fix every problem with the existing implementation in the process of doing so.
--
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] richardstartin commented on a change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r710459841
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/creator/RangeIndexCreatorTest.java
##########
@@ -242,141 +242,352 @@ private void addDataToIndexer(DataType dataType, int numDocs, int numValuesPerEn
}
}
- private void checkValueForDocId(DataType dataType, Number[] values, Number[] rangeStartArray, int rangeId, int docId,
+ private void verifyRangesForDataType(DataType dataType, Object values, Object ranges, int numValuesPerMVEntry,
+ RangeIndexReader<ImmutableRoaringBitmap> rangeIndexReader) {
+ switch (dataType) {
+ case INT: {
+ // single bucket ranges
+ int rangeId = 0;
+ for (int[] range : (int[][]) ranges) {
+ assertNull(rangeIndexReader.getMatchingDocIds(range[0], range[1]),
+ "range index can't guarantee match within a single range");
+ ImmutableRoaringBitmap partialMatches = rangeIndexReader.getPartiallyMatchingDocIds(range[0], range[1]);
+ assertNotNull(partialMatches, "partial matches for single range must not be null");
+ for (int docId : partialMatches.toArray()) {
+ checkValueForDocId(dataType, values, ranges, rangeId, docId, numValuesPerMVEntry);
+ }
+ ++rangeId;
+ }
+ // multi bucket ranges
+ int[] lowerPartialRange = ((int[][]) ranges)[0];
+ int[] coveredRange = ((int[][]) ranges)[1];
+ int[] upperPartialRange = ((int[][]) ranges)[2];
+ ImmutableRoaringBitmap matches = rangeIndexReader.getMatchingDocIds(lowerPartialRange[0], upperPartialRange[1]);
+ assertNotNull(matches, "matches for covered range must not be null");
+ for (int docId : matches.toArray()) {
+ checkValueForDocId(dataType, values, ranges, 1, docId, numValuesPerMVEntry);
+ }
+ assertEquals(matches, rangeIndexReader.getPartiallyMatchingDocIds(coveredRange[0], coveredRange[1]));
+ // partial matches must be the combination of the two edge buckets
+ ImmutableRoaringBitmap partialMatches = rangeIndexReader.getPartiallyMatchingDocIds(
+ lowerPartialRange[0], upperPartialRange[1]);
+ assertNotNull(partialMatches, "partial matches for single range must not be null");
+ assertEquals(ImmutableRoaringBitmap.or(
+ rangeIndexReader.getPartiallyMatchingDocIds(lowerPartialRange[0], lowerPartialRange[1]),
+ rangeIndexReader.getPartiallyMatchingDocIds(upperPartialRange[0], upperPartialRange[1])), partialMatches);
+ // edge cases
+ assertEquals(((int[]) values).length, numValuesPerMVEntry
Review comment:
This API doesn't exist for `Random.nextLong()`. The `Random` is seeded for determinism, as is good practice in testing (this is not a fuzz test, this is not property based testing) so this is not a problem.
--
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 change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r710539984
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/creator/RangeIndexCreatorTest.java
##########
@@ -35,11 +37,12 @@
import org.testng.annotations.Test;
import static org.apache.pinot.segment.spi.V1Constants.Indexes.BITMAP_RANGE_INDEX_FILE_EXTENSION;
+import static org.testng.Assert.*;
public class RangeIndexCreatorTest {
private static final File INDEX_DIR = new File(FileUtils.getTempDirectory(), "RangeIndexCreatorTest");
- private static final Random RANDOM = new Random();
+ private static final Random RANDOM = new Random(42);
Review comment:
I think the non-determinism in the article doesn't refer to fixing the input data, but the test should not be flaky (in the article: `In talking with development teams I've often heard about the problem of non-deterministic tests - tests that sometimes pass and sometimes fail.`).
The reason why we use random input values is because we usually cannot come up with (or too time consuming to create) all different inputs to cover all corner cases, so we use random input to do incremental checks during each CI run. The test should pass regardless of the input values, and when we detect test flakiness, we can check and fix the implementation (or the test).
I think this test can be flaky in certain cases (e.g. MIN/MAX integer values are generated, same values are generated and split into different ranges), but since the old implementation is going to be deprecated, we can leave it as is and not spend time fixing this test.
--
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 change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r710533725
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/creator/RangeIndexCreatorTest.java
##########
@@ -106,23 +109,22 @@ private void testDataType(DataType dataType)
throws IOException {
FieldSpec fieldSpec = new DimensionFieldSpec(COLUMN_NAME, dataType, true);
int numDocs = 1000;
- Number[] values = new Number[numDocs];
+ Object values = valuesArray(dataType, numDocs);
+ int numValuesPerRange;
try (RangeIndexCreator creator = new RangeIndexCreator(INDEX_DIR, fieldSpec, dataType, -1, -1, numDocs, numDocs)) {
addDataToIndexer(dataType, numDocs, 1, creator, values);
creator.seal();
+ // account for off by one bug in v1 implementation
+ numValuesPerRange = creator.getNumValuesPerRange() + 1;
Review comment:
I see, you are right. The `if (i > start + boundary)` in the current implementation is causing this, which should be `if (i >= start + boundary)` instead.
Since we're going to deprecate the old implementation, okay to leave it as is
--
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 edited a comment on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-919947162
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7435](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (770267c) into [master](https://codecov.io/gh/apache/pinot/commit/ad2e0632fa8d8ab84316d25ce2cd4c91c18b11bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ad2e063) will **decrease** coverage by `9.50%`.
> The diff coverage is `68.11%`.
[](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7435 +/- ##
============================================
- Coverage 71.90% 62.40% -9.51%
+ Complexity 3429 3374 -55
============================================
Files 1517 1507 -10
Lines 75118 74771 -347
Branches 10950 10911 -39
============================================
- Hits 54013 46660 -7353
- Misses 17470 24744 +7274
+ Partials 3635 3367 -268
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `29.02% <18.84%> (-0.14%)` | :arrow_down: |
| unittests1 | `69.87% <49.27%> (+0.15%)` | :arrow_up: |
| unittests2 | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `58.26% <ø> (ø)` | |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `88.88% <0.00%> (ø)` | |
| [...local/segment/index/datasource/BaseDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvQmFzZURhdGFTb3VyY2UuamF2YQ==) | `95.83% <ø> (ø)` | |
| [...al/segment/index/datasource/MutableDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvTXV0YWJsZURhdGFTb3VyY2UuamF2YQ==) | `69.23% <ø> (ø)` | |
| [...ent/virtualcolumn/VirtualColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `90.47% <ø> (ø)` | |
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `40.42% <41.93%> (-4.26%)` | :arrow_down: |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `87.00% <91.42%> (ø)` | |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `80.63% <100.00%> (+0.08%)` | :arrow_up: |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/controller/recommender/io/ConfigManager.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9pby9Db25maWdNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [282 more](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ad2e063...770267c](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] richardstartin commented on a change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r710456033
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/creator/RangeIndexCreatorTest.java
##########
@@ -106,23 +109,22 @@ private void testDataType(DataType dataType)
throws IOException {
FieldSpec fieldSpec = new DimensionFieldSpec(COLUMN_NAME, dataType, true);
int numDocs = 1000;
- Number[] values = new Number[numDocs];
+ Object values = valuesArray(dataType, numDocs);
+ int numValuesPerRange;
try (RangeIndexCreator creator = new RangeIndexCreator(INDEX_DIR, fieldSpec, dataType, -1, -1, numDocs, numDocs)) {
addDataToIndexer(dataType, numDocs, 1, creator, values);
creator.seal();
+ // account for off by one bug in v1 implementation
+ numValuesPerRange = creator.getNumValuesPerRange() + 1;
Review comment:
The current implementation reports 50 values per bucket but assigns 51 values to the first 19 buckets and 31 to the last 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
[GitHub] [pinot] richardstartin commented on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-920714310
I've fixed the pre-existing bugs and have added tests for these cases, which fail on master.
--
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 change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r710532269
##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/RangeIndexReader.java
##########
@@ -0,0 +1,91 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.spi.index.reader;
+
+import java.io.Closeable;
+
+/**
+ * Interface for indexed range queries
+ * @param <T>
+ */
+public interface RangeIndexReader<T> extends Closeable {
Review comment:
Sure that is fine. I saw you replied with thumb up and thought you want to change that
--
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 edited a comment on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-919947162
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7435](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (85de50b) into [master](https://codecov.io/gh/apache/pinot/commit/ad2e0632fa8d8ab84316d25ce2cd4c91c18b11bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ad2e063) will **decrease** coverage by `7.01%`.
> The diff coverage is `81.65%`.
> :exclamation: Current head 85de50b differs from pull request most recent head 1e99461. Consider uploading reports for the commit 1e99461 to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7435 +/- ##
============================================
- Coverage 71.90% 64.88% -7.02%
+ Complexity 3429 3318 -111
============================================
Files 1517 1507 -10
Lines 75118 74771 -347
Branches 10950 10911 -39
============================================
- Hits 54013 48516 -5497
- Misses 17470 22800 +5330
+ Partials 3635 3455 -180
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `30.43% <51.37%> (+0.03%)` | :arrow_up: |
| integration2 | `29.01% <49.08%> (-0.16%)` | :arrow_down: |
| unittests1 | `69.83% <67.41%> (+0.10%)` | :arrow_up: |
| unittests2 | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `58.26% <ø> (ø)` | |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `88.88% <0.00%> (ø)` | |
| [...local/segment/index/datasource/BaseDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvQmFzZURhdGFTb3VyY2UuamF2YQ==) | `95.83% <ø> (ø)` | |
| [...al/segment/index/datasource/MutableDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvTXV0YWJsZURhdGFTb3VyY2UuamF2YQ==) | `69.23% <ø> (ø)` | |
| [...ent/virtualcolumn/VirtualColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `90.47% <ø> (ø)` | |
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `40.42% <41.93%> (-4.26%)` | :arrow_down: |
| [.../pinot/core/data/manager/BaseTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvQmFzZVRhYmxlRGF0YU1hbmFnZXIuamF2YQ==) | `84.81% <82.69%> (-2.54%)` | :arrow_down: |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `87.00% <91.42%> (ø)` | |
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `68.69% <100.00%> (+1.20%)` | :arrow_up: |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `80.63% <100.00%> (+0.08%)` | :arrow_up: |
| ... and [210 more](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ad2e063...1e99461](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] richardstartin commented on a change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r710461911
##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/RangeIndexReader.java
##########
@@ -0,0 +1,91 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.spi.index.reader;
+
+import java.io.Closeable;
+
+/**
+ * Interface for indexed range queries
+ * @param <T>
+ */
+public interface RangeIndexReader<T> extends Closeable {
Review comment:
I don't consider this important at this stage.
--
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 edited a comment on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-919947162
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7435](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c8d861a) into [master](https://codecov.io/gh/apache/pinot/commit/ad2e0632fa8d8ab84316d25ce2cd4c91c18b11bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ad2e063) will **decrease** coverage by `34.08%`.
> The diff coverage is `51.37%`.
> :exclamation: Current head c8d861a differs from pull request most recent head 770267c. Consider uploading reports for the commit 770267c to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7435 +/- ##
=============================================
- Coverage 71.90% 37.82% -34.09%
+ Complexity 3429 92 -3337
=============================================
Files 1517 1516 -1
Lines 75118 75119 +1
Branches 10950 10949 -1
=============================================
- Hits 54013 28413 -25600
- Misses 17470 44503 +27033
+ Partials 3635 2203 -1432
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `30.40% <51.37%> (+0.01%)` | :arrow_up: |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.49% <0.00%> (-0.04%)` | :arrow_down: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `0.00% <ø> (-58.27%)` | :arrow_down: |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `0.00% <0.00%> (-80.55%)` | :arrow_down: |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
| [...local/segment/index/datasource/BaseDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvQmFzZURhdGFTb3VyY2UuamF2YQ==) | `0.00% <ø> (-95.84%)` | :arrow_down: |
| [...al/segment/index/datasource/MutableDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvTXV0YWJsZURhdGFTb3VyY2UuamF2YQ==) | `0.00% <ø> (-69.24%)` | :arrow_down: |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ent/virtualcolumn/VirtualColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `0.00% <ø> (-90.48%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `0.00% <0.00%> (-26.32%)` | :arrow_down: |
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `42.68% <33.33%> (-24.81%)` | :arrow_down: |
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `40.42% <41.93%> (-4.26%)` | :arrow_down: |
| ... and [882 more](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ad2e063...770267c](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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 edited a comment on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-919947162
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7435](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7cb8c49) into [master](https://codecov.io/gh/apache/pinot/commit/ad2e0632fa8d8ab84316d25ce2cd4c91c18b11bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ad2e063) will **decrease** coverage by `2.18%`.
> The diff coverage is `33.33%`.
> :exclamation: Current head 7cb8c49 differs from pull request most recent head 6ed2053. Consider uploading reports for the commit 6ed2053 to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7435 +/- ##
============================================
- Coverage 71.90% 69.71% -2.19%
+ Complexity 3429 3270 -159
============================================
Files 1517 1123 -394
Lines 75118 53052 -22066
Branches 10950 8001 -2949
============================================
- Hits 54013 36986 -17027
+ Misses 17470 13434 -4036
+ Partials 3635 2632 -1003
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `69.71% <33.33%> (-0.01%)` | :arrow_down: |
| unittests2 | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `0.00% <0.00%> (-44.69%)` | :arrow_down: |
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `58.26% <ø> (ø)` | |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `88.88% <0.00%> (ø)` | |
| [...local/segment/index/datasource/BaseDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvQmFzZURhdGFTb3VyY2UuamF2YQ==) | `95.83% <ø> (ø)` | |
| [...al/segment/index/datasource/MutableDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvTXV0YWJsZURhdGFTb3VyY2UuamF2YQ==) | `69.23% <ø> (ø)` | |
| [...ent/virtualcolumn/VirtualColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `90.47% <ø> (ø)` | |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `80.63% <100.00%> (+0.08%)` | :arrow_up: |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `75.70% <100.00%> (ø)` | |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [602 more](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ad2e063...6ed2053](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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 edited a comment on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-919947162
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7435](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c8d861a) into [master](https://codecov.io/gh/apache/pinot/commit/ad2e0632fa8d8ab84316d25ce2cd4c91c18b11bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ad2e063) will **decrease** coverage by `57.40%`.
> The diff coverage is `0.00%`.
> :exclamation: Current head c8d861a differs from pull request most recent head 770267c. Consider uploading reports for the commit 770267c to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7435 +/- ##
=============================================
- Coverage 71.90% 14.49% -57.41%
+ Complexity 3429 92 -3337
=============================================
Files 1517 1471 -46
Lines 75118 73314 -1804
Branches 10950 10757 -193
=============================================
- Hits 54013 10627 -43386
- Misses 17470 61928 +44458
+ Partials 3635 759 -2876
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.49% <0.00%> (-0.04%)` | :arrow_down: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../pinot/core/data/manager/BaseTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvQmFzZVRhYmxlRGF0YU1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-87.36%)` | :arrow_down: |
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-67.49%)` | :arrow_down: |
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `0.00% <0.00%> (-44.69%)` | :arrow_down: |
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `0.00% <ø> (-58.27%)` | :arrow_down: |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `0.00% <0.00%> (-80.55%)` | :arrow_down: |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
| [...local/segment/index/datasource/BaseDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvQmFzZURhdGFTb3VyY2UuamF2YQ==) | `0.00% <ø> (-95.84%)` | :arrow_down: |
| [...al/segment/index/datasource/MutableDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvTXV0YWJsZURhdGFTb3VyY2UuamF2YQ==) | `0.00% <ø> (-69.24%)` | :arrow_down: |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ent/virtualcolumn/VirtualColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `0.00% <ø> (-90.48%)` | :arrow_down: |
| ... and [1202 more](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ad2e063...770267c](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] richardstartin commented on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-920793656
Looks like flaky tests are being flaky again, please rerun CI.
--
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 change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r709590849
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/RangeIndexReaderImpl.java
##########
@@ -217,4 +272,26 @@ public void close() {
// NOTE: DO NOT close the PinotDataBuffer here because it is tracked by the caller and might be reused later. The
// caller is responsible of closing the PinotDataBuffer.
}
+
+ private ImmutableRoaringBitmap getMatchesInRange(int firstRangeId, int lastRangeId) {
+ if (firstRangeId == lastRangeId) {
+ return null;
+ }
+ MutableRoaringBitmap matching = new MutableRoaringBitmap();
+ for (int rangeId = firstRangeId + 1; rangeId < lastRangeId; ++rangeId) {
Review comment:
Won't work if `lastRangeId` is -1?
##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/RangeIndexReader.java
##########
@@ -0,0 +1,91 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.spi.index.reader;
+
+import java.io.Closeable;
+
+/**
+ * Interface for indexed range queries
+ * @param <T>
+ */
+public interface RangeIndexReader<T> extends Closeable {
+ /**
+ * Returns doc ids with a value between min and max, both inclusive.
+ * @param min the inclusive lower bound.
+ * @param max the inclusive upper bound.
+ * @return the matching doc ids.
+ */
+ T getMatchingDocIds(long min, long max);
Review comment:
(nit) Re-order the methods following `int, long, float, double` to be consistent with other places for readability
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/RangeIndexReaderImpl.java
##########
@@ -217,4 +272,26 @@ public void close() {
// NOTE: DO NOT close the PinotDataBuffer here because it is tracked by the caller and might be reused later. The
// caller is responsible of closing the PinotDataBuffer.
}
+
+ private ImmutableRoaringBitmap getMatchesInRange(int firstRangeId, int lastRangeId) {
+ if (firstRangeId == lastRangeId) {
+ return null;
Review comment:
If both first and last is -1, it might be no match or full match. We need to handle them separately
##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/RangeIndexReader.java
##########
@@ -0,0 +1,91 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.spi.index.reader;
+
+import java.io.Closeable;
+
+/**
+ * Interface for indexed range queries
+ * @param <T>
+ */
+public interface RangeIndexReader<T> extends Closeable {
Review comment:
Does it make sense to fix the type to `ImmutableBitmapDataProvider`?
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/RangeIndexReaderImpl.java
##########
@@ -217,4 +272,26 @@ public void close() {
// NOTE: DO NOT close the PinotDataBuffer here because it is tracked by the caller and might be reused later. The
// caller is responsible of closing the PinotDataBuffer.
}
+
+ private ImmutableRoaringBitmap getMatchesInRange(int firstRangeId, int lastRangeId) {
Review comment:
Good to add some comments on how the -1 is handled (list the different scenarios)
##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/RangeIndexReader.java
##########
@@ -0,0 +1,91 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.spi.index.reader;
+
+import java.io.Closeable;
+
+/**
+ * Interface for indexed range queries
+ * @param <T>
+ */
+public interface RangeIndexReader<T> extends Closeable {
+ /**
+ * Returns doc ids with a value between min and max, both inclusive.
+ * @param min the inclusive lower bound.
+ * @param max the inclusive upper bound.
+ * @return the matching doc ids.
+ */
+ T getMatchingDocIds(long min, long max);
Review comment:
Annotate the methods return value as `Nullable` and add javadoc on when to return `null`
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/RangeIndexReaderImpl.java
##########
@@ -217,4 +272,26 @@ public void close() {
// NOTE: DO NOT close the PinotDataBuffer here because it is tracked by the caller and might be reused later. The
// caller is responsible of closing the PinotDataBuffer.
}
+
+ private ImmutableRoaringBitmap getMatchesInRange(int firstRangeId, int lastRangeId) {
+ if (firstRangeId == lastRangeId) {
+ return null;
+ }
+ MutableRoaringBitmap matching = new MutableRoaringBitmap();
+ for (int rangeId = firstRangeId + 1; rangeId < lastRangeId; ++rangeId) {
Review comment:
(nit) We usually put `rangeId++`
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RangeIndexBasedFilterOperator.java
##########
@@ -51,74 +51,84 @@ public RangeIndexBasedFilterOperator(PredicateEvaluator rangePredicateEvaluator,
@Override
protected FilterBlock getNextBlock() {
- RangeIndexReader rangeIndexReader = (RangeIndexReader) _dataSource.getRangeIndex();
+ @SuppressWarnings("unchecked")
+ RangeIndexReader<ImmutableRoaringBitmap> rangeIndexReader =
+ (RangeIndexReader<ImmutableRoaringBitmap>) _dataSource.getRangeIndex();
assert rangeIndexReader != null;
+ ImmutableRoaringBitmap matches;
+ // if the implementation cannot match the entire query exactly, it will
+ // yield partial matches, which need to be verified by scanning. If it
+ // can answer the query exactly, this will be null.
+ ImmutableRoaringBitmap partialMatches;
int firstRangeId;
int lastRangeId;
if (_rangePredicateEvaluator instanceof SortedDictionaryBasedRangePredicateEvaluator) {
- firstRangeId = rangeIndexReader
- .findRangeId(((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getStartDictId());
// NOTE: End dictionary id is exclusive in OfflineDictionaryBasedRangePredicateEvaluator.
- lastRangeId = rangeIndexReader
- .findRangeId(((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getEndDictId() - 1);
+ matches = rangeIndexReader.getMatchingDocIds(
+ ((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getStartDictId(),
+ ((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getEndDictId() - 1);
+ partialMatches = rangeIndexReader.getPartiallyMatchingDocIds(
+ ((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getStartDictId(),
+ ((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getEndDictId() - 1);
} else {
switch (_rangePredicateEvaluator.getDataType()) {
case INT:
- firstRangeId = rangeIndexReader
- .findRangeId(((IntRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).geLowerBound());
- lastRangeId = rangeIndexReader
- .findRangeId(((IntRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound());
+ matches = rangeIndexReader.getMatchingDocIds(
+ ((IntRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).geLowerBound(),
+ ((IntRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound());
+ partialMatches = rangeIndexReader.getPartiallyMatchingDocIds(
+ ((IntRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).geLowerBound(),
+ ((IntRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound());
break;
case LONG:
- firstRangeId = rangeIndexReader
- .findRangeId(((LongRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).geLowerBound());
- lastRangeId = rangeIndexReader
- .findRangeId(((LongRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound());
+ matches = rangeIndexReader.getMatchingDocIds(
+ ((LongRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).geLowerBound(),
+ ((LongRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound());
+ partialMatches = rangeIndexReader.getPartiallyMatchingDocIds(
+ ((LongRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).geLowerBound(),
+ ((LongRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound());
break;
case FLOAT:
- firstRangeId = rangeIndexReader
- .findRangeId(((FloatRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).geLowerBound());
- lastRangeId = rangeIndexReader
- .findRangeId(((FloatRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound());
+ matches = rangeIndexReader.getMatchingDocIds(
+ ((FloatRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).geLowerBound(),
+ ((FloatRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound());
+ partialMatches = rangeIndexReader.getPartiallyMatchingDocIds(
+ ((FloatRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).geLowerBound(),
+ ((FloatRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound());
break;
case DOUBLE:
- firstRangeId = rangeIndexReader
- .findRangeId(((DoubleRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).geLowerBound());
- lastRangeId = rangeIndexReader
- .findRangeId(((DoubleRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound());
+ matches = rangeIndexReader.getMatchingDocIds(
+ ((DoubleRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).geLowerBound(),
+ ((DoubleRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound());
+ partialMatches = rangeIndexReader.getPartiallyMatchingDocIds(
+ ((DoubleRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).geLowerBound(),
+ ((DoubleRawValueBasedRangePredicateEvaluator) _rangePredicateEvaluator).getUpperBound());
break;
default:
throw new IllegalStateException("String and Bytes data type not supported for Range Indexing");
}
}
-
- // Need to scan the first and last range as they might be partially matched
- // TODO: Detect fully matched first and last range
- ImmutableRoaringBitmap docIdsToScan;
- if (firstRangeId == lastRangeId) {
- docIdsToScan = rangeIndexReader.getDocIds(firstRangeId);
+ // this branch is likely until RangeIndexReader reimplemented and enabled by default
+ if (null != partialMatches) {
Review comment:
(nit) we usually put `(partialMatches != null)`, same for other places
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RangeIndexBasedFilterOperator.java
##########
@@ -51,74 +51,84 @@ public RangeIndexBasedFilterOperator(PredicateEvaluator rangePredicateEvaluator,
@Override
protected FilterBlock getNextBlock() {
- RangeIndexReader rangeIndexReader = (RangeIndexReader) _dataSource.getRangeIndex();
+ @SuppressWarnings("unchecked")
+ RangeIndexReader<ImmutableRoaringBitmap> rangeIndexReader =
+ (RangeIndexReader<ImmutableRoaringBitmap>) _dataSource.getRangeIndex();
assert rangeIndexReader != null;
+ ImmutableRoaringBitmap matches;
+ // if the implementation cannot match the entire query exactly, it will
+ // yield partial matches, which need to be verified by scanning. If it
+ // can answer the query exactly, this will be null.
+ ImmutableRoaringBitmap partialMatches;
int firstRangeId;
int lastRangeId;
if (_rangePredicateEvaluator instanceof SortedDictionaryBasedRangePredicateEvaluator) {
- firstRangeId = rangeIndexReader
- .findRangeId(((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getStartDictId());
// NOTE: End dictionary id is exclusive in OfflineDictionaryBasedRangePredicateEvaluator.
- lastRangeId = rangeIndexReader
- .findRangeId(((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getEndDictId() - 1);
+ matches = rangeIndexReader.getMatchingDocIds(
+ ((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getStartDictId(),
Review comment:
(nit) Cache the `min` and `max` value
--
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] richardstartin commented on a change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r709628801
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/RangeIndexReaderImpl.java
##########
@@ -217,4 +272,26 @@ public void close() {
// NOTE: DO NOT close the PinotDataBuffer here because it is tracked by the caller and might be reused later. The
// caller is responsible of closing the PinotDataBuffer.
}
+
+ private ImmutableRoaringBitmap getMatchesInRange(int firstRangeId, int lastRangeId) {
+ if (firstRangeId == lastRangeId) {
+ return null;
+ }
+ MutableRoaringBitmap matching = new MutableRoaringBitmap();
+ for (int rangeId = firstRangeId + 1; rangeId < lastRangeId; ++rangeId) {
Review comment:
No, this is correct. `firstRangeId == 0` means that the lower bound was below the lowest bucket, so range 0 (`firstRangeId + 1`) is within the range of the query so long as `lastRangeId > 0`. Similarly, a `lastRangeId == -1` means that the upper bound of the query was below the first range, which means that none of the rows are within the range of the query, so an empty bitmap is produced.
--
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 edited a comment on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-919947162
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7435](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (770267c) into [master](https://codecov.io/gh/apache/pinot/commit/ad2e0632fa8d8ab84316d25ce2cd4c91c18b11bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ad2e063) will **decrease** coverage by `2.02%`.
> The diff coverage is `49.27%`.
[](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7435 +/- ##
============================================
- Coverage 71.90% 69.87% -2.03%
+ Complexity 3429 3289 -140
============================================
Files 1517 1123 -394
Lines 75118 53143 -21975
Branches 10950 8012 -2938
============================================
- Hits 54013 37134 -16879
+ Misses 17470 13377 -4093
+ Partials 3635 2632 -1003
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `69.87% <49.27%> (+0.15%)` | :arrow_up: |
| unittests2 | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `0.00% <0.00%> (-44.69%)` | :arrow_down: |
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `58.26% <ø> (ø)` | |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `88.88% <0.00%> (ø)` | |
| [...local/segment/index/datasource/BaseDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvQmFzZURhdGFTb3VyY2UuamF2YQ==) | `95.83% <ø> (ø)` | |
| [...al/segment/index/datasource/MutableDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvTXV0YWJsZURhdGFTb3VyY2UuamF2YQ==) | `69.23% <ø> (ø)` | |
| [...ent/virtualcolumn/VirtualColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `90.47% <ø> (ø)` | |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `87.00% <91.42%> (ø)` | |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `80.63% <100.00%> (+0.08%)` | :arrow_up: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [606 more](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ad2e063...770267c](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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 edited a comment on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-919947162
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7435](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8110c14) into [master](https://codecov.io/gh/apache/pinot/commit/ad2e0632fa8d8ab84316d25ce2cd4c91c18b11bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ad2e063) will **decrease** coverage by `1.21%`.
> The diff coverage is `52.23%`.
[](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7435 +/- ##
============================================
- Coverage 71.90% 70.68% -1.22%
- Complexity 3429 3455 +26
============================================
Files 1517 1517
Lines 75118 75148 +30
Branches 10950 10955 +5
============================================
- Hits 54013 53118 -895
- Misses 17470 18395 +925
Partials 3635 3635
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `29.10% <22.38%> (-0.07%)` | :arrow_down: |
| unittests1 | `69.78% <29.85%> (+0.06%)` | :arrow_up: |
| unittests2 | `14.50% <0.00%> (-0.02%)` | :arrow_down: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `58.26% <ø> (ø)` | |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `88.88% <0.00%> (ø)` | |
| [...local/segment/index/datasource/BaseDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvQmFzZURhdGFTb3VyY2UuamF2YQ==) | `95.83% <ø> (ø)` | |
| [...al/segment/index/datasource/MutableDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvTXV0YWJsZURhdGFTb3VyY2UuamF2YQ==) | `69.23% <ø> (ø)` | |
| [...ent/virtualcolumn/VirtualColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `90.47% <ø> (ø)` | |
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `35.71% <36.58%> (-8.97%)` | :arrow_down: |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `72.07% <78.26%> (ø)` | |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `80.63% <100.00%> (+0.08%)` | :arrow_up: |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...nverttorawindex/ConvertToRawIndexTaskExecutor.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvY29udmVydHRvcmF3aW5kZXgvQ29udmVydFRvUmF3SW5kZXhUYXNrRXhlY3V0b3IuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [106 more](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ad2e063...8110c14](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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 edited a comment on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-919947162
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7435](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (85de50b) into [master](https://codecov.io/gh/apache/pinot/commit/ad2e0632fa8d8ab84316d25ce2cd4c91c18b11bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ad2e063) will **increase** coverage by `0.09%`.
> The diff coverage is `81.65%`.
> :exclamation: Current head 85de50b differs from pull request most recent head 1e99461. Consider uploading reports for the commit 1e99461 to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7435 +/- ##
============================================
+ Coverage 71.90% 72.00% +0.09%
+ Complexity 3429 3409 -20
============================================
Files 1517 1516 -1
Lines 75118 75119 +1
Branches 10950 10949 -1
============================================
+ Hits 54013 54086 +73
+ Misses 17470 17405 -65
+ Partials 3635 3628 -7
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `30.43% <51.37%> (+0.03%)` | :arrow_up: |
| integration2 | `29.16% <49.08%> (-0.01%)` | :arrow_down: |
| unittests1 | `69.89% <67.41%> (+0.17%)` | :arrow_up: |
| unittests2 | `14.48% <0.00%> (-0.04%)` | :arrow_down: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `58.26% <ø> (ø)` | |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `88.88% <0.00%> (ø)` | |
| [...local/segment/index/datasource/BaseDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvQmFzZURhdGFTb3VyY2UuamF2YQ==) | `95.83% <ø> (ø)` | |
| [...al/segment/index/datasource/MutableDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvTXV0YWJsZURhdGFTb3VyY2UuamF2YQ==) | `69.23% <ø> (ø)` | |
| [...ent/virtualcolumn/VirtualColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `90.47% <ø> (ø)` | |
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `40.42% <41.93%> (-4.26%)` | :arrow_down: |
| [.../pinot/core/data/manager/BaseTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvQmFzZVRhYmxlRGF0YU1hbmFnZXIuamF2YQ==) | `84.81% <82.69%> (-2.54%)` | :arrow_down: |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `87.00% <91.42%> (ø)` | |
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `68.69% <100.00%> (+1.20%)` | :arrow_up: |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `80.63% <100.00%> (+0.08%)` | :arrow_up: |
| ... and [31 more](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ad2e063...1e99461](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] richardstartin commented on a change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r709632713
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/RangeIndexReaderImpl.java
##########
@@ -217,4 +272,26 @@ public void close() {
// NOTE: DO NOT close the PinotDataBuffer here because it is tracked by the caller and might be reused later. The
// caller is responsible of closing the PinotDataBuffer.
}
+
+ private ImmutableRoaringBitmap getMatchesInRange(int firstRangeId, int lastRangeId) {
+ if (firstRangeId == lastRangeId) {
+ return null;
Review comment:
Not necessarily, -1 currently means both smaller than the smallest range and larger than the largest range, so it could mean a full match or no match depending on the original query parameters. This is consistent with the current behaviour though, which appears to be incorrect.
--
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] richardstartin commented on a change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r709633913
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RangeIndexBasedFilterOperator.java
##########
@@ -51,74 +51,84 @@ public RangeIndexBasedFilterOperator(PredicateEvaluator rangePredicateEvaluator,
@Override
protected FilterBlock getNextBlock() {
- RangeIndexReader rangeIndexReader = (RangeIndexReader) _dataSource.getRangeIndex();
+ @SuppressWarnings("unchecked")
+ RangeIndexReader<ImmutableRoaringBitmap> rangeIndexReader =
+ (RangeIndexReader<ImmutableRoaringBitmap>) _dataSource.getRangeIndex();
assert rangeIndexReader != null;
+ ImmutableRoaringBitmap matches;
+ // if the implementation cannot match the entire query exactly, it will
+ // yield partial matches, which need to be verified by scanning. If it
+ // can answer the query exactly, this will be null.
+ ImmutableRoaringBitmap partialMatches;
int firstRangeId;
int lastRangeId;
if (_rangePredicateEvaluator instanceof SortedDictionaryBasedRangePredicateEvaluator) {
- firstRangeId = rangeIndexReader
- .findRangeId(((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getStartDictId());
// NOTE: End dictionary id is exclusive in OfflineDictionaryBasedRangePredicateEvaluator.
- lastRangeId = rangeIndexReader
- .findRangeId(((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getEndDictId() - 1);
+ matches = rangeIndexReader.getMatchingDocIds(
+ ((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getStartDictId(),
Review comment:
Why?
--
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 edited a comment on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-919947162
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7435](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (85de50b) into [master](https://codecov.io/gh/apache/pinot/commit/ad2e0632fa8d8ab84316d25ce2cd4c91c18b11bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ad2e063) will **increase** coverage by `0.06%`.
> The diff coverage is `81.65%`.
> :exclamation: Current head 85de50b differs from pull request most recent head 1e99461. Consider uploading reports for the commit 1e99461 to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7435 +/- ##
============================================
+ Coverage 71.90% 71.96% +0.06%
+ Complexity 3429 3403 -26
============================================
Files 1517 1516 -1
Lines 75118 75119 +1
Branches 10950 10949 -1
============================================
+ Hits 54013 54059 +46
+ Misses 17470 17427 -43
+ Partials 3635 3633 -2
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `30.43% <51.37%> (+0.03%)` | :arrow_up: |
| integration2 | `29.01% <49.08%> (-0.16%)` | :arrow_down: |
| unittests1 | `69.83% <67.41%> (+0.10%)` | :arrow_up: |
| unittests2 | `14.48% <0.00%> (-0.04%)` | :arrow_down: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `58.26% <ø> (ø)` | |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `88.88% <0.00%> (ø)` | |
| [...local/segment/index/datasource/BaseDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvQmFzZURhdGFTb3VyY2UuamF2YQ==) | `95.83% <ø> (ø)` | |
| [...al/segment/index/datasource/MutableDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvTXV0YWJsZURhdGFTb3VyY2UuamF2YQ==) | `69.23% <ø> (ø)` | |
| [...ent/virtualcolumn/VirtualColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `90.47% <ø> (ø)` | |
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `40.42% <41.93%> (-4.26%)` | :arrow_down: |
| [.../pinot/core/data/manager/BaseTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvQmFzZVRhYmxlRGF0YU1hbmFnZXIuamF2YQ==) | `84.81% <82.69%> (-2.54%)` | :arrow_down: |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `87.00% <91.42%> (ø)` | |
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `68.69% <100.00%> (+1.20%)` | :arrow_up: |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `80.63% <100.00%> (+0.08%)` | :arrow_up: |
| ... and [29 more](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ad2e063...1e99461](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] richardstartin commented on a change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r709877633
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RangeIndexBasedFilterOperator.java
##########
@@ -51,74 +51,84 @@ public RangeIndexBasedFilterOperator(PredicateEvaluator rangePredicateEvaluator,
@Override
protected FilterBlock getNextBlock() {
- RangeIndexReader rangeIndexReader = (RangeIndexReader) _dataSource.getRangeIndex();
+ @SuppressWarnings("unchecked")
+ RangeIndexReader<ImmutableRoaringBitmap> rangeIndexReader =
+ (RangeIndexReader<ImmutableRoaringBitmap>) _dataSource.getRangeIndex();
assert rangeIndexReader != null;
+ ImmutableRoaringBitmap matches;
+ // if the implementation cannot match the entire query exactly, it will
+ // yield partial matches, which need to be verified by scanning. If it
+ // can answer the query exactly, this will be null.
+ ImmutableRoaringBitmap partialMatches;
int firstRangeId;
int lastRangeId;
if (_rangePredicateEvaluator instanceof SortedDictionaryBasedRangePredicateEvaluator) {
- firstRangeId = rangeIndexReader
- .findRangeId(((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getStartDictId());
// NOTE: End dictionary id is exclusive in OfflineDictionaryBasedRangePredicateEvaluator.
- lastRangeId = rangeIndexReader
- .findRangeId(((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getEndDictId() - 1);
+ matches = rangeIndexReader.getMatchingDocIds(
+ ((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getStartDictId(),
Review comment:
There is no performance benefit to the suggested change, these values are constants in `SortedDictionaryBasedRangePredicateEvaluator` (and all other implementations used in this block of code):
```java
public int getStartDictId() {
return _startDictId;
}
public int getEndDictId() {
return _endDictId;
}
```
I will make the change on the basis of readability, but it's very difficult to spot bottlenecks by code review so it's better to rely on data.
--
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 change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r709645668
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RangeIndexBasedFilterOperator.java
##########
@@ -51,74 +51,84 @@ public RangeIndexBasedFilterOperator(PredicateEvaluator rangePredicateEvaluator,
@Override
protected FilterBlock getNextBlock() {
- RangeIndexReader rangeIndexReader = (RangeIndexReader) _dataSource.getRangeIndex();
+ @SuppressWarnings("unchecked")
+ RangeIndexReader<ImmutableRoaringBitmap> rangeIndexReader =
+ (RangeIndexReader<ImmutableRoaringBitmap>) _dataSource.getRangeIndex();
assert rangeIndexReader != null;
+ ImmutableRoaringBitmap matches;
+ // if the implementation cannot match the entire query exactly, it will
+ // yield partial matches, which need to be verified by scanning. If it
+ // can answer the query exactly, this will be null.
+ ImmutableRoaringBitmap partialMatches;
int firstRangeId;
int lastRangeId;
if (_rangePredicateEvaluator instanceof SortedDictionaryBasedRangePredicateEvaluator) {
- firstRangeId = rangeIndexReader
- .findRangeId(((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getStartDictId());
// NOTE: End dictionary id is exclusive in OfflineDictionaryBasedRangePredicateEvaluator.
- lastRangeId = rangeIndexReader
- .findRangeId(((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getEndDictId() - 1);
+ matches = rangeIndexReader.getMatchingDocIds(
+ ((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getStartDictId(),
Review comment:
Both performance and readability. What I mean is to put `((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getStartDictId()` and `((SortedDictionaryBasedRangePredicateEvaluator) _rangePredicateEvaluator).getEndDictId() - 1` into local variables instead of calculating them twice
--
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 edited a comment on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-919947162
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7435](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ca1708c) into [master](https://codecov.io/gh/apache/pinot/commit/ad2e0632fa8d8ab84316d25ce2cd4c91c18b11bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ad2e063) will **decrease** coverage by `57.40%`.
> The diff coverage is `0.00%`.
> :exclamation: Current head ca1708c differs from pull request most recent head 14cf0d9. Consider uploading reports for the commit 14cf0d9 to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7435 +/- ##
=============================================
- Coverage 71.90% 14.49% -57.41%
+ Complexity 3429 92 -3337
=============================================
Files 1517 1471 -46
Lines 75118 73314 -1804
Branches 10950 10757 -193
=============================================
- Hits 54013 10628 -43385
- Misses 17470 61928 +44458
+ Partials 3635 758 -2877
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.49% <0.00%> (-0.04%)` | :arrow_down: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../pinot/core/data/manager/BaseTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvQmFzZVRhYmxlRGF0YU1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-87.36%)` | :arrow_down: |
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-67.49%)` | :arrow_down: |
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `0.00% <0.00%> (-44.69%)` | :arrow_down: |
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `0.00% <ø> (-58.27%)` | :arrow_down: |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `0.00% <0.00%> (-80.55%)` | :arrow_down: |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
| [...local/segment/index/datasource/BaseDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvQmFzZURhdGFTb3VyY2UuamF2YQ==) | `0.00% <ø> (-95.84%)` | :arrow_down: |
| [...al/segment/index/datasource/MutableDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvTXV0YWJsZURhdGFTb3VyY2UuamF2YQ==) | `0.00% <ø> (-69.24%)` | :arrow_down: |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ent/virtualcolumn/VirtualColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `0.00% <ø> (-90.48%)` | :arrow_down: |
| ... and [1203 more](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ad2e063...14cf0d9](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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 change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r710400097
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/creator/RangeIndexCreatorTest.java
##########
@@ -35,11 +37,12 @@
import org.testng.annotations.Test;
import static org.apache.pinot.segment.spi.V1Constants.Indexes.BITMAP_RANGE_INDEX_FILE_EXTENSION;
+import static org.testng.Assert.*;
public class RangeIndexCreatorTest {
private static final File INDEX_DIR = new File(FileUtils.getTempDirectory(), "RangeIndexCreatorTest");
- private static final Random RANDOM = new Random();
+ private static final Random RANDOM = new Random(42);
Review comment:
Don't fix the seed
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/RangeIndexReaderImpl.java
##########
@@ -164,57 +220,71 @@ private long getOffset(final int rangeId) {
* @param value
* @return
*/
- public int findRangeId(int value) {
+ private int findRangeId(int value) {
for (int i = 0; i < _rangeStartArray.length; i++) {
if (value < _rangeStartArray[i].intValue()) {
return i - 1;
}
}
- if (value <= _lastRangeEnd.intValue()) {
- return _rangeStartArray.length - 1;
- }
- return -1;
+ return value <= _lastRangeEnd.intValue() ? _rangeStartArray.length - 1 : _rangeStartArray.length;
}
- public int findRangeId(long value) {
+ private int findRangeId(long value) {
for (int i = 0; i < _rangeStartArray.length; i++) {
if (value < _rangeStartArray[i].longValue()) {
return i - 1;
}
}
- if (value <= _lastRangeEnd.longValue()) {
- return _rangeStartArray.length - 1;
- }
- return -1;
+ return value <= _lastRangeEnd.longValue() ? _rangeStartArray.length - 1 : _rangeStartArray.length;
}
- public int findRangeId(float value) {
+ private int findRangeId(float value) {
for (int i = 0; i < _rangeStartArray.length; i++) {
if (value < _rangeStartArray[i].floatValue()) {
return i - 1;
}
}
- if (value <= _lastRangeEnd.floatValue()) {
- return _rangeStartArray.length - 1;
- }
- return -1;
+ return value <= _lastRangeEnd.floatValue() ? _rangeStartArray.length - 1 : _rangeStartArray.length;
}
- public int findRangeId(double value) {
+ private int findRangeId(double value) {
for (int i = 0; i < _rangeStartArray.length; i++) {
if (value < _rangeStartArray[i].doubleValue()) {
return i - 1;
}
}
- if (value <= _lastRangeEnd.doubleValue()) {
- return _rangeStartArray.length - 1;
- }
- return -1;
+ return value <= _lastRangeEnd.doubleValue() ? _rangeStartArray.length - 1 : _rangeStartArray.length;
}
@Override
public void close() {
// NOTE: DO NOT close the PinotDataBuffer here because it is tracked by the caller and might be reused later. The
// caller is responsible of closing the PinotDataBuffer.
}
+
+ private ImmutableRoaringBitmap getMatchesInRange(int firstRangeId, int lastRangeId) {
+ // produce bitmap of all ranges fully covered by buckets.
+ // 1. if firstRangeId is -1, the query range covers the first bucket
+ // 2. if lastRangeId is _rangeStartArray.length, the query range covers the last bucket
+ // 3. the loop isn't entered if the range ids are equal
+ MutableRoaringBitmap matching = firstRangeId < lastRangeId ? new MutableRoaringBitmap() : null;
+ for (int rangeId = firstRangeId + 1; rangeId < lastRangeId; rangeId++) {
+ matching.or(getDocIds(rangeId));
+ }
+ return matching;
+ }
+
+ private ImmutableRoaringBitmap getPartialMatchesInRange(int firstRangeId, int lastRangeId) {
+ if (isOutOfRange(firstRangeId)) {
+ return isOutOfRange(lastRangeId) ? null : getDocIds(lastRangeId);
+ }
+ if (isOutOfRange(lastRangeId)) {
+ return getDocIds(firstRangeId);
+ }
+ return ImmutableRoaringBitmap.or(getDocIds(firstRangeId), getDocIds(lastRangeId));
+ }
+
+ private boolean isOutOfRange(int docId) {
Review comment:
```suggestion
private boolean isOutOfRange(int rangeId) {
```
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/RangeIndexReaderImpl.java
##########
@@ -65,53 +65,109 @@ public RangeIndexReader(PinotDataBuffer dataBuffer) {
long rangeArrayStartOffset = offset;
_rangeStartArray = new Number[_numRanges];
- final long lastOffset = dataBuffer.getLong(offset + (_numRanges + 1) * _valueType.size() + _numRanges * Long.BYTES);
+ final long lastOffset = dataBuffer.getLong(offset + (long) (_numRanges + 1) * _valueType.size()
+ + (long) _numRanges * Long.BYTES);
- _bitmapIndexOffset = offset + (_numRanges + 1) * _valueType.size();
+ _bitmapIndexOffset = offset + (long) (_numRanges + 1) * _valueType.size();
Preconditions.checkState(lastOffset == dataBuffer.size(),
"The last offset should be equal to buffer size! Current lastOffset: " + lastOffset + ", buffer size: "
+ dataBuffer.size());
switch (_valueType) {
case INT:
for (int i = 0; i < _numRanges; i++) {
- _rangeStartArray[i] = dataBuffer.getInt(rangeArrayStartOffset + i * Integer.BYTES);
+ _rangeStartArray[i] = dataBuffer.getInt(rangeArrayStartOffset + (long) i * Integer.BYTES);
}
- _lastRangeEnd = dataBuffer.getInt(rangeArrayStartOffset + _numRanges * Integer.BYTES);
+ _lastRangeEnd = dataBuffer.getInt(rangeArrayStartOffset + (long) _numRanges * Integer.BYTES);
break;
case LONG:
for (int i = 0; i < _numRanges; i++) {
- _rangeStartArray[i] = dataBuffer.getLong(rangeArrayStartOffset + i * Long.BYTES);
+ _rangeStartArray[i] = dataBuffer.getLong(rangeArrayStartOffset + (long) i * Long.BYTES);
}
- _lastRangeEnd = dataBuffer.getLong(rangeArrayStartOffset + _numRanges * Long.BYTES);
+ _lastRangeEnd = dataBuffer.getLong(rangeArrayStartOffset + (long) _numRanges * Long.BYTES);
break;
case FLOAT:
for (int i = 0; i < _numRanges; i++) {
- _rangeStartArray[i] = dataBuffer.getFloat(rangeArrayStartOffset + i * Float.BYTES);
+ _rangeStartArray[i] = dataBuffer.getFloat(rangeArrayStartOffset + (long) i * Float.BYTES);
}
- _lastRangeEnd = dataBuffer.getFloat(rangeArrayStartOffset + _numRanges * Float.BYTES);
+ _lastRangeEnd = dataBuffer.getFloat(rangeArrayStartOffset + (long) _numRanges * Float.BYTES);
break;
case DOUBLE:
for (int i = 0; i < _numRanges; i++) {
- _rangeStartArray[i] = dataBuffer.getDouble(rangeArrayStartOffset + i * Double.BYTES);
+ _rangeStartArray[i] = dataBuffer.getDouble(rangeArrayStartOffset + (long) i * Double.BYTES);
}
- _lastRangeEnd = dataBuffer.getDouble(rangeArrayStartOffset + _numRanges * Double.BYTES);
+ _lastRangeEnd = dataBuffer.getDouble(rangeArrayStartOffset + (long) _numRanges * Double.BYTES);
break;
default:
throw new RuntimeException("Range Index Unsupported for dataType:" + _valueType);
}
}
- @VisibleForTesting
- public Number[] getRangeStartArray() {
- return _rangeStartArray;
+ /**
+ * {@inheritDoc}
+ */
+ @Override
Review comment:
Also annotate the implementations as nullable
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/RangeIndexReaderImpl.java
##########
@@ -164,57 +220,71 @@ private long getOffset(final int rangeId) {
* @param value
* @return
*/
- public int findRangeId(int value) {
+ private int findRangeId(int value) {
for (int i = 0; i < _rangeStartArray.length; i++) {
if (value < _rangeStartArray[i].intValue()) {
return i - 1;
}
}
- if (value <= _lastRangeEnd.intValue()) {
- return _rangeStartArray.length - 1;
- }
- return -1;
+ return value <= _lastRangeEnd.intValue() ? _rangeStartArray.length - 1 : _rangeStartArray.length;
}
- public int findRangeId(long value) {
+ private int findRangeId(long value) {
for (int i = 0; i < _rangeStartArray.length; i++) {
if (value < _rangeStartArray[i].longValue()) {
return i - 1;
}
}
- if (value <= _lastRangeEnd.longValue()) {
- return _rangeStartArray.length - 1;
- }
- return -1;
+ return value <= _lastRangeEnd.longValue() ? _rangeStartArray.length - 1 : _rangeStartArray.length;
}
- public int findRangeId(float value) {
+ private int findRangeId(float value) {
for (int i = 0; i < _rangeStartArray.length; i++) {
if (value < _rangeStartArray[i].floatValue()) {
return i - 1;
}
}
- if (value <= _lastRangeEnd.floatValue()) {
- return _rangeStartArray.length - 1;
- }
- return -1;
+ return value <= _lastRangeEnd.floatValue() ? _rangeStartArray.length - 1 : _rangeStartArray.length;
}
- public int findRangeId(double value) {
+ private int findRangeId(double value) {
for (int i = 0; i < _rangeStartArray.length; i++) {
if (value < _rangeStartArray[i].doubleValue()) {
return i - 1;
}
}
- if (value <= _lastRangeEnd.doubleValue()) {
- return _rangeStartArray.length - 1;
- }
- return -1;
+ return value <= _lastRangeEnd.doubleValue() ? _rangeStartArray.length - 1 : _rangeStartArray.length;
}
@Override
public void close() {
// NOTE: DO NOT close the PinotDataBuffer here because it is tracked by the caller and might be reused later. The
// caller is responsible of closing the PinotDataBuffer.
}
+
+ private ImmutableRoaringBitmap getMatchesInRange(int firstRangeId, int lastRangeId) {
+ // produce bitmap of all ranges fully covered by buckets.
+ // 1. if firstRangeId is -1, the query range covers the first bucket
+ // 2. if lastRangeId is _rangeStartArray.length, the query range covers the last bucket
+ // 3. the loop isn't entered if the range ids are equal
+ MutableRoaringBitmap matching = firstRangeId < lastRangeId ? new MutableRoaringBitmap() : null;
Review comment:
```suggestion
MutableRoaringBitmap matching = firstRangeId < (lastRangeId - 1) ? new MutableRoaringBitmap() : null;
```
##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/reader/RangeIndexReader.java
##########
@@ -0,0 +1,91 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.spi.index.reader;
+
+import java.io.Closeable;
+
+/**
+ * Interface for indexed range queries
+ * @param <T>
+ */
+public interface RangeIndexReader<T> extends Closeable {
Review comment:
^^
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/creator/RangeIndexCreatorTest.java
##########
@@ -106,23 +109,22 @@ private void testDataType(DataType dataType)
throws IOException {
FieldSpec fieldSpec = new DimensionFieldSpec(COLUMN_NAME, dataType, true);
int numDocs = 1000;
- Number[] values = new Number[numDocs];
+ Object values = valuesArray(dataType, numDocs);
+ int numValuesPerRange;
try (RangeIndexCreator creator = new RangeIndexCreator(INDEX_DIR, fieldSpec, dataType, -1, -1, numDocs, numDocs)) {
addDataToIndexer(dataType, numDocs, 1, creator, values);
creator.seal();
+ // account for off by one bug in v1 implementation
+ numValuesPerRange = creator.getNumValuesPerRange() + 1;
Review comment:
I don't think it is off by one in the implementation. The difference is that in the implementation, the same value will always go to the same range id (line 286), which is not align with the setup in this test
##########
File path: pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/creator/RangeIndexCreatorTest.java
##########
@@ -242,141 +242,352 @@ private void addDataToIndexer(DataType dataType, int numDocs, int numValuesPerEn
}
}
- private void checkValueForDocId(DataType dataType, Number[] values, Number[] rangeStartArray, int rangeId, int docId,
+ private void verifyRangesForDataType(DataType dataType, Object values, Object ranges, int numValuesPerMVEntry,
+ RangeIndexReader<ImmutableRoaringBitmap> rangeIndexReader) {
+ switch (dataType) {
+ case INT: {
+ // single bucket ranges
+ int rangeId = 0;
+ for (int[] range : (int[][]) ranges) {
+ assertNull(rangeIndexReader.getMatchingDocIds(range[0], range[1]),
+ "range index can't guarantee match within a single range");
+ ImmutableRoaringBitmap partialMatches = rangeIndexReader.getPartiallyMatchingDocIds(range[0], range[1]);
+ assertNotNull(partialMatches, "partial matches for single range must not be null");
+ for (int docId : partialMatches.toArray()) {
+ checkValueForDocId(dataType, values, ranges, rangeId, docId, numValuesPerMVEntry);
+ }
+ ++rangeId;
+ }
+ // multi bucket ranges
+ int[] lowerPartialRange = ((int[][]) ranges)[0];
+ int[] coveredRange = ((int[][]) ranges)[1];
+ int[] upperPartialRange = ((int[][]) ranges)[2];
+ ImmutableRoaringBitmap matches = rangeIndexReader.getMatchingDocIds(lowerPartialRange[0], upperPartialRange[1]);
+ assertNotNull(matches, "matches for covered range must not be null");
+ for (int docId : matches.toArray()) {
+ checkValueForDocId(dataType, values, ranges, 1, docId, numValuesPerMVEntry);
+ }
+ assertEquals(matches, rangeIndexReader.getPartiallyMatchingDocIds(coveredRange[0], coveredRange[1]));
+ // partial matches must be the combination of the two edge buckets
+ ImmutableRoaringBitmap partialMatches = rangeIndexReader.getPartiallyMatchingDocIds(
+ lowerPartialRange[0], upperPartialRange[1]);
+ assertNotNull(partialMatches, "partial matches for single range must not be null");
+ assertEquals(ImmutableRoaringBitmap.or(
+ rangeIndexReader.getPartiallyMatchingDocIds(lowerPartialRange[0], lowerPartialRange[1]),
+ rangeIndexReader.getPartiallyMatchingDocIds(upperPartialRange[0], upperPartialRange[1])), partialMatches);
+ // edge cases
+ assertEquals(((int[]) values).length, numValuesPerMVEntry
Review comment:
We should set a bound for int and long to ensure the random value is not MIN or MAX
--
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 edited a comment on pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#issuecomment-919947162
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7435](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8110c14) into [master](https://codecov.io/gh/apache/pinot/commit/ad2e0632fa8d8ab84316d25ce2cd4c91c18b11bf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ad2e063) will **decrease** coverage by `9.49%`.
> The diff coverage is `52.23%`.
[](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7435 +/- ##
============================================
- Coverage 71.90% 62.41% -9.50%
+ Complexity 3429 3363 -66
============================================
Files 1517 1508 -9
Lines 75118 74800 -318
Branches 10950 10917 -33
============================================
- Hits 54013 46685 -7328
- Misses 17470 24736 +7266
+ Partials 3635 3379 -256
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `29.10% <22.38%> (-0.07%)` | :arrow_down: |
| unittests1 | `69.78% <29.85%> (+0.06%)` | :arrow_up: |
| unittests2 | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `58.26% <ø> (ø)` | |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `88.88% <0.00%> (ø)` | |
| [...local/segment/index/datasource/BaseDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvQmFzZURhdGFTb3VyY2UuamF2YQ==) | `95.83% <ø> (ø)` | |
| [...al/segment/index/datasource/MutableDataSource.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2RhdGFzb3VyY2UvTXV0YWJsZURhdGFTb3VyY2UuamF2YQ==) | `69.23% <ø> (ø)` | |
| [...ent/virtualcolumn/VirtualColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3ZpcnR1YWxjb2x1bW4vVmlydHVhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `90.47% <ø> (ø)` | |
| [...operator/filter/RangeIndexBasedFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvUmFuZ2VJbmRleEJhc2VkRmlsdGVyT3BlcmF0b3IuamF2YQ==) | `35.71% <36.58%> (-8.97%)` | :arrow_down: |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `72.07% <78.26%> (ø)` | |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `80.63% <100.00%> (+0.08%)` | :arrow_up: |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/controller/recommender/io/ConfigManager.java](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9pby9Db25maWdNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [278 more](https://codecov.io/gh/apache/pinot/pull/7435/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ad2e063...8110c14](https://codecov.io/gh/apache/pinot/pull/7435?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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 change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r709631211
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/RangeIndexReaderImpl.java
##########
@@ -217,4 +272,26 @@ public void close() {
// NOTE: DO NOT close the PinotDataBuffer here because it is tracked by the caller and might be reused later. The
// caller is responsible of closing the PinotDataBuffer.
}
+
+ private ImmutableRoaringBitmap getMatchesInRange(int firstRangeId, int lastRangeId) {
+ if (firstRangeId == lastRangeId) {
+ return null;
+ }
+ MutableRoaringBitmap matching = new MutableRoaringBitmap();
+ for (int rangeId = firstRangeId + 1; rangeId < lastRangeId; ++rangeId) {
Review comment:
Based on my reading, the `findRangeId()` will return `-1` if the value is smaller than the lower bound of the lowest bucket or larger than the higher bound of the highest bucket. What if the `lastRangeId == -1` because it is larger than the higher bound?
--
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] richardstartin commented on a change in pull request #7435: refactor RangeIndexReader to make it easier to evolve its implementation
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7435:
URL: https://github.com/apache/pinot/pull/7435#discussion_r709628801
##########
File path: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/RangeIndexReaderImpl.java
##########
@@ -217,4 +272,26 @@ public void close() {
// NOTE: DO NOT close the PinotDataBuffer here because it is tracked by the caller and might be reused later. The
// caller is responsible of closing the PinotDataBuffer.
}
+
+ private ImmutableRoaringBitmap getMatchesInRange(int firstRangeId, int lastRangeId) {
+ if (firstRangeId == lastRangeId) {
+ return null;
+ }
+ MutableRoaringBitmap matching = new MutableRoaringBitmap();
+ for (int rangeId = firstRangeId + 1; rangeId < lastRangeId; ++rangeId) {
Review comment:
No, this is correct. `firstRangeId == -1` means that the lower bound was below the lowest bucket, so range 0 (`firstRangeId + 1`) is within the range of the query so long as `lastRangeId > 0`. Similarly, a `lastRangeId == -1` means that the upper bound of the query was below the first range, which means that none of the rows are within the range of the query, so an empty bitmap is produced.
--
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