Enable setting custom vertex ids in TP3 interface #147

[amcp]

fixes #147, another bug fix that didn't make it into Titan by the end of 2015
Signed-off-by: Alexander Patrikalakis [email protected]

[sjudeng]

I wonder if this might solve the issue I saw with custom vertex ids in upgrading to TinkerPop 3.2.3 (see notes in #78). To resolve there I updated test GraphComputerImplementations to include configuration around id generation but I wasn't happy with that approach.

[amcp]

@sjudeng I've got to put in for the night it being 2AM here, but if this interests you and given the extremely small size of the fix I encourage you to pick up from here and rip out the config workaround you put in GraphComputerImplementations to see if it fixes the issue you encountered.

[sjudeng]

I took a look at this branch. Initially I was having test failures of the form The provided key/value array must have a String or T on even array indices, which I resolved with the below update.

--- a/janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/JanusGraphBlueprintsTransaction.java
+++ b/janusgraph-core/src/main/java/org/janusgraph/graphdb/tinkerpop/JanusGraphBlueprintsTransaction.java
@@ -115,7 +115,8 @@ public abstract class JanusGraphBlueprintsTransaction implements JanusGraphTrans
             label = (labelValue instanceof VertexLabel)?(VertexLabel)labelValue:getOrCreateVertexLabel((String) labelValue);
         }
 
-        final JanusGraphVertex vertex = addVertex(ElementHelper.getIdValue(keyValues).orElse(null), label);
+        final Number id = (Number) ElementHelper.getIdValue(keyValues).orElse(null);
+        JanusGraphVertex vertex = addVertex(id != null ? id.longValue() : null, label);

I then tried running through tests in the PeerPressureTest suite via InMemoryJanusGraphComputerTest, which use custom vertex ids. But these gave errors at StandardJanusGraphTx.java#L514. I experimented with an additional conditional there for graph.getConfiguration().allowVertexIdSetting() && UserVertex.is(vertexId) but that also failed (UserVertex.is(vertexId) was false). I also tried forcing it past that error but then ended up with an error lower down at IDManager.java#L474.

[sjudeng]

See note above regarding build errors. I'll try to look at this as well when I get some time.

[sjudeng]

@amcp I rebased and added a commit to this PR. The commit includes a fix to JanusGraphBlueprintsTransaction to resolve test failures as described in the comment above. Also during testing I found that the existing documented method for converting user ids to vertex ids, JanusGraphId#toVertexId(long), is no longer valid as it doesn't take into account partitioning bits used in JanusGraph vertex assignment. Attempts to flush vertices with custom ids created using this method can lead to assert/IllegalArgumentException errors. To resolve this issue I added methods to IDManager for converting user vertex ids to/from JanusGraph vertex ids and deprecated JanusGraphId.

All default and TinkerPop tests are passing with these updates. Given my involvement in the implementation I think it would be best to have another independent review.

Dismissing my review since I contributed to the implementation.

[amcp]

minor comments.

[amcp]

can you test this method?

[sjudeng]

@amcp There's a test added as part of the commit (see VertexIDAssignerTest#testCustomIdAssignment).

[amcp]

can you test this method?

[amcp]

@sjudeng I agree we need another review.
@analytically the content of this PR changed slightly so can you re-review please?

[analytically]

Let's merge.