extend SegmentDirectory.Reader iface to abstract access to index buffers from StarTree index #10158
Conversation
|
|
||
| public abstract String toString(); | ||
|
|
||
| public PinotDataBuffer getStarTreeIndex() |
There was a problem hiding this comment.
fyi, getStarTreeIndex and getStarTreeIndexMap were added very recenlty by #9828 but they didn't abstract the access to ST index very cleanly and not very generic. This PR improves on this.
…ers from StarTree index
klsince
force-pushed
the
segment_reader_for_st_index
branch
from
43ffdea to
0fc2506
Compare
Codecov Report
@@ Coverage Diff @@
## master #10158 +/- ##
=============================================
+ Coverage 34.84% 70.39% +35.55%
- Complexity 200 5804 +5604
=============================================
Files 2006 2018 +12
Lines 108718 109001 +283
Branches 16501 16541 +40
=============================================
+ Hits 37880 76734 +38854
+ Misses 67566 26891 -40675
- Partials 3272 5376 +2104
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -248,25 +250,26 @@ protected void load() | |||
|
|
|||
| private synchronized void loadData() | |||
| throws IOException { | |||
| if (_columnIndexDirectory != null) { | |||
| return; | |||
| if (_columnIndexDirectory == null) { | |||
There was a problem hiding this comment.
Suggest reverting this part. We do the check to prevent loadData() twice. We only need to add if (CollectionUtils.isNotEmpty(_segmentMetadata.getStarTreeV2MetadataList()) {...}
There was a problem hiding this comment.
The new changes continue to ensure _columnIndexDirectory (and _starTreeIndexReader) is loaded once. I separate their handling because the SegmentDirectory.Writer closes and sets _columnIndexDirectory to null, but Writer doesn't need to close and set _starTreeIndexReader to null, as it is for readonly.
There was a problem hiding this comment.
Hmm, it doesn't seem correct to me. When closing the writer, we should close the star-tree as well
There was a problem hiding this comment.
I think SegmentDirectory.close() is supposed to release everything owned by the directory, and IMO the SegmentDirectory..Writer.close() is more about to flush any changes, but _starTreeIndexReader is for read only.
There was a problem hiding this comment.
But after writer.close() is called, I don't think we can read the segment anymore.
The reader/writer name can be misleading as they are not independent of the segment directory. Unless I am missing something?
There was a problem hiding this comment.
The comment about SegmentDirectory interface suggests that's possible, but need to call createReader() to get a new reader. The buffers are not loaded redundantly with the null-checks in loadData() method.
We only need to add if (CollectionUtils.isNotEmpty(_segmentMetadata.getStarTreeV2MetadataList()) {...}
Then I should close _starTreeIndexReader in Writer.close() method too. I'm fine with that way.
| return _starTreeIndexReader != null; | ||
| } | ||
|
|
||
| public SegmentDirectory.Reader getSegmentIndexReaderFor(String indexName, SegmentIndexType type) { |
There was a problem hiding this comment.
I don't think this is a common abstraction for segment level index. Suggest for now just add APIs for star-tree: getStarTreeReader(int starTreeId)
Move the access of StarTree index data buffers behind the SegmentDirectory interface, so that the access to ST index data can be customized via custom SegmentDirectory implementations.