xiangfu0 commented on code in PR #20007:
URL: https://github.com/apache/pinot/pull/20007#discussion_r4356279956
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentIndexCreationDriverImpl.java:
##########
@@ -296,14 +297,23 @@ public void build()
for (GenericRow row : result.getTransformedRows()) {
_indexCreator.indexRow(row);
+ numDocsIndexed++;
}
_totalIndexTimeNs += System.nanoTime() - recordReadStopTimeNs;
_incompleteRowsFound += result.getIncompleteRowCount();
_skippedRowsFound += result.getSkippedRowCount();
_sanitizedRowsFound += result.getSanitizedRowCount();
}
+ // The stats pass sized the dictionaries and wrote total docs into the
metadata. If the index pass read a
+ // different number of rows, some dictionary ids own no document and the
indexes disagree with the metadata.
+ if (numDocsIndexed != _totalDocs) {
+ throw new IllegalStateException(
+ "Segment: " + _config.getSegmentName() + " of table: " +
_config.getTableName() + " indexed "
Review Comment:
Nice guard. Worth noting this count check lives only in the row-based
`build()` path, not in `buildByColumn()` or `buildColumnar()`. That seems
intentional (those don't re-read rows through the two passes the same way, and
the sorted-index close() check still protects them), but a one-line comment
saying so would save the next reader from wondering if it was an omission.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentIndexCreationDriverImpl.java:
##########
@@ -296,14 +297,23 @@ public void build()
for (GenericRow row : result.getTransformedRows()) {
_indexCreator.indexRow(row);
+ numDocsIndexed++;
}
_totalIndexTimeNs += System.nanoTime() - recordReadStopTimeNs;
_incompleteRowsFound += result.getIncompleteRowCount();
_skippedRowsFound += result.getSkippedRowCount();
_sanitizedRowsFound += result.getSanitizedRowCount();
}
Review Comment:
One edge case for `continueOnError`: if the record reader is
non-deterministic and drops different rows on each pass, this will now fail the
build rather than silently produce a corrupt segment. That's the correct
outcome, but it's a behavior change for anyone who was relying on best-effort
builds over a flaky reader. Might be worth a sentence in the PR description so
it's not a surprise.
--
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]
