Skip to content

Conversation

marianotepper
Copy link
Contributor

@marianotepper marianotepper commented Sep 30, 2025

The main intent of this PR is to add an interface in front of the OnHeapGraphIndex. This is because it has become clear recently that we will need new implementations for mutable graphs (that are more conservative in the use of heap memory). This PR paves the way towards that path.

Importantly, MutableGraphIndex is protected inside its package, because modifications should only be done through the GraphIndexBuilder class and never directly.

These changes triggered a lot of minor changes everywhere.

Copy link
Contributor

github-actions bot commented Sep 30, 2025

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 trigger and review regression testing results against the base branch via Run Bench Main?
  • 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?

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

@marianotepper
Copy link
Contributor Author

@sam-herman @michaeljmarshall Please make sure that the new design does not preclude any usages of JVector

Copy link
Contributor

@sam-herman sam-herman left a comment

Choose a reason for hiding this comment

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

I think this should work well with #519

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

Overall, looks good. I pulled the PR branch to my machine, built it, imported it into CC main (datastax/cassandra@748018e), fixed compilation errors, and ran VectorUpdateDeleteTest and didn't have any test failures.

I left some minor comments mostly on naming/javadocs and one comment about the getView() method that will impact CC.

Copy link
Collaborator

@tlwillke tlwillke left a comment

Choose a reason for hiding this comment

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

LGTM

…is being mutated by GraphIndexBuilder and a FrozenView once mutations are done.
Copy link
Collaborator

@tlwillke tlwillke left a comment

Choose a reason for hiding this comment

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

LGTM

@tlwillke tlwillke self-requested a review October 2, 2025 17:25
Copy link
Collaborator

@tlwillke tlwillke left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

@MarkWolters MarkWolters merged commit ddabdb8 into main Oct 2, 2025
12 checks passed
@MarkWolters MarkWolters deleted the mutable-interface branch October 2, 2025 19:50
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.

5 participants