Read legacy text index directory without probing consolidated entry when it exists#18919
Open
xiangfu0 wants to merge 1 commit into
Open
Read legacy text index directory without probing consolidated entry when it exists#18919xiangfu0 wants to merge 1 commit into
xiangfu0 wants to merge 1 commit into
Conversation
…hen it exists With storeInSegmentFile=true, the text index reader factory probed the consolidated segment-file entry (getIndexFor) unconditionally. On a V1/V2 segment backed by FilePerIndexDirectory, getIndexFor resolves the still-present legacy Lucene text-index DIRECTORY and mapForReads rejects it (IllegalArgumentException: must be a regular file), killing the segment load before the legacy fallback could run — making the flag non-rolling-upgrade-safe for existing text segments. TextIndexHandler cannot rescue these segments because its combined-format conversion is v3-gated. The factory now checks the on-disk artifact first: if it is a Lucene directory (an unmigrated V3 segment, or any V1/V2 segment), read it directly and skip the probe entirely. Same pattern as the vector index fix in apache#18852. Tests cover both the V1 root layout and the v3/ subdirectory layout, asserting the probe never runs while the legacy directory exists.
xiangfu0
force-pushed
the
fix-text-index-legacy-directory-probe
branch
from
cb8749f to
12939e2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18919 +/- ##
============================================
+ Coverage 64.80% 64.81% +0.01%
Complexity 1347 1347
============================================
Files 3392 3396 +4
Lines 211769 212504 +735
Branches 33345 33485 +140
============================================
+ Hits 137232 137745 +513
- Misses 63451 63598 +147
- Partials 11086 11161 +75
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
With
storeInSegmentFile=true,TextIndexType.ReaderFactoryprobed the consolidated segment-file entry (segmentReader.getIndexFor(column, StandardIndexes.text())) unconditionally.On a V1/V2 segment backed by
FilePerIndexDirectorywhose column still has the legacy Lucene text-index directory on disk,getFileForresolves that directory (File.exists()is true for directories) andmapForReadsrejects it withIllegalArgumentException: File: ... must be a regular file— killing the whole segment load before any fallback can run. This makes flippingstoreInSegmentFile=truenon-rolling-upgrade-safe for existing V1/V2 text segments.TextIndexHandlercannot rescue these segments either: its combined-format conversion is v3-gated, so the reader factory must handle them.This is the text-index twin of the vector index bug fixed in #18852.
Fix
Legacy-directory-first gate, same pattern as #18852: if
SegmentDirectoryPaths.findTextIndexIndexFilefinds a Lucene directory on disk (an unmigrated V3 segment, or any V1/V2 segment), construct the directory-basedLuceneTextIndexReaderdirectly and skip the consolidated probe entirely. The probe only runs when no legacy directory exists.Tests
New
TextIndexTypeTestmirrors the vector regression tests:testReaderFactoryUsesLegacyTextDirectoryWithoutProbingConsolidatedEntry— builds a real legacy Lucene text index in the V1 root layout, stubsgetIndexForto throwIllegalArgumentException("... must be a regular file"), asserts the reader is constructed, serves queries, and the probe never runs.testReaderFactoryUsesLegacyTextDirectoryInV3LayoutWithoutProbing— same for thev3/subdirectory layout.Both tests fail without the fix (verified by stashing the main change) and pass with it.