[
https://issues.apache.org/jira/browse/TINKERPOP-2745?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17539472#comment-17539472
]
ASF GitHub Bot commented on TINKERPOP-2745:
-------------------------------------------
spmallette commented on code in PR #1659:
URL: https://github.com/apache/tinkerpop/pull/1659#discussion_r876931423
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/VertexProgram.java:
##########
@@ -48,6 +50,10 @@
public static final String VERTEX_PROGRAM = "gremlin.vertexProgram";
+ default List<Pair<String, Class>> getVertexPropertyKeys() {
Review Comment:
Is there a reason you can't get this information from
`getVertexComputeKeys()`? Seems like it would be better if `VertexComputeKey`
was modified to include the data type which is what you seem to want to add.
It feels like, with this change you'd be making the `VertexProgram` developer
define the keys twice which leaves open the possibility for divergence.
Here's an example from this PR in `ConnectedComponentVertexProgram`:
```java
@Override
public List<Pair<String, Class>> getVertexPropertyKeys() {
return Arrays.asList(
org.apache.commons.lang3.tuple.Pair.of(HALTED_TRAVERSERS,
Object.class),
org.apache.commons.lang3.tuple.Pair.of(ACTIVE_TRAVERSERS,
Object.class),
org.apache.commons.lang3.tuple.Pair.of(COMPONENT, Object.class));
}
```
It defines `COMPONENT` when i think it should be `property`.
I think using the existing `getVertexComputeKeys()` approach also would
solve the testing issue that @mikepersonick brought up because that method is
implicitly tested already - you wouldn't really need to add anything new.
Perhaps you would add some basic unit test to validate the new behavior in the
`VertexComputeKey`?
You targeted `3.5-dev` so you need to do this in a fashion that is
non-breaking. I sense that is possible if you add a new
`VertexComputeKey.of(String, boolean, Class<?>)` and have the old
`of(String,boolean)` call the new method with `Object.class` for the last
argument. Then you could go through all of the existing `VertexProgram`
instances and set the types explicitly. I don't think that will break
anything.
Assuming this all makes sense and I'm not missing anything, please make sure
to include a CHANGELOG entry when you're done.
> Add getter for vertex properties in vertex programs
> ---------------------------------------------------
>
> Key: TINKERPOP-2745
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2745
> Project: TinkerPop
> Issue Type: Improvement
> Reporter: Boxuan Li
> Priority: Major
>
> Many if not all vertex programs will mutate vertex properties during the
> computation process. For graphs that enforce a schema, this could lead to
> failure. JanusGraph, for example, when used with FulgoraGraphComputer and
> disabled automatic schema maker, will report error for vertex programs. See
> [https://gitter.im/janusgraph/janusgraph?at=626c568910cfc315bc5781a0]
>
> This ticket proposes a getter method for each vertex program, which would
> return the vertex properties together with their data types needed in the
> computation. The graph providers or users could then leverage this
> information to create the corresponding schema.
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
