Skip to content

GraphTraversalProvider.getEdgeProperty still answers from the base CSR while an overlay is active, so the next caller inherits the bug #6301 fixed #6315

Description

@lvca

Found while fixing #6301 (PR #6306). The instances are closed; the shape is not.

What #6306 did, and what it did not do

getEdgeProperty(nodeId, neighborIndex, direction, edgeType, property) addresses an edge by its position in the node's neighbour list. That contract holds only while the position identifies the same edge getNeighborIds reports there - and an active DeltaOverlay breaks it: the property columns are aligned with the base CSR's forward edge slots, while getNeighborIds serves the overlay's view of the node (deletions dropped, additions merged in, the whole list re-sorted by Arrays.sort). The n-th neighbour of that list is then not the n-th edge of the column store.

PR #6306 closed this for every caller that exists today, by narrowing hasEdgeProperties():

// GraphAnalyticalView
public boolean hasEdgeProperties() {
  return snap != null && snap.edgeColumnStores != null && !snap.edgeColumnStores.isEmpty() && snap.overlay == null;
}

getEdgeProperty itself is unchanged. Called directly with an overlay active it still returns a value addressed against the base CSR - a weight belonging to some other edge - with nothing in its signature or behaviour to say so. Verified on 0237e7b: the method body contains no reference to snap.overlay.

So the guarantee currently rests on every caller remembering to ask hasEdgeProperties() (or servesEdgeProperty) first. That is exactly the shape #6301 was: a correct-if-you-know-the-rule API, where breaking the rule produces a plausible wrong number rather than an error. The next caller written against the SPI has no way to discover the rule from the method it is calling.

Why it matters more than the caller count suggests

The failure is silent. A wrong weight produces a wrong shortest path, a wrong MST, a wrong Steiner tree - never an exception, never a log line. #6301 sat undetected long enough to reach four independent call sites, and the two in SQLFunction* were only found because a reviewer went looking on the third pass.

It is also reachable in ordinary operation, not a corner: an overlay is what UpdateMode.SYNCHRONOUS produces on every commit against a built view.

Options

  1. Return null when the overlay is active. Honest, and matches getMeanEdgesPerConnectedPair, which already answers MULTIPLICITY_UNKNOWN for exactly this overlay. The catch: null is also how "this edge has no value for that property" is reported, so a caller still cannot distinguish "unavailable" from "absent" - it would silently default the weight instead of silently reading the wrong one. Better, but not closed.
  2. Make the overlay resolvable. getNeighborIds knows which entries came from the base CSR and at what offset; carrying that through would let getEdgeProperty map a post-overlay position back to its forward slot, and answer exactly for base edges and null only for overlay-added ones. This closes the shape rather than the instances, and is the only option that lets a weighted algorithm keep the columnar path while a view is being updated - which is the case the CSR exists for.
  3. Remove the positional form from the SPI and leave edgeWeightsOf (added in fix(#6295, #6300, #6301, #6302): a weight belongs to its own edge, and the phase a call spends its time in is the one that has to be abortable #6306) as the only way in. It already handles the multi-type merge and the BOTH split, and it is the only correct way to use getEdgeProperty today. Making the sharp tool private to the SPI's own default method is what stops a fifth hand-rolled copy from appearing.

Option 3 is the smallest change that closes the shape, and 2 is the one worth doing if the columnar path should survive an overlay at all. They compose: 3 now, 2 when someone needs the performance.

Related

#6301, PR #6306, #6296, #6289.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions