fix: read legacy compacted LSM keys across shared leaves - #5210
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Code Review
This pull request introduces changes to LSMTreeIndexCompacted to support backward compatibility with legacy compacted files written before the shared-leaf writer safeguard. It ensures that the first chunk of an overflowing key, which may reside on a preceding leaf, is correctly retrieved. A new compatibility test has been added to verify this behavior. The review feedback suggests wrapping the ResultSet queries in the test with try-with-resources to prevent potential resource leaks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| final ResultSet scan = database.query("sql", "SELECT count(*) AS c FROM Tok WHERE word.trim() = 'dup'"); | ||
| assertThat(((Number) scan.next().getProperty("c")).longValue()).as("full scan count").isEqualTo(EXPECTED_DUPLICATES); | ||
|
|
||
| final ResultSet indexed = database.query("sql", "SELECT count(*) AS c FROM Tok WHERE word = 'dup' AND lang = 'xx'"); | ||
| assertThat(((Number) indexed.next().getProperty("c")).longValue()).as("legacy compacted-index lookup count") | ||
| .isEqualTo(EXPECTED_DUPLICATES); |
There was a problem hiding this comment.
The ResultSet objects returned by database.query should be closed to prevent potential resource leaks during test execution. Wrapping them in a try-with-resources block ensures they are properly closed.
| final ResultSet scan = database.query("sql", "SELECT count(*) AS c FROM Tok WHERE word.trim() = 'dup'"); | |
| assertThat(((Number) scan.next().getProperty("c")).longValue()).as("full scan count").isEqualTo(EXPECTED_DUPLICATES); | |
| final ResultSet indexed = database.query("sql", "SELECT count(*) AS c FROM Tok WHERE word = 'dup' AND lang = 'xx'"); | |
| assertThat(((Number) indexed.next().getProperty("c")).longValue()).as("legacy compacted-index lookup count") | |
| .isEqualTo(EXPECTED_DUPLICATES); | |
| try (final ResultSet scan = database.query("sql", "SELECT count(*) AS c FROM Tok WHERE word.trim() = 'dup'")) { | |
| assertThat(((Number) scan.next().getProperty("c")).longValue()).as("full scan count").isEqualTo(EXPECTED_DUPLICATES); | |
| } | |
| try (final ResultSet indexed = database.query("sql", "SELECT count(*) AS c FROM Tok WHERE word = 'dup' AND lang = 'xx'")) { | |
| assertThat(((Number) indexed.next().getProperty("c")).longValue()).as("legacy compacted-index lookup count") | |
| .isEqualTo(EXPECTED_DUPLICATES); | |
| } |
There was a problem hiding this comment.
Addressed in dfed7b7. Both query results now use try-with-resources, and LegacySharedLeafIndexCompatibilityTest passes.
|
@justinblethrow-cloud thanks for the PR, checking it... |
|
Thanks for the PR! |
A unique key holds a single value that never overflows a page, so the pre-safeguard shared-leaf layout cannot occur on a unique index. Gate both the exact-lookup preceding-leaf read and the ascending-range shared-predecessor start on !unique, removing an extra page read from every unique compacted-index point lookup and full-key range that matches beyond the first root slot. Follow-up to #5210 (merged); non-unique behavior and the regression fixture are unchanged.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5210 +/- ##
=============================================
- Coverage 66.75% 33.90% -32.86%
- Complexity 0 1190 +1190
=============================================
Files 1696 1696
Lines 137700 137716 +16
Branches 29531 29536 +5
=============================================
- Hits 91926 46696 -45230
- Misses 33306 82049 +48743
+ Partials 12468 8971 -3497 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
* fix: read legacy compacted LSM keys across shared leaves * fix: start legacy compacted ranges at shared leaves * test: close legacy fixture query results --------- Co-authored-by: justinblethrow-cloud <226385385+justinblethrow-cloud@users.noreply.github.com> (cherry picked from commit a2a7f11)
A unique key holds a single value that never overflows a page, so the pre-safeguard shared-leaf layout cannot occur on a unique index. Gate both the exact-lookup preceding-leaf read and the ascending-range shared-predecessor start on !unique, removing an extra page read from every unique compacted-index point lookup and full-key range that matches beyond the first root slot. Follow-up to #5210 (merged); non-unique behavior and the regression fixture are unchanged. (cherry picked from commit d758998)
What does this PR do?
Makes exact lookups and full-key ordered ranges compatible with compacted LSM
files written before the shared-leaf overflow safeguard landed.
For a matching root key, the reader now also inspects the leaf immediately
preceding the first matching root entry. Older compacted files can contain the
first chunk of a high-cardinality key on that leaf, while the root entry for the
leaf is keyed by the preceding key. The existing result set removes overlap.
Ascending full-key ranges now start at the first matching root entry and the
possible shared predecessor, rather than at the last matching root entry.
The regression test opens a synthetic legacy database fixture containing
30,000 records with one composite key and verifies exact lookup plus ascending
and descending full-key ranges before and after reopen.
Motivation
Current
mainprevents newly compacted files from creating this layout, butthat writer change cannot repair files already persisted by an older release.
On the included fixture, current
mainreturns 28,857 records by exact lookupand 4,154 through the equivalent ascending full-key range, while a full scan
returns 30,000. With this reader change, exact lookup and both range directions
return 30,000 without rebuilding the index.
Related issues
after the compaction-atomicity fix in [lsm] Compaction failure atomicity: orphaned series become visible on the next round; first-compaction temp file leaks #4946; this PR addresses a distinct
backward-compatibility case for already-written shared-leaf layouts.
part of feat: [#4687] native BM25 full-text scoring (field boosts, caret, EXPLAIN/PROFILE) #4695.
Additional Notes
exact root-key matches that have a preceding leaf in the same compacted
series.
e03ffbbfand an unfiltered localengine run both failed the unrelated
Issue5147SuperNodeChunkRaceTestwiththe same
BufferUnderflowException. The focused LSM matrix below is green../mvnw clean package -DskipTestscompleted successfully across all 24 Mavenmodules. Tests were deliberately skipped in that packaging check because the
base revision has the unrelated failure above.
Checklist
mvn clean packagecommandFocused verification currently passes 35 tests across the legacy fixture,
duplicate composite-key regressions, compaction correctness and atomicity, and
general LSM type-index behavior.