- Notifications
You must be signed in to change notification settings - Fork875
[COR-56] Unify VertexRef#22165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:devel
Are you sure you want to change the base?
[COR-56] Unify VertexRef#22165
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit theCursor dashboard to activate Pro and start your 14-day free trial.
Uh oh!
There was an error while loading.Please reload this page.
| // need a mechanism here as well to distinguish between (non)required | ||
| // parameters. | ||
| callback(Step{to, edge, previous, fetchedType, step.getDepth() +1, | ||
| callback(Step{VertexRef{to}, VertexRef{edge}, previous, fetchedType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Bug: Edge ID incorrectly wrapped in VertexRef type
Theedge variable (an edge ID of typeEdgeType/HashedStringRef) is incorrectly wrapped inVertexRef{edge}. TheClusterProviderStep constructor expectsEdgeType for the second parameter, notVertexRef. While this currently works due to the implicit conversion operator inVertexRef, it semantically treats an edge ID as a vertex reference. Onlyto (the vertex ID) should be wrapped inVertexRef;edge should be passed directly.
jvolmer left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Two small include improvements and the cursor-comment above looks legitimate. Otherwise, LGTM
| using VertexSet = arangodb::containers::HashSet<VertexRef, std::hash<VertexRef>, | ||
| std::equal_to<VertexRef>>; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should probably be exchanged with#include "Graph/Types/VertexSet.h"
| using VertexSet = arangodb::containers::HashSet<VertexRef, std::hash<VertexRef>, | ||
| std::equal_to<VertexRef>>; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same here
94c2ae2 to4e40c07Compare
Uh oh!
There was an error while loading.Please reload this page.
Scope & Purpose
Use the same definition of
VertexRefthroughout;Before this patch, there were various places in the codebase which used
using VertexRef = HashedStringRef, as well asSteps in the traverser code wrapping thisHashedStringRefinto a helper class, which is identical for every Step type.This PR introduces a class
VertexRefwhich replaces all these uses consistently (and is currently only a wrapper aroundHashedStringRefRequires Enterprise PR:https://github.com/arangodb/enterprise/pull/1563
Note
Introduces a unified
VertexRefwrapper (overHashedStringRef) and replaces ad-hoc vertex ID types/usages across graph components, updating APIs, data structures, and tests.graph::VertexRef(Graph/Types/VertexRef.{h,cpp}) andVertexSet(Graph/Types/VertexSet.h); migrateForbiddenVertices.hto use them.HashedStringRefvertex IDs withVertexRefin public methods (e.g.,reset,startVertex,addVertexToBuilder) across executors (TraversalExecutor,ShortestPathExecutor,EnumeratePathsExecutor).VertexRef; switch internal maps/sets to key byVertexRef.SingleServerProvider,ClusterProvider,ProviderTracer, and step types to store/returnVertexRefand propagate through expansion/builders.PathResultandSingleProviderPathResultto store vertices asVertexRef; update memory accounting and VPack emission;PathValidator*now usesVertexReffor uniqueness/forbidden checks.VertexRef.Written byCursor Bugbot for commit2cdb2fe. This will update automatically on new commits. Configurehere.