Iterator/stream: fix reverse-prefix bug + Spliterator SORTED contract, add coverage - #292
Open
bernardladenthin wants to merge 2 commits into
Open
Conversation
Adds unit coverage for lmdbjavagh-269 iterator/stream helpers that had none, and corrects the LmdbStream spliterator characteristics. Tests: - BufferProxyPrefixTest: containsPrefix + incrementLeastSignificantByte across all four proxies, with edge cases (empty/oversized prefix, unsigned bytes, trailing-0xFF carry, all-0xFF -> null). - CompareAsIntegerKeysTest: length-mismatch guard and non-4/8-byte lexicographic fallback for the integer-key comparators. - KeyRangeBuilderTest: builder/prefix/start-stop-inclusive accessors; also documents that getType() is null for builder()/prefix()-created ranges. - LmdbStreamCharacteristicsTest: locks in the corrected spliterator contract. Fix (LmdbStream): - Drop the false SORTED and DISTINCT characteristics: entries are produced in key order but no KeyVal comparator is exposed, and DUPSORT can repeat keys. - getComparator() now throws IllegalStateException (required for a non-SORTED spliterator) instead of returning null. - Remove the dead entryComparator plumbing (stubbed factories returning null), which also clears the unused-parameter warnings. The fuller alternative (real Comparator<KeyVal<T>> + SORTED) was investigated and rejected: the spliterator emits a single reused KeyVal instance, so advertising SORTED would be unsound without first emitting per-element snapshots. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Fix (correctness):
- incrementLeastSignificantByte over-shot the prefix successor when the prefix
ended in 0xFF: {0x01,0xFF} produced {0x02,0xFF} instead of the tight {0x02},
keeping the trailing 0xFF. In reverse prefix iteration this seeks past the
prefix range and steps back onto an unrelated higher key, so iteration wrongly
yields nothing. Now truncates trailing 0xFF across all four proxies (big-endian
byte-string branch). LmdbPrefixReversedSuccessorTest reproduces and guards it.
KeyRange:
- Builder/empty ranges now derive the equivalent KeyRangeType, so getType() is no
longer null (a null type would NPE the legacy CursorIterable path). Prefix
ranges still have no KeyRangeType (documented).
Dbi:
- Document that stream()/newIterate() emit a single reused KeyVal holder (so
collect/sorted/distinct observe aliased entries) and that the stream is ORDERED
but not SORTED.
- Simplify getNameAsString(Charset): drop the dead try/catch and misleading
"assume UTF8" comment; new String(byte[], Charset) never throws.
- Fix stale javadoc link CursorIterable.JavaRangeComparator -> JavaRangeComparator.
Tests:
- DbiIterateApiTest: newIterate(EntryConsumer) overloads, ranged stream,
LmdbIterable single-use, getNameAsString, toString.
- BufferProxyPrefixTest / KeyRangeBuilderTest updated for the corrected successor
truncation and the derived KeyRangeType.
Full suite (excluding the pre-existing flaky TestLmdbStreamBenchmark) green: 1687
tests, 0 failures.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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
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.
Follow-up work stacked on the
iterator-performancebranch (targets that branch, notmaster). It fixes two correctness/contract issues in the new iterator/stream code, adds the missing unit/integration coverage for the gh-269 helpers, and tidies a few related items. All changes are additive or self-contained; the design decisions on the branch (see "Deliberately left open" below) are untouched.Fixes
1. Reverse prefix iteration silently drops rows when the prefix ends in
0xFFBufferProxy.incrementLeastSignificantBytecomputed the "prefix successor" by incrementing the least-significant non-0xFFbyte but keeping the trailing0xFFbytes — e.g.{0x01,0xFF}→{0x02,0xFF}instead of the tight{0x02}.In
LmdbPrefixReversedIterator/LmdbPrefixReversedSpliteratorthe successor is used toMDB_SET_RANGEpast the prefix range and step back. When it over-shoots, an unrelated higher key that sorts between the last prefix match and the over-shot successor is landed on, thecontainsPrefixcheck fails, and iteration wrongly yields nothing.LmdbPrefixReversedSuccessorTest(keys{0x01,0xFF},{0x01,0xFF,0x05},{0x02,0x00}; reverse-prefix{0x01,0xFF}returned[]before the fix).0xFF(tight successor) across all four proxies' big-endian byte-string branch (ByteArray, ByteBuffer, DirectBuffer, Netty). The little-endian/integer-key branch is left as-is — prefix scans operate on big-endian byte strings.2.
LmdbStreamSpliterator advertisedSORTEDwith anullcomparatorcreateEntryComparator/createReversedEntryComparatorreturnednull(real body commented out), yet every spliterator reportedORDERED | DISTINCT | SORTED | NONNULLandgetComparator()returned thatnull. ASORTEDspliterator with anullcomparator implies natural ordering, butKeyValis notComparable.SORTEDandDISTINCT(DUPSORT can repeat keys) →ORDERED | NONNULL.getComparator()now throwsIllegalStateException, as required for a non-sorted spliterator.entryComparatorplumbing (also clears the "uselessrangeComparatorparameter" scan alerts).Comparator<KeyVal<T>>+SORTEDisn't safe without first emitting per-element snapshots, because the spliterator emits a single reusedKeyVal(collect(toList())returns one aliased instance for every row). That's a larger design change and is left to the maintainers.3.
KeyRange.getType()returnednullfor builder-created rangesThe private constructors used by
KeyRange.builder()never settype, sogetType()returnednulland would NPE the legacyCursorIterablepath. Builder/empty ranges now derive the equivalentKeyRangeType. Prefix ranges still have noKeyRangeType(documented) as they are only consumed by the new iterators.Tidy-ups
Dbi.stream()/newIterate()javadoc now warns that entries are a single reusedKeyValholder (socollect/sorted/distinctobserve aliased entries) and that the stream isORDEREDbut notSORTED.Dbi.getNameAsString(Charset): removed a deadtry/catchand misleading "assume UTF8" comment (new String(byte[], Charset)never throws).CursorIterable.JavaRangeComparator→JavaRangeComparator.New/updated tests
BufferProxyPrefixTest—containsPrefix+incrementLeastSignificantByteacross all four proxies, incl. edge cases (empty/oversized prefix, unsigned bytes, trailing-0xFFtruncation, all-0xFF→ null).CompareAsIntegerKeysTest— length-mismatch guard + non-4/8-byte lexicographic fallback for the integer-key comparators.KeyRangeBuilderTest— builder/prefix/inclusive accessors + derivedKeyRangeType.LmdbStreamCharacteristicsTest— locks in the corrected spliterator contract.DbiIterateApiTest—newIterate(EntryConsumer)overloads, ranged stream,LmdbIterablesingle-use,getNameAsString,toString.LmdbPrefixReversedSuccessorTest— the reverse-prefix regression above.Verification
Full test suite (excluding the pre-existing, environment-flaky
TestLmdbStreamBenchmark, which fails atEnv.openunrelated to these changes): 1687 tests, 0 failures.mvn fmt:checkis clean for all touched files. Java 8 source level unchanged.Deliberately left open (branch decisions, not addressed here)
These came up while reviewing the branch and are for the maintainers, not this PR:
newIterate(...)and retiring/adaptingCursorIterablefor API compatibility.MDB_UNSIGNEDKEYDbiFlag should probably be the default behaviour #249 / CursorIterable used with BACKWARD_AT_LEAST and BACKWARD_CLOSED is not returning expected ranges of results when using DUPSORT #267 work from the iterator perf change.master; squashing WIP history and purging the accidentally-committed native binaries (src/main/resources/org/lmdbjava/*.so|*.dll) that live in branch history.Env/Dbi/Cursormethods still thinly covered) and the remaining code-scanning alerts (deprecatedEnv.openDbiin tests,NumberFormatExceptionin CSV parsing, field shadowing) — some intentionally not touched here as they live in branch-owned test helpers.KeyVal-snapshot redesign that would letSORTED+ a real comparator return.🤖 Generated with Claude Code