dengfengjiang77 opened a new issue, #16850: URL: https://github.com/apache/lucene/issues/16850
### Description Follow-up from #16674 , @mikemccand [pointed out](https://github.com/apache/lucene/issues/16674#issuecomment-6018767712) that [checkIntegrity](https://github.com/apache/lucene/blob/8476457b8752be2fd8094eaf5cd9bf0cea9594ae/lucene/core/src/java/org/apache/lucene/codecs/KnnVectorsWriter.java#L123-L131) does another full read of `.vec` before [raw copy](https://github.com/apache/lucene/blob/8476457b8752be2fd8094eaf5cd9bf0cea9594ae/lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99FlatVectorsWriter.java#L300-L331), and that it is nearly redundant, since steps 1-2 could verify the checksum while they read. The PR #16839 contribute to remove centroid pass[step 2], we need dig into if we have chance to simplify checkIntegrity as step 0. Here are proposal changes: - Add a merge-specific check to `KnnVectorsReader`, the existing merge argument only makes the [check abortable](https://github.com/apache/lucene/blob/8476457b8752be2fd8094eaf5cd9bf0cea9594ae/lucene/core/src/java/org/apache/lucene/codecs/CodecUtil.java#L654), so it can't be reused to mean 'skip'. - The flat reader overrides it: while the merge reads `.vec`, it also computes the file's checksum from start to end , reading any bytes the merge skips(for example: deleted vectors), and compares it with the footer at the end. - If the merge does not read in file order, fall back to the full check. - No check `.veq` during merges. since step 3 recomputes it from `.vec`. We will keep the full check for `.vex`. ```text Reads of the source files in one merge, for segments with deletes step 0 step 1 step 2 step 3 .vec reads checkIntegrity raw copy centroid quantization main .vec .veq .vec .vec .vec 4 #16839 .vec .veq .vec, centroid - .vec 3 this issue - .vec, centroid, checksum - .vec 2 "this issue" assumes #16839 is merged. .vex (HNSW only) keeps its full check in step 0. ``` Test proposal: flip one byte in each part of a source `.vec` and check if every merge fails. For merge performance, compare real force merges on Cohere 1M x 1024 between #16839 and this change , with a cold and a warm page cache , with random and block deletes, after that check both merge time and bytes read from disk. Some ideas in foresee risks and mitigation plan: - Error detection timing[@mikemccand mentioned]: `.vec` corruption is found during the raw copy instead of in step 0. The outcome is the same: the merge exception is [tragic](https://github.com/apache/lucene/blob/8476457b8752be2fd8094eaf5cd9bf0cea9594ae/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java#L5109-L5113), `IndexWriter` closes, and the last commit stays intact (tested locally). Only more merge work is wasted. - Missed bytes: Without step 0, corrupt live vectors are copied silently. so the new check must cover every byte, mitigation here is we can verify all the bytes up to the footer were read, otherwise fallback to the full check. - `.veq` corruption is no longer reported. This is a tradeoff part, the merged segment is still correct, but the source segment is deleted after the merge, so the corruption is never reported. Keeping the check costs 2.2 s cold. Questions: 1. Is finding `.vec` corruption during the raw copy, instead of up front, acceptable? 2. OK to drop the `.veq` check during merges? 3. Is a separate merge-specific method the right shape? Thanks a lot for the feedback ❤️!! And I will list a summary table show that what process can be 'skip' or replaced in other steps, check it one by one to show the pro and cons, hope I can have a better optimization design later. -- 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]
