Skip to content

Consolidate NodeRecordTask and test ordinal holes - #2

Draft
jaipilot[bot] wants to merge 1 commit into
jaipilot-v8/jvector-695from
jaipilot/pr-1-qXkVBdQ6xwaX
Draft

Consolidate NodeRecordTask and test ordinal holes#2
jaipilot[bot] wants to merge 1 commit into
jaipilot-v8/jvector-695from
jaipilot/pr-1-qXkVBdQ6xwaX

Conversation

@jaipilot

@jaipilot jaipilot Bot commented Aug 23, 2026

Copy link
Copy Markdown

What

  • NodeRecordTask.java: extracted three blocks duplicated verbatim between callLegacy() and buildFullRecord() into shared private helpers: writeOmittedFeaturesAndEmptyNeighbors, checkNodeExists, writeNeighborSection.
  • Grid.java: collapsed the useParallelConstruction if/else in builderWithSuppliers() (two branches differing only in which concrete Builder subtype is constructed) into a single ternary-selected variable, removing 3 duplicated statements.

Why

Both duplications were introduced by this PR's NodeRecordTask/ParallelGraphWriter rewrite, which had no dedicated unit test coverage anywhere in the repository. Extracting the duplicated logic reduces the surface that future edits to validation/hole/neighbor-writing semantics would otherwise have to keep in sync by hand across two call sites.

Behavior preservation

  • Added testOrdinalHolesOnePhase/testOrdinalHolesTwoPhase to the existing TestRandomAccessOnDiskGraphIndexWriter (which already covered the batched vs. legacy paths for non-hole ordinals) to lock the OMITTED-ordinal branch touched by the cleanup, using an OrdinalMapper.MapMapper with gaps.
  • Ran the same 6-test focused command against the unmodified PR-head production code (baseline) and the final candidate (candidate): identical pass/fail outcome (6/6 passed both times).
  • Ran the full 5-module reactor build+test suite against the candidate: BUILD SUCCESS, 204/204 tests passed (2 pre-existing skips).
  • Measured NodeRecordTask write timings (batched and legacy paths) before/after the extract-method refactor across 5 iterations each: medians differ by <3ms (noise level) -- no performance regression.

Limitations

  • No JMH/PIT/architecture tooling is configured in this repository; evidence is limited to JUnit test execution, full reactor build status, and a manual timing comparison.
  • Grid.java's builderWithSuppliers change was verified by full-module compile and the existing (unrelated) jvector-examples test suite; there is no dedicated unit test for this example/benchmark helper in the repository, before or after this change.

Generated by JAIPilot Cloud for #1 from Anthropic session sesn_01YAcXgx4HvDqXkVBdQ6xwaX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants