Jackie-Jiang commented on a change in pull request #8211:
URL: https://github.com/apache/pinot/pull/8211#discussion_r808536561
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
##########
@@ -578,7 +579,11 @@ private void addNewRow(GenericRow row)
// Update inverted index
RealtimeInvertedIndexReader invertedIndex =
indexContainer._invertedIndex;
if (invertedIndex != null) {
- invertedIndex.add(dictId, docId);
+ try {
+ invertedIndex.add(dictId, docId);
Review comment:
For `RealtimeInvertedIndexReader.add()`, we should also handle the case
of `_bitmaps.size() < dictId` which is possible when the inverted index is not
updated for the previous record. We can fill empty bitmaps for the missing
`dictId`s.
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
##########
@@ -676,6 +697,14 @@ private void addNewRow(GenericRow row)
}
}
+ private void recordIndexingError(FieldConfig.IndexType indexType, Exception
exception) {
+ _logger.error("failed to index value with {}", indexType, exception);
+ if (_serverMetrics != null) {
+ String metricKeyName = _realtimeTableName + "-" + indexType +
"-indexingError";
+ _serverMetrics.addValueToTableGauge(metricKeyName,
ServerGauge.DOCUMENT_COUNT, 1);
Review comment:
This should not be a gauge, but a meter
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
##########
@@ -910,18 +939,22 @@ public void destroy() {
// Re-order documents using the inverted index
RealtimeInvertedIndexReader invertedIndex = indexContainer._invertedIndex;
- int[] docIds = new int[_numDocsIndexed];
+ int[] docIds = new int[numDocsIndexed];
+ int[] batch = new int[256];
Review comment:
Not related, but good enhancement. Suggest putting it in a separate PR
and we can directly merge?
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
##########
@@ -465,20 +468,19 @@ public void addExtraColumns(Schema newSchema) {
_logger.info("Newly added columns: " +
_newlyAddedColumnsFieldMap.toString());
}
- // NOTE: Okay for single-writer
- @SuppressWarnings("NonAtomicOperationOnVolatileField")
@Override
public boolean index(GenericRow row, @Nullable RowMetadata rowMetadata)
throws IOException {
boolean canTakeMore;
+ int numDocsIndexed = _numDocsIndexed;
Review comment:
+1
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java
##########
@@ -527,7 +530,7 @@ private void updateDictionary(GenericRow row) {
IndexContainer indexContainer = entry.getValue();
Object value = row.getValue(column);
MutableDictionary dictionary = indexContainer._dictionary;
- if (dictionary != null) {
+ if (dictionary != null && value != null) {
Review comment:
This won't work because `addNewRow()` expects value not be `null`, and
`dictId` properly set. The correct fix should be within the
`NullValueTransformer` where we put default values for all `null` fields. It
can be done in a separate PR
--
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]
