Copilot commented on code in PR #19046:
URL: https://github.com/apache/pinot/pull/19046#discussion_r3669215963
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/VectorIndexHandler.java:
##########
@@ -545,6 +551,58 @@ && hasCombinedFile(indexDir, column, desiredBackend)) {
}
}
+ /**
+ * Checks whether the HNSW vector index config has changed by comparing the
metadata file written at index-creation
+ * time with the current {@link VectorIndexConfig}.
+ *
+ * <p>The metadata file (written by {@code HnswVectorIndexCreator}) records
the vector dimension and
+ * {@link VectorSimilarityFunction} used to build the index. Changing either
field
+ * produces a structurally incompatible index (wrong vector size) or
silently wrong query results (wrong distance
+ * metric), so a rebuild is required.
+ *
+ * <p>Legacy segments built before this metadata was introduced have no
metadata file; config-change detection is
+ * skipped for those segments (the file will be created the next time the
index is rebuilt for any reason).
+ */
+ private boolean isVectorConfigChanged(File indexDir, String column,
VectorIndexConfig desiredConfig,
+ String segmentName) {
+ File segmentDirectory = SegmentDirectoryPaths.segmentDirectoryFor(indexDir,
+ _segmentDirectory.getSegmentMetadata().getVersion());
+ Properties metadata =
VectorIndexUtils.readVectorIndexMetadata(segmentDirectory, column);
+ if (metadata == null) {
+ // No metadata file — legacy segment; skip config-change detection.
+ return false;
+ }
+ try {
+ int storedDimension =
Integer.parseInt(metadata.getProperty("dimension"));
+ int desiredDimension = desiredConfig.getVectorDimension();
+ if (storedDimension != desiredDimension) {
+ LOGGER.info("Vector index config changed for segment: {}, column: {} "
+ + "(dimension: {} → {}). Index needs to be rebuilt.",
+ segmentName, column, storedDimension, desiredDimension);
+ return true;
+ }
+
+ VectorSimilarityFunction storedFunc =
+
VectorSimilarityFunction.valueOf(metadata.getProperty("distanceFunction"));
+ VectorIndexConfig.VectorDistanceFunction desiredDistanceFunc =
desiredConfig.getVectorDistanceFunction();
+ // Null distanceFunction defaults to COSINE (see
VectorIndexConfig.DEFAULT_VECTOR_DISTANCE_FUNCTION).
+ if (desiredDistanceFunc == null) {
+ desiredDistanceFunc = VectorIndexConfig.VectorDistanceFunction.COSINE;
+ }
+ VectorSimilarityFunction desiredFunc =
VectorIndexUtils.toSimilarityFunction(desiredDistanceFunc);
+ if (storedFunc != desiredFunc) {
+ LOGGER.info("Vector index config changed for segment: {}, column: {} "
+ + "(distanceFunction: {} → {}). Index needs to be rebuilt.",
+ segmentName, column, storedFunc, desiredFunc);
+ return true;
+ }
+ } catch (Exception e) {
+ LOGGER.warn("Failed to parse vector index metadata for segment: {},
column: {}; skipping config-change check",
+ segmentName, column, e);
+ }
Review Comment:
If the metadata file exists but is unreadable/corrupt or missing expected
keys, the current behavior is to log a warning and then *skip* the
config-change check (returning false). That can leave an incompatible index in
place and produce wrong results when dimension/distance changes. Safer behavior
is to treat parse failure as requiring a rebuild (the rebuild will also rewrite
a clean metadata file).
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/vector/HnswVectorIndexCreator.java:
##########
@@ -110,6 +110,8 @@ public void seal() {
try {
LOGGER.info("Sealing HNSW index for column: {}", _vectorColumn);
_indexWriter.forceMerge(1);
+ VectorIndexUtils.writeVectorIndexMetadata(_segmentIndexDir,
_vectorColumn, _vectorDimension,
+ _vectorSimilarityFunction);
Review Comment:
The metadata file is written to the current (V1/V2) segment build directory,
but offline segment generation is documented here as always V1 and then
converted to V3. The V1/V2→V3 converter copies the Lucene HNSW directory but
does not copy sibling files like `*.vector.hnsw.metadata`, and then deletes all
files in the V1/V2 directory — so the metadata will be dropped for V3 segments
and config-drift detection will be silently skipped.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/invertedindex/VectorIndexHandler.java:
##########
@@ -496,7 +501,8 @@ public void updateIndices(SegmentDirectory.Writer
segmentWriter)
VectorBackendType desiredBackend = desiredConfig.resolveBackendType();
VectorBackendType existingBackend =
VectorIndexUtils.detectVectorIndexBackend(indexDir, column);
// Storage-format comparison — see the twin check in needUpdateIndices.
- if (existingBackend != null && existingBackend !=
VectorIndexUtils.storageFormatOf(desiredBackend)) {
+ if ((existingBackend != null && existingBackend !=
VectorIndexUtils.storageFormatOf(desiredBackend))
+ || isVectorConfigChanged(indexDir, column, desiredConfig,
segmentName)) {
LOGGER.info("Rebuilding Vector index for segment: {}, column: {}
(backend changed from {} to {})",
segmentName, column, existingBackend, desiredBackend);
Review Comment:
This log line says the backend changed, but the rebuild can also be
triggered by `isVectorConfigChanged(...)` (dimension/distance change) even when
the backend stays the same. The current message can be misleading during
ops/debugging (e.g., logs show "backend changed from HNSW to HNSW").
--
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]
