You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2022/05/16 02:46:12 UTC
[GitHub] [tinkerpop] li-boxuan opened a new pull request, #1659: TINKERPOP-2745: Add getter method for properties
li-boxuan opened a new pull request, #1659:
URL: https://github.com/apache/tinkerpop/pull/1659
This commit adds VertexProgram::getVertexPropertyKeys method
which returns a list of (property key, class type) pairs. The
pairs contain vertex property keys that are read or written in
the execution of the vertex program. Graph providers could
leverage this information to create schema ahead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] li-boxuan commented on a diff in pull request #1659: TINKERPOP-2745: Add getter method for properties
Posted by GitBox <gi...@apache.org>.
li-boxuan commented on code in PR #1659:
URL: https://github.com/apache/tinkerpop/pull/1659#discussion_r878761723
##########
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:
> It defines COMPONENT when i think it should be property
You are right, I was wrong. I used the default property name in the test so didn't catch this bug.
> Is there a reason you can't get this information from getVertexComputeKeys()
Thanks, I totally forgot about it 😅 . That should mostly suffice the use case I described, except that I am not 100% sure why other vertex programs don't contain `HALTED_TRAVERSERS` and `ACTIVE_TRAVERSERS` in `vertexComputeKeys`. Anyways, this PR is not very useful now so I am closing it. Thank you @mikepersonick and @spmallette for your reviews!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] li-boxuan commented on pull request #1659: TINKERPOP-2745: Add getter method for properties
Posted by GitBox <gi...@apache.org>.
li-boxuan commented on PR #1659:
URL: https://github.com/apache/tinkerpop/pull/1659#issuecomment-1127161042
I am not sure how to test them in the TinkerPop codebase, but I have verified them using JanusGraph.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1659: TINKERPOP-2745: Add getter method for properties
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1659:
URL: https://github.com/apache/tinkerpop/pull/1659#discussion_r876908792
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/clustering/connected/ConnectedComponentVertexProgram.java:
##########
@@ -43,12 +44,10 @@
import org.apache.tinkerpop.gremlin.structure.Vertex;
import org.apache.tinkerpop.gremlin.structure.VertexProperty;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashSet;
-import java.util.Iterator;
-import java.util.Set;
+import java.util.*;
Review Comment:
nit: please check your IDE settings - looks like it added a wildcard import. the tinkerpop style is to be explicit with import
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1659: TINKERPOP-2745: Add getter method for properties
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1659:
URL: https://github.com/apache/tinkerpop/pull/1659#discussion_r876910116
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/clustering/connected/ConnectedComponentVertexProgram.java:
##########
@@ -76,6 +75,14 @@ public class ConnectedComponentVertexProgram implements VertexProgram<String> {
private ConnectedComponentVertexProgram() {}
+ @Override
+ public List<Pair<String, Class>> getVertexPropertyKeys() {
+ return Arrays.asList(
+ org.apache.commons.lang3.tuple.Pair.of(HALTED_TRAVERSERS, Object.class),
Review Comment:
Please prefer `org.javatuples.Pair` as that is used throughout the project.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] li-boxuan commented on pull request #1659: TINKERPOP-2745: Add getter method for properties
Posted by GitBox <gi...@apache.org>.
li-boxuan commented on PR #1659:
URL: https://github.com/apache/tinkerpop/pull/1659#issuecomment-1130943291
> Would you mind taking a look at these and see if there is a way to add tests somehow
TinkerGraph itself is schemaless so I cannot think of a way to test the new getter methods. It is possible to add a new graph provider in the test package which enforces some sort of schema so that these new getter methods can be tested. That being said, I am not sure if that is worth it and if that is the best practice. Maybe @spmallette has some insights here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] mikepersonick commented on pull request #1659: TINKERPOP-2745: Add getter method for properties
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on PR #1659:
URL: https://github.com/apache/tinkerpop/pull/1659#issuecomment-1128938260
Would you mind taking a look at these and see if there is a way to add tests somehow?
https://github.com/apache/tinkerpop/blob/master/gremlin-test/features/map/PageRank.feature
https://github.com/apache/tinkerpop/blob/master/gremlin-test/features/map/PeerPressure.feature
https://github.com/apache/tinkerpop/blob/master/gremlin-test/features/map/ShortestPath.feature
And in the associated Java tests here:
https://github.com/apache/tinkerpop/tree/master/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #1659: TINKERPOP-2745: Add getter method for properties
Posted by GitBox <gi...@apache.org>.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] li-boxuan closed pull request #1659: TINKERPOP-2745: Add getter method for properties
Posted by GitBox <gi...@apache.org>.
li-boxuan closed pull request #1659: TINKERPOP-2745: Add getter method for properties
URL: https://github.com/apache/tinkerpop/pull/1659
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org