Changes in Pinot segment column reader, data source, and factory to support nulls better and support valid doc IDs. #17443
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #17443 +/- ##
============================================
+ Coverage 63.29% 63.30% +0.01%
Complexity 1475 1475
============================================
Files 3162 3163 +1
Lines 188681 188692 +11
Branches 28873 28872 -1
============================================
+ Hits 119424 119459 +35
+ Misses 60007 59979 -28
- Partials 9250 9254 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…es_pinot_col_reader
…not segment columnar data source
| * @param indexSegment Source segment to read from | ||
| * | ||
| */ | ||
| public PinotSegmentColumnarDataSource(IndexSegment indexSegment, RoaringBitmap validDocIds) { |
There was a problem hiding this comment.
the backward incomaptibility is fine as this class is unused and recently added by me
| @Override | ||
| public ColumnReaderFactory createColumnReaderFactory() { | ||
| return new PinotSegmentColumnReaderFactory(_indexSegment, _initializeDefaultValueReaders); | ||
| return new PinotSegmentColumnReaderFactory(_indexSegment, true, false); |
There was a problem hiding this comment.
we're setting these flags by default based on their appropriate use case.
No client is yet using PinotSegmentColumnarDataSource for this to be a problem.
| public void close() | ||
| throws IOException { | ||
| // Segment lifecycle is managed externally, so no cleanup needed here | ||
| _indexSegment.destroy(); |
There was a problem hiding this comment.
Pinot segment record reader also destroys the segment by default, thus enabling the same capability here.
| * This parameter is kept for backward compatibility and will be removed in future. | ||
| */ | ||
| public PinotSegmentColumnReaderFactory(IndexSegment indexSegment, boolean initializeDefaultValueReaders) { | ||
| public PinotSegmentColumnReaderFactory(IndexSegment indexSegment, boolean skipDefaultNullValues, |
There was a problem hiding this comment.
Adding the new variable in the middle because initialize default value readers will get deprecated in the future. Also, this is a new method added by me and not yet used, so it shouldn't cause any backward incompatibility.
…es_pinot_col_reader
krishan1390
changed the title
3 participants
Uh oh!
There was an error while loading. Please reload this page.