Support for numeric vertex ids?

[fabiencoppens]

Hi. I see in GremlinUtils.getIdField that only String ids are supported:

} else if (idField.getType() != String.class) {
            throw new GremlinInvalidEntityIdFieldException("the type of @Id/id field should be String");
        }

Are there any plans to support numeric id types? We're hoping to use spring-data-gremlin on top of JanusGraph, which only supports numeric ids.

[fabiencoppens]

ADDITIONAL INFO: As a sanity check, I commented out that if statement in GremlinUtils.getIdField that I referenced above, and re-ran the spring-data-gremlin-example UTs again (after having converted the entity ids to Longs), but still run into an issue because the id value ends up marshalled as a string in the gremlin query that is ultimately generated: com.microsoft.spring.data.gremlin.exception.GremlinQueryException: unable to complete execute [g.addV('Person').property(id, '89757').property('name', 'person-name').property('age', '4'), g.addV('Person').property(id, '123456789').property('name', 'person-No.0').property('age', '18'), g.addV('Person').property(id, '666666').property('name', 'person-No.1').property('age', '27'), g.V('123456789').addE('Relation').to(g.V('666666')).property(id, '2333').property('name', 'brother')] from gremlin; nested exception is java.util.concurrent.CompletionException: org.apache.tinkerpop.gremlin.driver.exception.ResponseException: Cannot cast java.lang.String to java.lang.Number

As you can see, the problem comes from e.g. .property(id, '89757') I think this is caused by GremlinSourceVertexWriter.write() line 32: source.setId(converter.getFieldValue(domain, source.getIdField().getName()).toString());

[Incarnation-p-lee]

@fabiencoppens Thanks for your effect and issue.

For first one, I think it simple to support Id with Long type. Can you prepare the PR for this enhancement ?

For id issue, you mean for JanusGraph, it cannot accept the syntax like property(id, '44444') ? In previous, we use 'id' for property, but there is another issue about id. See this PR https://github.com/Microsoft/spring-data-gremlin/pull/134 and Issue https://github.com/Microsoft/spring-data-gremlin/issues/132

Looks some graphDb can support this but some not, Hi @kschulst , Can you share more information about this and your backend graphdb. We may consider one trade-off solution for this.

[mad]

JanusGraph doesn't support clean user defined ID. It uses own layout for internal id, thus id non-required param

Can you add non-required id support?

[Incarnation-p-lee]

@fabiencoppens OK. I see, Just modify that should not work for Long id. Need farther code change there. @mad Could you please file another issue for this ?

[Incarnation-p-lee]

@fabiencoppens Our test env backend graphdb do not support the number id. Cloud you please share (by email) one configuration with the database support that ? Then I can add some test for this.

[fabiencoppens]

@Incarnation-p-lee Do you need a config for UTs, i.e. with an embedded JanusGraph and datastore, or for integration tests? The testing I did when I opened this ticket was using JanusGraph running as a Gremlin Server, with a Cassandra backing datastore. I'm guessing you need the latter since spring-data-gremlin uses the Java client (from the gremlin driver) with websockets to talk to a non-embedded tinkerpop-compliant runtime. I can email you the config to use for JanusGraph as a gremlin server, with Cassandra as the backend. Let me know if that works of if you need something else. Thanks. By the way, as I mentioned in my comment on Issue #142, in order to do my quick testing I had to use a non-recommended setting on JanusGraph to make it accept user provided ids. As they mention in the JanusGraph doc, this is definitely not recommended.

[ptclarke]

@fabiencoppens @Incarnation-p-lee Suspect the change will be broader than simply supporting longs as ids as janusgraph assigns and manages ids itself so most operations will need to find, provide or return the internal id; e.g. "g.addV('person').property('id', 'Thomas').property('firstName', 'Thomas')" becomes "g.addV('person').property('firstName', 'Thomas') and "g.V('Thomas').addE('knows').to(g.V('Mary'))" becomes "g.V().has('firstName','Thomas').addE('knows').to(g.V().has('firstName','Mary'))" [Crude examples]

[Incarnation-p-lee]

@fabiencoppens OK, We can add UT for long id support. But for more professional, we should add IT for this. But our CI env backend graph db do not support that. I can add some UT at first.

[Incarnation-p-lee]

@ptclarke Agree, like findById here means the real internal id from backend graphdb. For no-required id, may need another Annotation for this, as spring-data-commons use id or @Id for real id already.

[fabiencoppens]

@Incarnation-p-lee I can't seem to find your email in your github profile, so as to be able to send you JanusGraph config files for testing purposes

[Incarnation-p-lee]

@fabiencoppens use this panli@microsoft.com

[kschulst]

@Incarnation-p-lee Hi, sorry for delay in following up. I have only had the chance to skim through the changes, but as far as I can see it looks fine to me. Will validate against gremlin-server (and neptune).

[Incarnation-p-lee]

The id supported both Integer and Long with Edge and Vertex, cloud you please help to take a try ? If works well, please help to close this issue.

[fabiencoppens]

@Incarnation-p-lee We've tested with Long ids on vertices in JanusGraph, seems to work fine.

[Incarnation-p-lee]

@fabiencoppens I close this issue for now.