xiangfu0 commented on code in PR #19955:
URL: https://github.com/apache/pinot/pull/19955#discussion_r4343883808
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -1238,12 +1240,21 @@ private class ValueReader implements Closeable {
} else {
_dictionary = null;
}
- _columnReader = new
PinotSegmentColumnReader(columnMetadata.getColumnName(), _forwardIndexReader,
_dictionary,
- null, columnMetadata.getMaxNumberOfMultiValues());
+ // A null source value is stored as the column's default value. Read the
null vector too, so the function sees
+ // null for those docs, as it does at ingestion, instead of deriving a
non-null value from the placeholder.
+ String column = columnMetadata.getColumnName();
+ NullValueVectorReaderImpl nullValueVectorReader = null;
+ if (_segmentWriter.hasIndexFor(column,
StandardIndexes.nullValueVector())) {
+ nullValueVectorReader =
+ new NullValueVectorReaderImpl(_segmentWriter.getIndexFor(column,
StandardIndexes.nullValueVector()));
+ }
+ _columnReader = new PinotSegmentColumnReader(column,
_forwardIndexReader, _dictionary, nullValueVectorReader,
+ columnMetadata.getMaxNumberOfMultiValues());
}
+ @Nullable
Object getValue(int docId) {
- return _columnReader.getValue(docId);
+ return _columnReader.isNull(docId) ? null :
_columnReader.getValue(docId);
Review Comment:
Correct and consistent with `isNull` returning `false` when there is no null
vector, so null-handling-disabled stays unchanged. One behavior note worth
calling out in the release notes: for existing segments with null handling on,
a reload now flips previously non-null derived values (computed from the
placeholder) to null. That's the intended fix toward ingestion parity, just a
visible change on reload.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java:
##########
@@ -1238,12 +1240,21 @@ private class ValueReader implements Closeable {
} else {
_dictionary = null;
}
- _columnReader = new
PinotSegmentColumnReader(columnMetadata.getColumnName(), _forwardIndexReader,
_dictionary,
- null, columnMetadata.getMaxNumberOfMultiValues());
+ // A null source value is stored as the column's default value. Read the
null vector too, so the function sees
+ // null for those docs, as it does at ingestion, instead of deriving a
non-null value from the placeholder.
+ String column = columnMetadata.getColumnName();
+ NullValueVectorReaderImpl nullValueVectorReader = null;
+ if (_segmentWriter.hasIndexFor(column,
StandardIndexes.nullValueVector())) {
Review Comment:
Minor, pre-existing: if this `new NullValueVectorReaderImpl(...)` (or the
loop in `createDerivedColumnV1Indices` that builds the readers) throws partway,
the already-opened forward index / dictionary / earlier `ValueReader`s aren't
closed, since the `finally` only covers readers that made it into the list.
Your change adds one more throwing call in the constructor, so it slightly
widens that window. Not blocking, but a try/catch cleanup in the ctor (and
closing partial readers in the build loop) would be a nice follow-up.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
