Skip to content

Improved File Versioning - #707

Open
MarkWolters wants to merge 5 commits into
mainfrom
file_versioning
Open

Improved File Versioning#707
MarkWolters wants to merge 5 commits into
mainfrom
file_versioning

Conversation

@MarkWolters

Copy link
Copy Markdown
Contributor

Summary

This branch eliminates scattered version-comparison branching (if version >= X, if version < Y, version == Z) from the on-disk graph index read/write path and replaces it with a Strategy + Factory pattern: each
supported format version (2–6) gets its own GraphIndexFormat implementation, and GraphIndexFormatFactory centralizes version→format dispatch and detection.

What changed

New abstraction (graph/disk package):

  • GraphIndexFormat — strategy interface covering header/footer read+write, feature ordering, and full sequential/random-access write orchestration for a single format version.
  • AbstractGraphIndexFormat — shared default implementation; versions override only what's actually different for them.
  • GraphIndexFormatV2..GraphIndexFormatV6 — one small class per version, built on a natural inheritance chain (V4 extends V3, V5 extends V4 + footer, V6 extends V5 + feature reordering + fused-PQ
    support).
  • GraphIndexFormatFactoryforVersion(int), detectVersion(reader) (magic-number sniffing), getCurrentVersion().
  • WriteContext — bundles the ~7 parameters that used to be threaded individually through every serializer method.

Migrated call sites:

  • AbstractGraphIndexWriter (and its OnDiskSequentialGraphIndexWriter / RandomAccessOnDiskGraphIndexWriter subclasses) now delegate all format-specific work to the resolved GraphIndexFormat instead of
    branching on version inline. Feature-support validation (Builder.build(), fused-PQ gating) now asks the format (supportsFeature) instead of hardcoding version thresholds.
  • Header / CommonHeader now delegate header size/read/write and common-header size/read/write to the per-version format, removing the version >= 3 / >= 4 / >= 6 chains that used to live in both classes.
  • OnDiskGraphIndex load/footer dispatch goes through GraphIndexFormat.loadOnDiskIndex, removing the inline version >= 5 && useFooter branch; the two remaining fused-PQ-layout checks now ask
  • CompactWriter (streaming N:1 compaction writer) validates once, at construction, that fused PQ is actually supported by the target format; the three per-call version == 6 checks in its hot paths were simplified
    to the already-existing fusedPQEnabled flag, since this writer only ever targets the current version. No added per-record overhead.

Fixed along the way:

  • A regression where the writer/reader split silently dropped eager in-memory cache priming (getInMemoryLayers/getInMemoryFeatures) for non-footer format versions — caught by a ramBytesUsed() test failure,
    fixed by centralizing construction + priming in OnDiskGraphIndex.construct().
  • allFeatures() / nonFusedFeatures() helpers now return explicit, frozen EnumSet.of(...) listings instead of deriving from EnumSet.allOf(...) / complementOf(...), so a future FeatureId addition can't
    silently change what an already-shipped version claims to support.

Testing

  • New TestGraphIndexFormatFactory: version→format dispatch, unsupported-version rejection, per-version characteristics (supportsMultiLayer, usesFooter, getSupportedFeatures), and magic-number version
    detection.
  • New TestOnDiskGraphIndex.testVersionRoundTrip: explicit write/read round trips for versions 3, 4, and 5 (single-layer and, for 4/5, multi-layer), in addition to the existing version-2 and current-version (6)
    coverage.
  • Full jvector-tests suite passes (208 tests, 0 failures).

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Before you submit for review:

  • Does your PR follow guidelines from CONTRIBUTIONS.md?
  • Did you summarize what this PR does clearly and concisely?
  • Did you include performance data for changes which may be performance impacting?
  • Did you include useful docs for any user-facing changes or features?
  • Did you include useful javadocs for developer oriented changes, explaining new concepts or key changes?
  • Did you rebase your branch onto the latest main for regression testing and PR submission?
  • Did you trigger regression testing via Run Bench Main and review results?
  • Did you adhere to the code formatting guidelines (TBD)
  • Did you group your changes for easy review, providing meaningful descriptions for each commit?
  • Did you ensure that all files contain the correct copyright header?
  • Did you add documentation for this feature to the release notes directory?

If you did not complete any of these, then please explain below.

private final boolean supportsMultiLayer;
private final boolean usesFooter;

/** A magic number to indicate the file footer */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get a slightly better readout on what the footer looks like here. These constants don't really explain it for the uninitiated.

@jshook jshook left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll re-review when the convos are resolved


@Test
public void testForVersionReturnsMatchingFormat() {
for (int version = 2; version <= OnDiskGraphIndex.CURRENT_VERSION; version++) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to implement the version property as a decorator interface, so that callers which need to handle V1 look more like:
if (index instanceof VersionTaggedIndex vti) {
// normative code
} else {
// v1
}

As this allows us to avoid defining a method for a version which is effectively a false promise or actually undefined functionally.

* on, so that a change to one version's format can't silently change another's behavior.
*/
@Test
public void testFormatCharacteristicsPerVersion() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like above, decorator interfaces for the features only implemented by a subset of indexes make for cleaner type system here. Each thing which you have to have implicit knowledge of based on a number to feature matrix can be properly represented as a decorator interface, as the call site logic for instanceof can be efficient when required.

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.

2 participants