Copilot commented on code in PR #19088:
URL: https://github.com/apache/pinot/pull/19088#discussion_r3762697935
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -695,21 +696,31 @@ public boolean index(GenericRow row, @Nullable
StreamMessageMetadata metadata)
_numDocsIndexed = numDocsIndexed;
}
} else {
- // Update dictionary first
- updateDictionary(row);
-
// If metrics aggregation is enabled and if the dimension values were
already seen, this will return existing
- // docId, else this will return a new docId.
+ // docId, else this will return a new docId. Dictionary must be updated
before the rollup key is computed.
+ boolean dictHadError = updateDictionary(row);
int docId = getOrCreateDocId();
if (docId == numDocsIndexed) {
- // New row
- addNewRow(numDocsIndexed, row);
+ // New row — fail-soft complete-the-row so _numDocsIndexed and
per-column lengths stay aligned (#16316).
+ boolean rowHadError = addNewRow(numDocsIndexed, row);
+ if (dictHadError || rowHadError) {
+ recordIncompleteRow();
+ }
// Update number of documents indexed at last to make the latest row
queryable
canTakeMore = numDocsIndexed++ < _capacity;
} else {
assert isAggregateMetricsEnabled();
- aggregateMetrics(row, docId);
+ try {
+ aggregateMetrics(row, docId);
+ if (dictHadError) {
+ recordIncompleteRow();
+ }
+ } catch (Exception e) {
+ // In-place rollup already has a complete prior row; meter and keep
previous metric values.
+ recordIndexingError("AGGREGATE_METRICS", e);
+ recordIncompleteRow();
Review Comment:
`aggregateMetrics()` mutates each metric as it iterates, so this catch
cannot “keep previous metric values.” If one metric succeeds and a later
aggregator rejects its input (the multi-metric form of #16317), the first
metric remains updated while the later metric remains unchanged, silently
applying only part of the row. Compute/validate all metric updates before
committing them, or restore every previously written metric when any
aggregation fails.
This issue also appears in the following locations of the same file:
- line 924
- line 1071
- line 1162
--
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]
