Copilot commented on code in PR #18920:
URL: https://github.com/apache/pinot/pull/18920#discussion_r3669206059
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/JsonIndexHandler.java:
##########
@@ -65,12 +71,43 @@ public boolean needUpdateIndices(SegmentDirectory.Reader
segmentReader) {
String segmentName = _segmentDirectory.getSegmentMetadata().getName();
Set<String> columnsToAddIdx = new HashSet<>(_jsonIndexConfigs.keySet());
Set<String> existingColumns =
segmentReader.toSegmentDirectory().getColumnsWithIndex(StandardIndexes.json());
- // Check if any existing index need to be removed.
+ // Load metadata properties once to avoid repeated disk reads when
checking multiple columns.
+ PropertiesConfiguration properties = loadMetadataProperties();
+ // Check if any existing index need to be removed or if the config changed.
for (String column : existingColumns) {
if (!columnsToAddIdx.remove(column)) {
LOGGER.info("Need to remove existing json index from segment: {},
column: {}", segmentName, column);
return true;
}
+ // Column exists in both existing indexes and config; check if config
changed.
+ // When properties != null but storedConfig is null the index predates
config persistence (legacy segment):
+ // only backfill (rebuild) when the forward index is present or
reconstructable. If the forward index is
+ // absent (e.g. forwardIndexDisabled=true with no dictionary/inverted
pair), preserve the existing JSON
+ // index untouched — createForwardIndexIfNeeded() would fail with no
dictionary/inverted index available.
+ JsonIndexConfig currentConfig = _jsonIndexConfigs.get(column);
+ JsonIndexConfig storedConfig = readStoredJsonIndexConfig(column,
properties);
+ if (properties != null) {
+ // configChanged: stored config exists and differs from current —
rebuild regardless of forward
+ // index presence (same pre-existing behaviour;
createForwardIndexIfNeeded will fail if the
+ // forward index is truly unrecoverable, which requires an explicit
segment refresh).
+ boolean configChanged = storedConfig != null &&
!storedConfig.equals(currentConfig);
+ // legacyBackfill: stored config is absent (either the index predates
config persistence, or
+ // the stored value failed to deserialize and
readStoredJsonIndexConfig returned null). Only
+ // backfill when the forward index is present or reconstructable; if
it is absent
+ // (forwardIndexDisabled=true with no dictionary/inverted pair)
createForwardIndexIfNeeded()
+ // would throw, so preserve the existing JSON index instead.
+ boolean legacyBackfill = storedConfig == null &&
isForwardIndexAvailable(segmentReader, column);
+ if (configChanged) {
+ LOGGER.info("Need to rebuild json index for segment: {}, column: {}
due to config change", segmentName,
+ column);
+ return true;
+ }
Review Comment:
`needUpdateIndices()` will return true for `configChanged` even when the
forward index is not recoverable. This can cause
`SegmentPreProcessor.needProcess()` to run preprocessing and then fail during
`updateIndices()` after deleting the existing JSON index. Consider guarding
`configChanged` with the same forward-index-availability check used for legacy
backfill (and WARN when the config can’t be applied).
This issue also appears on line 147 of the same file.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/JsonIndexHandler.java:
##########
@@ -186,8 +320,13 @@ private void
handleNonDictionaryBasedColumn(SegmentDirectory.Writer segmentWrite
ForwardIndexReaderContext readerContext =
forwardIndexReader.createContext();
JsonIndexCreator jsonIndexCreator =
StandardIndexes.json().createIndexCreator(context, config)) {
int numDocs = columnMetadata.getTotalDocs();
+ boolean isMapType = columnMetadata.getDataType() == DataType.MAP;
for (int i = 0; i < numDocs; i++) {
- jsonIndexCreator.add(forwardIndexReader.getString(i, readerContext));
+ // MAP columns store binary-encoded data; getString() returns garbled
bytes.
+ // Use getMap() + MapUtils.toString() to produce the JSON string the
index creator expects.
+ String value = isMapType ?
MapUtils.toString(forwardIndexReader.getMap(i, readerContext))
+ : forwardIndexReader.getString(i, readerContext);
+ jsonIndexCreator.add(value);
Review Comment:
`MapUtils.toString(map)` defaults to sorting keys (including nested), which
can add noticeable CPU overhead in this per-document loop. JSON object key
order is not semantically significant for indexing, so using the unsorted
writer should be sufficient and faster.
--
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]
