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/01/12 00:28:25 UTC
[GitHub] [tinkerpop] mikepersonick opened a new pull request #1545: Orderability Semantics (final)
mikepersonick opened a new pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545
Implementation of total orderability semantics.
Resolves:
https://issues.apache.org/jira/browse/TINKERPOP-2604
https://issues.apache.org/jira/browse/TINKERPOP-2606
https://issues.apache.org/jira/browse/TINKERPOP-2622
https://issues.apache.org/jira/browse/TINKERPOP-2641
--
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 removed a comment on pull request #1545: Orderability Semantics (final)
Posted by GitBox <gi...@apache.org>.
mikepersonick removed a comment on pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545#issuecomment-1011158398
Workflow failures for this PR are environmental. All checks for these commits have successfully passed:
https://github.com/mikepersonick/tinkerpop/actions/runs/1688165187
--
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] divijvaidya commented on a change in pull request #1545: Orderability Semantics (final)
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545#discussion_r784765974
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java
##########
@@ -61,13 +61,13 @@
/**
* Configuration key used by {@link GraphFactory}} to determine which graph to instantiate.
*/
- public static final String GRAPH = "gremlin.graph";
+ String GRAPH = "gremlin.graph";
/**
* This should only be used by providers to create keys, labels, etc. in a namespace safe from users.
* Users are not allowed to generate property keys, step labels, etc. that are key'd "hidden".
*/
- public static class Hidden {
+ class Hidden {
Review comment:
ok makes sense.
--
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 edited a comment on pull request #1545: Orderability Semantics (final)
Posted by GitBox <gi...@apache.org>.
mikepersonick edited a comment on pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545#issuecomment-1013145078
I will update the docs with some examples and yes I was planning on doing a squash once we got close, seems like we are!
As for Graph, my philosophy is clean a little bit as you go and eventually everything will be clean.
--
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 merged pull request #1545: Orderability Semantics (final)
Posted by GitBox <gi...@apache.org>.
spmallette merged pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545
--
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 edited a comment on pull request #1545: Orderability Semantics (final)
Posted by GitBox <gi...@apache.org>.
mikepersonick edited a comment on pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545#issuecomment-1011158398
Workflow failures for this PR are environmental. All checks for these commits have successfully passed:
https://github.com/mikepersonick/tinkerpop/actions/runs/1688165187
--
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] divijvaidya commented on a change in pull request #1545: Orderability Semantics (final)
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on a change in pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545#discussion_r782906051
##########
File path: gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js
##########
@@ -298,6 +305,17 @@ function toPath(value) {
return new Path(new Array(0), parts.map(x => parseValue.call(this, x)));
}
+/*
+ * TODO Implement me.
Review comment:
Please create a JIRA and add here
##########
File path: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/driver/remote/AbstractRemoteGraphProvider.java
##########
@@ -206,6 +206,10 @@
test = "org.apache.tinkerpop.gremlin.process.traversal.step.map.UnfoldTest",
method = "g_V_valueMap_unfold_mapXkeyX",
reason = "Tests that include lambdas are not supported by the test suite for remotes")
+@Graph.OptOut(
+ test = "org.apache.tinkerpop.gremlin.process.traversal.step.OrderabilityTest",
+ method = "g_inject_order_with_unknown_type",
+ reason = "Tests that inject a generic Java Object are not supported by the test suite for remotes")
Review comment:
Nice. We can use same reason for at rest of the places.
##########
File path: gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/GherkinTestRunner.cs
##########
@@ -85,6 +85,8 @@ public class GherkinTestRunner
"g_V_hasXperson_name_markoX_bothXknowsX_groupCount_byXvaluesXnameX_foldX",
IgnoreReason.ArrayKeysInMapNotAssertingInGherkin
},
+ {"g_V_properties_order", IgnoreReason.NoReason},
Review comment:
Please add a reason for Ignore here. A comment should be sufficient.
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java
##########
@@ -61,13 +61,13 @@
/**
* Configuration key used by {@link GraphFactory}} to determine which graph to instantiate.
*/
- public static final String GRAPH = "gremlin.graph";
+ String GRAPH = "gremlin.graph";
/**
* This should only be used by providers to create keys, labels, etc. in a namespace safe from users.
* Users are not allowed to generate property keys, step labels, etc. that are key'd "hidden".
*/
- public static class Hidden {
+ class Hidden {
Review comment:
Restricting the scope is a backward incompatible change since vendors might be extending this class. Please revert back to public scope.
Same for other changes in this file.
##########
File path: tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/io/graphson/AbstractTinkerGraphGraphSONTranslatorProvider.java
##########
@@ -242,12 +242,24 @@ public GraphTraversalSource traversal(final Graph graph) {
return g.withStrategies(new TranslationStrategy(g, new GraphSONTranslator<>(JavaTranslator.of(g), version), true));
}
+ @Graph.OptOut(
+ test = "org.apache.tinkerpop.gremlin.process.traversal.step.OrderabilityTest",
+ method = "g_inject_order",
+ reason = "GraphSONv2 does not properly round trip Maps and Sets")
Review comment:
This might not be important for 3.6.x since GraphSonV3 is supposed to be used here but we still haven't deprecated graphsonv2 hence, it requires some attention. Please create a JIRA for this. We will have to investigate what's wrong with graphsonv2 and document the shortcoming.
##########
File path: gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GroovyTranslatorProvider.java
##########
@@ -215,6 +215,10 @@
test = "org.apache.tinkerpop.gremlin.process.traversal.step.map.WriteTest",
method = "*",
reason = "read and write tests don't translate locally well because of calling iterate() inside read()/write() add a none()")
+@Graph.OptOut(
+ test = "org.apache.tinkerpop.gremlin.process.traversal.step.OrderabilityTest",
+ method = "g_inject_order_with_unknown_type",
+ reason = "Cannot round trip a generic Java Object")
Review comment:
Maybe update the reason here to say something about how this doesn't work for remote. To a layman code reader, this leaves a cliffhanger about why cannot be roundtrip generic objects 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 #1545: Orderability Semantics (final)
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545#issuecomment-1013145078
I will update the docs with some examples and yes I was planning on doing a squash once we got close, seems like we are!
--
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] divijvaidya commented on pull request #1545: Orderability Semantics (final)
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545#issuecomment-1013036263
Thank for this contribution Mike! This will go a long way in establishing consistent semantics for users of Gremlin.
Vote +1
--
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 #1545: Orderability Semantics (final)
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545#issuecomment-1013081630
This is a glorious looking PR @mikepersonick (and @divijvaidya is no slouch on the code reviews - watch out @nastra you have competition in the category for "Best Code Reviewer"! 😄 )
Tests look really good. I think the upgrade docs could use a bit of sample code to demonstrate all you wrote there:
```text
gremlin> g.V().values().order() // 3.5.x
java.lang.String cannot be cast to java.lang.Integer
Type ':help' or ':h' for help.
Display stack trace? [yN]n
gremlin> g.V().values().order() // 3.6.0
<whatever the output is>
```
As an administrative note, a rebase to cleanup the commit history and to get us to one commit would be cool (unless you feel some commits are better here individually for some reason).
The changes to Graph.java around removing the method scope isn't a big deal, but it is a bit unlike all our other interface definitions. i've often thought about switching our style there to how you have it as every IDE nags about it. maybe we should just go all in and do that code reformat at some point.
Other than that VOTE +1 - let's see if we can find a final committer to ok this so we can get this merged today.
--
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 #1545: Orderability Semantics (final)
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545#issuecomment-1011158398
Workflow failures are environmental for this PR. All checks for these commits have successfully passed:
https://github.com/mikepersonick/tinkerpop/actions/runs/1688165187
--
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 change in pull request #1545: Orderability Semantics (final)
Posted by GitBox <gi...@apache.org>.
mikepersonick commented on a change in pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545#discussion_r783106049
##########
File path: gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java
##########
@@ -61,13 +61,13 @@
/**
* Configuration key used by {@link GraphFactory}} to determine which graph to instantiate.
*/
- public static final String GRAPH = "gremlin.graph";
+ String GRAPH = "gremlin.graph";
/**
* This should only be used by providers to create keys, labels, etc. in a namespace safe from users.
* Users are not allowed to generate property keys, step labels, etc. that are key'd "hidden".
*/
- public static class Hidden {
+ class Hidden {
Review comment:
I did not restrict the scope. I can revert the changes but all those publics and statics were redundant, since they are inside a public interface.
I also added a new GraphFeature for OrderabilitySemantics.
--
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] krlawrence commented on pull request #1545: Orderability Semantics (final)
Posted by GitBox <gi...@apache.org>.
krlawrence commented on pull request #1545:
URL: https://github.com/apache/tinkerpop/pull/1545#issuecomment-1013216832
I did not have time to build the branch but have read every diff. Looks really good @mikepersonick - assuming the comments from @divijvaidya and @spmallette are accounted for a big thank you and a
VOTE +1
--
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