You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/02/21 12:05:51 UTC
[GitHub] [incubator-pinot] siddharthteotia opened a new pull request #5087:
Support creation of inverted index on a new column in V1 segment format
format as part of segment reload
siddharthteotia opened a new pull request #5087: Support creation of inverted index on a new column in V1 segment format format as part of segment reload
URL: https://github.com/apache/incubator-pinot/pull/5087
This is a bug fix. As part of working on PR https://github.com/apache/incubator-pinot/pull/5074, I was adding tests for supporting segment reload of text indexes for both V1 and V3 segment formats. The test was constantly failing when the format is V1, a new column is added and text index is enabled.
It looks like there is underlying bug due to which this was happening and the exact same scenario doesn't seem to work with inverted index as well. Very easily reproducible in SegmentPreprocessorTest. The existing tests didn't catch it because although they were testing for adding new columns with V1, they didn't test the scenario of enabling inverted index after adding a new column in V1.
This is the stack trace:
`java.lang.NullPointerException
at org.apache.pinot.core.segment.index.SegmentMetadataImpl.getForwardIndexFileName(SegmentMetadataImpl.java:507)
at org.apache.pinot.core.segment.store.FilePerIndexDirectory.getFileFor(FilePerIndexDirectory.java:171)
at org.apache.pinot.core.segment.store.FilePerIndexDirectory.getReadBufferFor(FilePerIndexDirectory.java:145)
at org.apache.pinot.core.segment.store.FilePerIndexDirectory.getForwardIndexBufferFor(FilePerIndexDirectory.java:62)
at org.apache.pinot.core.segment.store.SegmentLocalFSDirectory.getIndexForColumn(SegmentLocalFSDirectory.java:237)
at org.apache.pinot.core.segment.store.SegmentLocalFSDirectory.access$000(SegmentLocalFSDirectory.java:42)
at org.apache.pinot.core.segment.store.SegmentLocalFSDirectory$Writer.getIndexFor(SegmentLocalFSDirectory.java:470)
at org.apache.pinot.core.segment.index.loader.invertedindex.InvertedIndexHandler.getForwardIndexReader(InvertedIndexHandler.java:145)
at org.apache.pinot.core.segment.index.loader.invertedindex.InvertedIndexHandler.createInvertedIndexForColumn(InvertedIndexHandler.java:110)
at org.apache.pinot.core.segment.index.loader.invertedindex.InvertedIndexHandler.createInvertedIndices(InvertedIndexHandler.java:73)
at org.apache.pinot.core.segment.index.loader.SegmentPreProcessor.process(SegmentPreProcessor.java:97)
at org.apache.pinot.core.segment.index.loader.SegmentPreProcessorTest.checkUpdateDefaultColumns(SegmentPreProcessorTest.java:300)
at org.apache.pinot.core.segment.index.loader.SegmentPreProcessorTest.testV1UpdateDefaultColumns(SegmentPreProcessorTest.java:275)`
In InvertedIndexHandler, when we are trying to create the inverted index for newly created column, it gets the forward index which should already exist since SegmentPreprocessor would have already updated default column values using V1DefaultColumnHandler.
The problem happens in **FilePerIndexDirectory.getForwardIndexBufferFor (column)** as it tries to get ColumnMetadata for the column from segment metadata.
ColumnMetadata is null since the segment metadata that this code is working with is stale (new columns have been added but preprocessor code didn't reload the metadata for directory).
The fix is to reload the metadata in SegmentPreprocessor for the SegmentDirectory.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia merged pull request #5087:
Support creation of inverted index on a new column in V1 segment format as
part of segment reload
Posted by GitBox <gi...@apache.org>.
siddharthteotia merged pull request #5087: Support creation of inverted index on a new column in V1 segment format as part of segment reload
URL: https://github.com/apache/incubator-pinot/pull/5087
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on issue #5087: Support
creation of inverted index on a new column in V1 segment format as part of
segment reload
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #5087: Support creation of inverted index on a new column in V1 segment format as part of segment reload
URL: https://github.com/apache/incubator-pinot/pull/5087#issuecomment-589658842
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5087?src=pr&el=h1) Report
> Merging [#5087](https://codecov.io/gh/apache/incubator-pinot/pull/5087?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/21850ba17132c52f175d85930ca561849a1218b5?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `100%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/5087?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #5087 +/- ##
============================================
+ Coverage 58.25% 58.26% +0.01%
Complexity 12 12
============================================
Files 1209 1209
Lines 64537 64541 +4
Branches 9484 9484
============================================
+ Hits 37594 37606 +12
+ Misses 24172 24166 -6
+ Partials 2771 2769 -2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5087?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...che/pinot/core/segment/store/SegmentDirectory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5087/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3N0b3JlL1NlZ21lbnREaXJlY3RvcnkuamF2YQ==) | `71.42% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...ot/core/segment/store/SegmentLocalFSDirectory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5087/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L3N0b3JlL1NlZ21lbnRMb2NhbEZTRGlyZWN0b3J5LmphdmE=) | `66.84% <100%> (+0.54%)` | `0 <0> (ø)` | :arrow_down: |
| [...core/segment/index/loader/SegmentPreProcessor.java](https://codecov.io/gh/apache/incubator-pinot/pull/5087/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L2luZGV4L2xvYWRlci9TZWdtZW50UHJlUHJvY2Vzc29yLmphdmE=) | `83.33% <100%> (+0.47%)` | `0 <0> (ø)` | :arrow_down: |
| [...er/validation/BrokerResourceValidationManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5087/diff?src=pr&el=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci92YWxpZGF0aW9uL0Jyb2tlclJlc291cmNlVmFsaWRhdGlvbk1hbmFnZXIuamF2YQ==) | `25% <0%> (-56.25%)` | `0% <0%> (ø)` | |
| [.../org/apache/pinot/client/PinotClientException.java](https://codecov.io/gh/apache/incubator-pinot/pull/5087/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Bpbm90Q2xpZW50RXhjZXB0aW9uLmphdmE=) | `33.33% <0%> (-33.34%)` | `0% <0%> (ø)` | |
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/5087/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `62% <0%> (-14%)` | `0% <0%> (ø)` | |
| [...pinot/broker/api/resources/PinotClientRequest.java](https://codecov.io/gh/apache/incubator-pinot/pull/5087/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | `23.4% <0%> (-8.52%)` | `0% <0%> (ø)` | |
| [.../realtime/impl/ThreadSafeMutableRoaringBitmap.java](https://codecov.io/gh/apache/incubator-pinot/pull/5087/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL1RocmVhZFNhZmVNdXRhYmxlUm9hcmluZ0JpdG1hcC5qYXZh) | `92.85% <0%> (-7.15%)` | `0% <0%> (ø)` | |
| [...impl/dictionary/DoubleOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5087/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT25IZWFwTXV0YWJsZURpY3Rpb25hcnkuamF2YQ==) | `47.56% <0%> (-6.1%)` | `0% <0%> (ø)` | |
| [.../impl/dictionary/LongOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/5087/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvTG9uZ09mZkhlYXBNdXRhYmxlRGljdGlvbmFyeS5qYXZh) | `73.11% <0%> (-5.38%)` | `0% <0%> (ø)` | |
| ... and [21 more](https://codecov.io/gh/apache/incubator-pinot/pull/5087/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5087?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5087?src=pr&el=footer). Last update [21850ba...95a3fc9](https://codecov.io/gh/apache/incubator-pinot/pull/5087?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org