Read legacy text index directory without probing consolidated entry when it exists#18919
Read legacy text index directory without probing consolidated entry when it exists#18919xiangfu0 wants to merge 1 commit into
Conversation
cb8749f to
12939e2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18919 +/- ##
============================================
+ Coverage 65.21% 65.24% +0.02%
Complexity 1405 1405
============================================
Files 3418 3418
Lines 214651 214652 +1
Branches 33922 33923 +1
============================================
+ Hits 139989 140041 +52
+ Misses 63356 63335 -21
+ Partials 11306 11276 -30
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:
|
e7cf769 to
042febc
Compare
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found one high-signal SegmentDirectory correctness issue; see inline comment.
| // directory and fails to map it (not a regular file), which would kill the segment load | ||
| // before any fallback could run. This keeps storeInSegmentFile=true rolling-upgrade-safe | ||
| // for existing text segments. | ||
| File legacyTextIndex = SegmentDirectoryPaths.findTextIndexIndexFile(segmentDir, metadata.getColumnName()); |
There was a problem hiding this comment.
This probe needs the same local-directory guard used by the vector reader. SegmentDirectoryPaths.findTextIndexIndexFile() calls findFormatFile(), which requires segmentDir.isDirectory(). For a remote/tiered SegmentDirectory, getPath() can be a non-existent local path while getIndexFor() can still return the consolidated columns.psf buffer, so storeInSegmentFile=true can fail segment load before reading that buffer. Please guard the probe with segmentDir.isDirectory() and add a non-local consolidated-entry regression test like the vector reader has.
042febc to
b2c8049
Compare
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found one high-signal issue; see inline comment.
| // directory and fails to map it (not a regular file), which would kill the segment load | ||
| // before any fallback could run. This keeps storeInSegmentFile=true rolling-upgrade-safe | ||
| // for existing text segments. | ||
| File legacyTextIndex = SegmentDirectoryPaths.findTextIndexIndexFile(segmentDir, metadata.getColumnName()); |
There was a problem hiding this comment.
This calls findTextIndexIndexFile() before trying the consolidated text-index entry, but that helper requires segmentDir to be a local directory. For non-local SegmentDirectory implementations, segment load can fail here before getIndexFor(..., text()) has a chance to read the valid columns.psf entry. The vector reader guards the same sidecar probe with segmentDir.isDirectory(); this path should do the same and add a non-local consolidated-entry regression test.
4433c2f to
ccf0cb7
Compare
…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.
ccf0cb7 to
7b8c035
Compare
2 participants
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.