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/14 22:20:57 UTC
[GitHub] [tinkerpop] li-boxuan opened a new pull request, #1657: TINKERPOP-2742 Give graph providers a hint of property cardinality
li-boxuan opened a new pull request, #1657:
URL: https://github.com/apache/tinkerpop/pull/1657
Currently, Graph::getCardinality only takes a vertex property key as the parameter,
which sometimes is limited for the graph provider to make a proper choice
about the cardinality of the vertex property. This commit overloads this method
which takes both the property key and values as parameters so that the graph
provider can hopefully make a better choice. For example, a graph provider can
use list/set cardinality when it sees multiple values and uses single
cardinality when it only sees one value. This is particularly useful when loading
a graph using the IO step without a pre-defined schema.
--
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 #1657: TINKERPOP-2742 Give graph providers a hint of property cardinality
Posted by GitBox <gi...@apache.org>.
li-boxuan commented on code in PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#discussion_r873080619
##########
tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerGraphProvider.java:
##########
@@ -107,7 +107,7 @@ protected static boolean requiresPersistence(final Class<?> test, final String t
*/
protected static boolean requiresListCardinalityAsDefault(final LoadGraphWith.GraphData loadGraphWith,
final Class<?> test, final String testMethodName) {
- return loadGraphWith == LoadGraphWith.GraphData.CREW
+ return loadGraphWith == LoadGraphWith.GraphData.CREW && !testMethodName.startsWith("shouldReadWriteCrew")
Review Comment:
We don't set default vertex property cardinality as LIST for `shouldReadWriteCrew` test case. By doing so, `shouldReadWriteCrew` will fail without the changes in this PR because TinkerPop will use SINGLE cardinality by default.
--
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 #1657: TINKERPOP-2742 Give graph providers a hint of property cardinality
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#discussion_r876938569
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java:
##########
@@ -42,14 +42,7 @@
import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
import java.io.File;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Set;
-import java.util.UUID;
+import java.util.*;
Review Comment:
nit: please remove wildcard imports
--
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 a diff in pull request #1657: TINKERPOP-2742 Give graph providers a hint of property cardinality
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on code in PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#discussion_r874888967
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/Attachable.java:
##########
@@ -291,12 +290,18 @@ public static Vertex createVertex(final Attachable<Vertex> attachableVertex, fin
final Vertex vertex = hostGraph.features().vertex().willAllowId(baseVertex.id()) ?
hostGraph.addVertex(T.id, baseVertex.id(), T.label, baseVertex.label()) :
hostGraph.addVertex(T.label, baseVertex.label());
- baseVertex.properties().forEachRemaining(vp -> {
- final VertexProperty vertexProperty = hostGraph.features().vertex().properties().willAllowId(vp.id()) ?
- vertex.property(hostGraph.features().vertex().getCardinality(vp.key()), vp.key(), vp.value(), T.id, vp.id()) :
- vertex.property(hostGraph.features().vertex().getCardinality(vp.key()), vp.key(), vp.value());
- vp.properties().forEachRemaining(p -> vertexProperty.property(p.key(), p.value()));
- });
+ Map<String, List<VertexProperty<Object>>> propertyMap = new HashMap<>();
Review Comment:
nit: final
##########
tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerGraphProvider.java:
##########
@@ -107,7 +107,7 @@ protected static boolean requiresPersistence(final Class<?> test, final String t
*/
protected static boolean requiresListCardinalityAsDefault(final LoadGraphWith.GraphData loadGraphWith,
final Class<?> test, final String testMethodName) {
- return loadGraphWith == LoadGraphWith.GraphData.CREW
+ return loadGraphWith == LoadGraphWith.GraphData.CREW && !testMethodName.startsWith("shouldReadWriteCrew")
Review Comment:
Is it possible to fix the test case instead? I don't think we should hard code test names into the graph provider code. If it's not possible to fix we'll need to create an annotation for this case.
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java:
##########
@@ -411,6 +404,15 @@ public boolean willAllowId(final Object id) {
public VertexProperty.Cardinality getCardinality(final String key) {
return defaultVertexPropertyCardinality;
}
+
+ @Override
+ public VertexProperty.Cardinality getCardinality(final String key, final Object... values) {
+ VertexProperty.Cardinality cardinality = defaultVertexPropertyCardinality;
Review Comment:
nit: final
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/Attachable.java:
##########
@@ -291,12 +290,18 @@ public static Vertex createVertex(final Attachable<Vertex> attachableVertex, fin
final Vertex vertex = hostGraph.features().vertex().willAllowId(baseVertex.id()) ?
hostGraph.addVertex(T.id, baseVertex.id(), T.label, baseVertex.label()) :
hostGraph.addVertex(T.label, baseVertex.label());
- baseVertex.properties().forEachRemaining(vp -> {
- final VertexProperty vertexProperty = hostGraph.features().vertex().properties().willAllowId(vp.id()) ?
- vertex.property(hostGraph.features().vertex().getCardinality(vp.key()), vp.key(), vp.value(), T.id, vp.id()) :
- vertex.property(hostGraph.features().vertex().getCardinality(vp.key()), vp.key(), vp.value());
- vp.properties().forEachRemaining(p -> vertexProperty.property(p.key(), p.value()));
- });
+ Map<String, List<VertexProperty<Object>>> propertyMap = new HashMap<>();
+ baseVertex.properties().forEachRemaining(vp -> propertyMap.computeIfAbsent(vp.key(), k -> new ArrayList<>()).add(vp));
+ for (Map.Entry<String, List<VertexProperty<Object>>> entry : propertyMap.entrySet()) {
+ VertexProperty.Cardinality cardinality = hostGraph.features().vertex().getCardinality(
Review Comment:
nit: final
--
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] codecov-commenter commented on pull request #1657: TINKERPOP-2742 Give graph providers a hint of property cardinality
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#issuecomment-1126821628
# [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1657?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1657](https://codecov.io/gh/apache/tinkerpop/pull/1657?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dc69004) into [3.5-dev](https://codecov.io/gh/apache/tinkerpop/commit/b170271060e52c129903fa32f8ffebc8617a3797?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b170271) will **decrease** coverage by `5.33%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## 3.5-dev #1657 +/- ##
===========================================
- Coverage 72.22% 66.89% -5.34%
===========================================
Files 25 23 -2
Lines 4353 3510 -843
===========================================
- Hits 3144 2348 -796
+ Misses 1015 976 -39
+ Partials 194 186 -8
```
| [Impacted Files](https://codecov.io/gh/apache/tinkerpop/pull/1657?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [gremlin-go/driver/cucumber/cucumberWorld.go](https://codecov.io/gh/apache/tinkerpop/pull/1657/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1nby9kcml2ZXIvY3VjdW1iZXIvY3VjdW1iZXJXb3JsZC5nbw==) | | |
| [gremlin-go/driver/cucumber/gremlin.go](https://codecov.io/gh/apache/tinkerpop/pull/1657/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1nby9kcml2ZXIvY3VjdW1iZXIvZ3JlbWxpbi5nbw==) | | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1657?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1657?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [b170271...dc69004](https://codecov.io/gh/apache/tinkerpop/pull/1657?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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 #1657: TINKERPOP-2742 Give graph providers a hint of property cardinality
Posted by GitBox <gi...@apache.org>.
li-boxuan commented on code in PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#discussion_r877020771
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java:
##########
@@ -411,6 +404,15 @@ public boolean willAllowId(final Object id) {
public VertexProperty.Cardinality getCardinality(final String key) {
return defaultVertexPropertyCardinality;
}
+
+ @Override
+ public VertexProperty.Cardinality getCardinality(final String key, final Object... values) {
Review Comment:
I totally agree with your concerns and it makes sense to be cautious about breaking changes. I would like to formulate the problems in two aspects:
1. Is it worth adding this new interface in general (with default behavior preserved)?
Maybe. It might be impossible or discouraged to change the default cardinality on the fly for some graph implementers. That being said, your concerns about SubgraphStep, PartitionStrategy, etc. still holds here.
2. Is it worth introducing this breaking change in TinkerGraph?
Probably not due to the reasons you gave. However, I think it is still worth considering the following actions:
a) Add a reminder to IO step on doc. I personally didn’t know that TinkerPop would not infer cardinality from input file. The fact that you might lose data (list/set properties) when you export and import again really confused me for some time. Not sure if it is documented somewhere else, but I read the source code to finally figure it out.
b) We can still infer the cardinality but we don’t really use it to preserve old behavior. Instead, we give a warning to users when we detect discrepancy between inferred cardinality and default cardinality.
--
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 closed pull request #1657: TINKERPOP-2742 Give graph providers a hint of property cardinality
Posted by GitBox <gi...@apache.org>.
spmallette closed pull request #1657: TINKERPOP-2742 Give graph providers a hint of property cardinality
URL: https://github.com/apache/tinkerpop/pull/1657
--
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 #1657: TINKERPOP-2742 Give graph providers a hint of property cardinality
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#discussion_r876979160
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java:
##########
@@ -411,6 +404,15 @@ public boolean willAllowId(final Object id) {
public VertexProperty.Cardinality getCardinality(final String key) {
return defaultVertexPropertyCardinality;
}
+
+ @Override
+ public VertexProperty.Cardinality getCardinality(final String key, final Object... values) {
Review Comment:
This changes the behavior of TinkerGraph in relation to attachable because it overrides the default cardinality based on the data in the attachable rather than letting the TinkerGraph configuration for cardinality do its job. I'm not sure that's wrong, but it is different and a breaking change. I honestly don't know which approach is more intuitive for a user:
1. Currently: The graph has a global configuration for cardinality. If you were reading in some data from a GraphSON file you would have to know what that it contained multi-properties and then configure the graph to use `list` or `set` rather than the default of `single` to get it to read properly.
2. After this change: The graph still has a global configuration for cardinality, except when reading in some data (i.e. using attachables) at which point the user gets a dynamic behavior to individually decide per property/value what the cardinality will be.
The fact that it is an exception and relevant only to attachables is what gives me pause. Also, what about `SubgraphStep`, `PartitionStrategy` and other places where the current `getCardinality(String)` is used? Should this behavior be there as well somehow?
Ultimately, I think there are two questions:
1. Is it helpful for TinkerGraph's behavior to change such that the default cardinality configuration can be overridden dynamically (perhaps that too is a configuration to preserve the old behavior)?
2. If it is helpful and we want that change, how is it used universally with as few exceptions as possible and if we can't get rid of all exceptions can we live with those that must stay?
@krlawrence any thoughts on this?
--
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 #1657: TINKERPOP-2742 Give graph providers a hint of property cardinality
Posted by GitBox <gi...@apache.org>.
li-boxuan commented on code in PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#discussion_r876504970
##########
tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerGraphProvider.java:
##########
@@ -107,7 +107,7 @@ protected static boolean requiresPersistence(final Class<?> test, final String t
*/
protected static boolean requiresListCardinalityAsDefault(final LoadGraphWith.GraphData loadGraphWith,
final Class<?> test, final String testMethodName) {
- return loadGraphWith == LoadGraphWith.GraphData.CREW
+ return loadGraphWith == LoadGraphWith.GraphData.CREW && !testMethodName.startsWith("shouldReadWriteCrew")
Review Comment:
The test `shouldReadWriteCrew` actually works fine without this change. However, in the old logic, whenever the graph is `CREW`, we always use LIST cardinality as the default cardinality in tests, which conceals the problem described in https://issues.apache.org/jira/projects/TINKERPOP/issues/TINKERPOP-2742.
While I agree hardcoding is not a good practice, this method already hardcodes some test names including `shouldAttachWithCreateMethod` and `testAttachableCreateMethod`. Since this is just a graph provider under the `test` package, I presume it is okay to do so (although not perfect).
--
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 pull request #1657: TINKERPOP-2742 Give graph providers a hint of property cardinality
Posted by GitBox <gi...@apache.org>.
spmallette commented on PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#issuecomment-1160732540
new PR at: #1719
--
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 #1657: TINKERPOP-2742 Give graph providers a hint of property cardinality
Posted by GitBox <gi...@apache.org>.
li-boxuan commented on code in PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#discussion_r876504970
##########
tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/TinkerGraphProvider.java:
##########
@@ -107,7 +107,7 @@ protected static boolean requiresPersistence(final Class<?> test, final String t
*/
protected static boolean requiresListCardinalityAsDefault(final LoadGraphWith.GraphData loadGraphWith,
final Class<?> test, final String testMethodName) {
- return loadGraphWith == LoadGraphWith.GraphData.CREW
+ return loadGraphWith == LoadGraphWith.GraphData.CREW && !testMethodName.startsWith("shouldReadWriteCrew")
Review Comment:
The test `shouldReadWriteCrew` actually works fine without this change. However, in the old logic, whenever the graph is `CREW`, we always use LIST cardinality as the default cardinality in tests, which conceals the problem described in https://issues.apache.org/jira/projects/TINKERPOP/issues/TINKERPOP-2742.
While I agree hardcoding is not a good practice, this method already hardcodes some test names including `shouldAttachWithCreateMethod` and `testAttachableCreateMethod`. Since this is just a graph provider under the `test` package, I presume it is okay to do so.
--
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 #1657: TINKERPOP-2742 Give graph providers a hint of property cardinality
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#discussion_r898437742
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java:
##########
@@ -411,6 +404,15 @@ public boolean willAllowId(final Object id) {
public VertexProperty.Cardinality getCardinality(final String key) {
return defaultVertexPropertyCardinality;
}
+
+ @Override
+ public VertexProperty.Cardinality getCardinality(final String key, final Object... values) {
Review Comment:
sorry, @li-boxuan i forgot to come back to this. i think your suggestions in (a) and (b) make sense. a documentation update and a warning to the logs. do you have time to make those changes?
--
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 #1657: TINKERPOP-2742 Give graph providers a hint of property cardinality
Posted by GitBox <gi...@apache.org>.
li-boxuan commented on code in PR #1657:
URL: https://github.com/apache/tinkerpop/pull/1657#discussion_r898463715
##########
tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java:
##########
@@ -411,6 +404,15 @@ public boolean willAllowId(final Object id) {
public VertexProperty.Cardinality getCardinality(final String key) {
return defaultVertexPropertyCardinality;
}
+
+ @Override
+ public VertexProperty.Cardinality getCardinality(final String key, final Object... values) {
Review Comment:
Sure, I'll do that shortly!
--
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