Adopt jspecify + NullAway null-checking on fdb-record-layer-lucene - #4583
Draft
arnaud-lacurie wants to merge 11 commits into
Draft
arnaud-lacurie wants to merge 11 commits into
arnaud-lacurie wants to merge 11 commits into
Conversation
arnaud-lacurie
changed the title
arnaud-lacurie
commented
Sep 7, 2026
| // you won't be able to repartition, so we always provide all the partition info. | ||
| if (request.getPartitionId() != null) { | ||
| infoStream = infoStream.filter(info -> info.getId() == request.getPartitionId()); | ||
| final Integer partitionId = request.getPartitionId(); |
Collaborator
Author
There was a problem hiding this comment.
request.getPartitionId() was previously called twice here — once for the null check, once inside the filter lambda to compare against info.getId(). SpotBugs correctly flags this: it can't (and shouldn't) assume two calls to a nullable-returning getter return the same value. Fixed by capturing it once in the partitionId local and reusing that in the filter.
| } | ||
|
|
||
| final PartitionedSortContext sortCriteria = luceneScanQuery.getSort() == null ? null : isSortedByPartitionField(luceneScanQuery.getSort()); | ||
| final Sort sort = luceneScanQuery.getSort(); |
Collaborator
Author
There was a problem hiding this comment.
Same double-getter pattern as the other findings in this cleanup: luceneScanQuery.getSort() was called once for the null check and again to compute sortCriteria. Fixed by capturing it once in the sort local instead of calling the nullable getter twice.
| final var newFieldNames = luceneQueryComponent.getFields().stream().map(field -> name + "_" + field).collect(Collectors.toList()); | ||
| final var newExplicitFieldNames = luceneQueryComponent.getExplicitFieldNames() == null | ||
| ? null : luceneQueryComponent.getExplicitFieldNames().stream().map(field -> name + "_" + field).collect(Collectors.toSet()); | ||
| final var explicitFieldNames = luceneQueryComponent.getExplicitFieldNames(); |
Collaborator
Author
There was a problem hiding this comment.
Same double-getter pattern: luceneQueryComponent.getExplicitFieldNames() was called once for the null check and again to build newExplicitFieldNames. Fixed by capturing it once in the explicitFieldNames local.
| switch (filter.getType()) { | ||
| case AUTO_COMPLETE: | ||
| final var resolvedFields = filter.getExplicitFieldNames() == null | ||
| final var explicitFieldNames = filter.getExplicitFieldNames(); |
Collaborator
Author
There was a problem hiding this comment.
Same double-getter pattern again, this time on filter.getExplicitFieldNames() in the AUTO_COMPLETE branch — null-checked once, then re-fetched for resolvedFields. Fixed the same way, via the explicitFieldNames local.
| if (preCommitCallback != null) { | ||
| store.getContext().getOrCreateCommitCheck(DRAIN_PRE_COMMIT_HOOK + state.index.getName(), | ||
| name -> () -> preCommitCallback.apply(store)); | ||
| return new CursorFactory<PendingWriteQueue.QueueEntry>() { |
Collaborator
Author
There was a problem hiding this comment.
Interesting root cause here, worth spelling out: the previous lambda captured pendingWriteQueue alongside its own @Nullable-annotated lastResult parameter. javac's lambda desugaring prepends captured variables (pendingWriteQueue, preCommitCallback) to the synthetic method's parameter list, but does not re-index the lambda's own parameter type annotations to account for the shift. The net effect: lastResult's @Nullable annotation ended up misattributed to the captured pendingWriteQueue slot in the compiled bytecode, which made SpotBugs (incorrectly) report NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE on pendingWriteQueue. A method-level suppression doesn't help either, since SpotBugs matches against the synthetic lambda method, not the enclosing declaration. The fix rewrites this as an explicit anonymous class implementing CursorFactory, since anonymous classes capture enclosing locals as real fields rather than prepended synthetic parameters — sidestepping the desugaring artifact entirely.
| throw new RecordCoreException(cause); | ||
| // Otherwise, wrap with generic RecordCoreException. cause may legitimately be null if the | ||
| // ExecutionException itself was constructed without one. | ||
| throw new RecordCoreException("Unexpected exception while lazily opening resource", cause); |
Collaborator
Author
There was a problem hiding this comment.
new RecordCoreException(cause) used the cause-only constructor, but cause (from ExecutionException#getCause()) can legitimately be null if the ExecutionException itself was constructed without one — that constructor would then NPE while building the exception message. Fixed by switching to the RecordCoreException(String, Throwable) overload, which tolerates a null cause.
…gress) Adds jspecify/NullAway wiring to fdb-record-layer-lucene.gradle and fixes NullAway findings across the module (real nullability bugs, missing @nullable annotations, and array-type tooling-limitation workarounds). compileJava still has ~32 remaining errors; compileTestJava, pmd, and spotbugs not yet run. Committing as a checkpoint before continuing.
More real nullability fixes (LuceneCursorContinuation.toBytes() override gotcha, LuceneIndexSpellCheckQueryPlan.fetchIndexRecords() same, field comparison suffix handling, lock factory context/value handling, stored fields writer eager init, primary key segment index invariant) plus remaining getPropertyValue/varargs-style widenings. Checkpoint before continuing the loop.
Fixes remaining compileJava findings: getFileName() incorrectly marked @nullable (never actually null), analyzer registry double Map.get() correlation, FDBIndexOutput close()-then-write fail-fast contract, FDBTieredMergePolicy/BitSetQuery Lucene-API null returns.
FDBDirectoryLockFactory.java was missing the jspecify Nullable import. BitSetQuery.java's DocIdSetBuilder.BulkAdder field hit the same qualified-nested-type @nullable placement issue seen elsewhere; fixed by importing BulkAdder and using the simple name.
Completes the NullAway compile-fix pass for the lucene module: compileTestJava now compiles clean (306 errors resolved across ~41 test files), on top of the already-clean compileJava. Also removes the temporary -Xmaxerrs diagnostic override added to see the full error list. Fix patterns applied, consistent with the production-code pass: - Objects.requireNonNull wraps at the point where a value is first obtained, for cases where an unannotated dependency (fdb-record-layer-core, Pair, Lucene) returns @nullable generically but the test's own setup guarantees presence (index/timer/path lookups, map entries, cursor results, etc.) - @nullable widening on a handful of fields/params that are genuinely lazily-initialized or optional (InjectedFailureRepository fields, FDBDirectoryBaseTest.createDirectory's indexOptions, LuceneIndexTestUtils/ LuceneIndexTestValidator/FDBLuceneTestBase Sort/executorService params) to match already-@nullable production contracts or call sites that pass null - A couple of genuine non-mechanical fixes: LuceneIndexMaintenanceTest and PendingWriteQueueIntegrationTest replaced null-literal FDBDirectoryLockFactory constructor args with real FDBDirectory instances (the field is unused by the lock path being tested, but the constructor param is non-null in production); LuceneHighlighterTest dropped a dead "return null" after Assertions.fail(...) by relying on fail's generic return type instead.
…igration in fdb-record-layer-lucene
Fixes the last 6 spotbugsMain findings blocking the jspecify + NullAway migration for this module. Five were genuine (if benign) double-getter races that SpotBugs correctly flags because it cannot prove a second call to the same nullable-returning accessor yields the same result as the one already null-checked: LuceneIndexMaintainer#getLuceneInfoForAllPartitions, LucenePartitioner#selectQueryPartitionAsync, LucenePlanner#prefixFieldNames, LucenePlanner#getQueryForLuceneComponent, and FDBDirectoryWrapper#createFDBDirectory. Each is fixed by capturing the nullable value in a local once and reusing it. The sixth, FDBDirectoryWrapper#cursorFactory, was a real false positive but not the cross-module jspecify/javax annotation mismatch this rollout has seen elsewhere: javac's lambda desugaring prepends captured parameters (pendingWriteQueue, preCommitCallback) to the synthetic lambda method's parameter list without re-indexing the lambda's own @nullable lastResult parameter annotation to match, so the annotation ends up misattributed to the captured pendingWriteQueue slot in the compiled bytecode. A method-level @SpotBugsSuppressWarnings on the enclosing method doesn't help here since SpotBugs matches suppressions against the exact synthetic method the bug is reported against, not the lambda's enclosing declaration (confirmed by SpotBugs reporting the suppression itself as unnecessary). Rewriting the lambda as an explicit anonymous class implementing CursorFactory avoids the desugaring artifact entirely, since anonymous classes capture enclosing locals as fields rather than prepended synthetic parameters.
…contracts Now that fdb-record-layer-core's jspecify/NullAway annotations are fully corrected, NullAway sees core's real nullability contracts for the first time from lucene's perspective (a separate NullAway-annotated package), surfacing errors in main and test sources that were previously hidden. Real fixes: - LazyOpener#getUnchecked: use RecordCoreException's message+cause constructor instead of the cause-only one, since the cause can legitimately be null. - LucenePrimaryKeySegmentIndexV1/V2#findDocument: rework the map lambda to return Optional<DocumentIndexEntry> instead of a bare nullable value, so the null-then-filter idiom no longer fights the type system. - LuceneIndexScrubbingToolsMissing#handleOneItem: widen the override's Issue type argument to @nullable, matching the corrected supertype contract (ValueIndexScrubbingToolsMissing); route the missing recordToIndex through a dedicated helper, mirroring the analogous fix in core. - LucenePlanner#planOther: widen filter to @nullable to match RecordQueryPlanner's real contract, and only take the Lucene-specific path when filter is non-null (planLucene isn't designed for a null filter). - LuceneSerializerTest#getSerializer: assert the invariant that random is always provided when encrypting, with Objects.requireNonNull. - FDBLuceneQueryTest/FDBLuceneMapQueryTest: correct a mistyped byte[] continuation local and use Objects.requireNonNull for a query result that is always populated in context. The remaining errors are all instances of two documented NullAway/JSpecify tooling gaps also seen elsewhere in this rollout: array (byte[]) parameter nullability isn't reliably recognized across the module boundary (e.g. scanIndex/scanRecords/executeQuery continuations), and Tuple.from's varargs parameter is from an unannotated external library that legitimately accepts null elements. These are suppressed with @SuppressWarnings("NullAway") plus an explanatory comment, following the same pattern already established in fdb-record-layer-core.
… fdb-record-layer-lucene Surfaced after fdb-record-layer-lucene's NullAway contracts were corrected to match fdb-record-layer-core's finalized signatures: - LuceneAnalyzerRegistryImpl#getLuceneAnalyzerCombinationProvider: suppress with explanation, NonnullPair.getLeft()/getRight() are guaranteed non-null by construction but SpotBugs falls back to the wider @nullable Pair contract. - LuceneFunctionKeyExpression.LuceneSortBy#evaluateFunction: real fix, capture FDBQueriedRecord#getIndexEntry() once instead of calling it twice, since SpotBugs cannot prove two separate calls would agree. - LuceneIndexKeyValueToPartialRecordUtils#buildIfFieldNameMatch: suppress with explanation, same NonnullPair false positive as above.
arnaud-lacurie
force-pushed
the
apple/arnaud-lacurie/jspecify-nullaway/lucene
branch
from
September 8, 2026 00:07
25f619c to
59b9d3b
Compare
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
1 participant
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.
10th of a 14-PR stack adopting jspecify + NullAway null-checking, stacked on #4582 (
fdb-record-layer-spatial). Same treatment applied tofdb-record-layer-lucene, split into two independently-worked chunks then merged. See inline comments for specific findings.