[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/792 ---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user FlorianHockmann commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/792#discussion_r175266150
--- Diff:
gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/TraversalEvaluation/TraversalParser.cs
---
@@ -398,7 +398,7 @@ private static ITokenParameter ParseParameter(string
text, ref int i)
{
return ParseNumber(text, ref i);
}
-if (text.Substring(i, 3).StartsWith("__."))
+if (text.Length >= i + 3 && text.Substring(i,
3).StartsWith("__."))
--- End diff --
Done
---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user dkuppitz commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/792#discussion_r175199478
--- Diff:
gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/TraversalEvaluation/TraversalParser.cs
---
@@ -398,7 +398,7 @@ private static ITokenParameter ParseParameter(string
text, ref int i)
{
return ParseNumber(text, ref i);
}
-if (text.Substring(i, 3).StartsWith("__."))
+if (text.Length >= i + 3 && text.Substring(i,
3).StartsWith("__."))
--- End diff --
Not a big deal, but with the length check included, it can be an equality
check, rather than `StartsWith()`.
---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user jorgebay commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/792#discussion_r175032311
--- Diff: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/Lambda.cs ---
@@ -0,0 +1,59 @@
+#region License
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#endregion
+
+namespace Gremlin.Net.Process.Traversal
+{
+///
+/// Represents a lambda.
+///
+public interface ILambda : IFunction, IBiFunction, IPredicate,
IUnaryOperator, IBinaryOperator, IComparator,
+IConsumer, ISupplier
+{
--- End diff --
We could add the properties to the `ILambda` interface, in the same way
it's done in java:
https://github.com/apache/tinkerpop/blob/f5f4a046f211851e901d303c6593975314171e55/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/function/Lambda.java#L33-L39
The risk of that interface ever changing is shared with Java and a single
serializer (for any `ILambda`) will only be needed.
---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user jorgebay commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/792#discussion_r170027393 --- Diff: gremlin-dotnet/glv/generate.groovy --- @@ -48,7 +48,7 @@ def toCSharpTypeMap = ["Long": "long", "TraversalMetrics": "E2", "Traversal": "ITraversal", "Traversal[]": "ITraversal[]", - "Predicate": "TraversalPredicate", + "Predicate": "object", --- End diff -- Makes sense ð ---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user FlorianHockmann commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/792#discussion_r170006977 --- Diff: gremlin-dotnet/glv/generate.groovy --- @@ -48,7 +48,7 @@ def toCSharpTypeMap = ["Long": "long", "TraversalMetrics": "E2", "Traversal": "ITraversal", "Traversal[]": "ITraversal[]", - "Predicate": "TraversalPredicate", + "Predicate": "object", --- End diff -- You're right, we should solve this problem, but I think that the changes for that are big enough to validate its own issue and it's also somewhat unrelated to this PR. So I created a new issue for that: [TINKERPOP-1901](https://issues.apache.org/jira/browse/TINKERPOP-1901). I suggest that we resolve that one first and then I simply rebase this PR afterwards and let the `Lambda` class implement whichever interface we introduce there for those cases. ---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user jorgebay commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/792#discussion_r167847541 --- Diff: gremlin-dotnet/glv/generate.groovy --- @@ -48,7 +48,7 @@ def toCSharpTypeMap = ["Long": "long", "TraversalMetrics": "E2", "Traversal": "ITraversal", "Traversal[]": "ITraversal[]", - "Predicate": "TraversalPredicate", + "Predicate": "object", --- End diff -- Sorry, I've read it a while back when the pull request was first opened and forgot about it :/ There are several consequences of using an untyped API for those methods: - We are stuck maintaining an API for which we can't predict how its being used (specially considering that db providers might support additional functionality on top of the GLV). - The user looses compile time checks. - It will be hard for the user to find the correct expected type (`Lambda`) and the error message on serialization might be unclear. I think we should avoid using `object` for predicates. We could use different workarounds for the `T` issue you highlighted above: - Use a class for `T` (not an enum), properties like `T.Id` could return instances of whatever interface we create for it. Given that java enums functionality is more comprehensive than what C# currently supports, it makes sense to use a class IMO. - Generate the offending traversal methods manually. ---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user FlorianHockmann commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/792#discussion_r167844157
--- Diff:
gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphSON/LambdaSerializer.cs ---
@@ -0,0 +1,45 @@
+#region License
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#endregion
+
+using System.Collections.Generic;
+using Gremlin.Net.Process.Traversal;
+
+namespace Gremlin.Net.Structure.IO.GraphSON
+{
+internal class LambdaSerializer : IGraphSONSerializer
+{
+private const int DefaultArgument = -1;
+
+public Dictionary Dictify(dynamic objectData,
GraphSONWriter writer)
+{
+Lambda lambda = objectData;
--- End diff --
Would you prefer the interface even if we don't use it in the traversal API
(see my response to your other comment)? I usually don't introduce interfaces
for classes that only hold data without any functionality, but I can of course
add such an interface if you prefer it that way.
---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user FlorianHockmann commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/792#discussion_r167843703
--- Diff: docs/src/reference/gremlin-variants.asciidoc ---
@@ -436,6 +438,22 @@ NOTE: Many of the TraversalStrategy classes in
Gremlin.Net are proxies to the re
JVM-based Gremlin traversal machine. As such, their `Apply(ITraversal)`
method does nothing. However, the strategy is
encoded in the Gremlin.Net bytecode and transmitted to the Gremlin
traversal machine for re-construction machine-side.
+=== The Lambda Solution
+
+Supporting link:https://en.wikipedia.org/wiki/Anonymous_function[anonymous
functions] across languages is difficult as
+most languages do not support lambda introspection and thus, code
analysis. While Gremlin.Net doesn't support C# lambdas, it
+is still able to represent lambdas in other languages. When the lambda is
represented in `Bytecode` its language is encoded
+such that the remote connection host can infer which translator and
ultimate execution engine to use.
+
+[source,csharp]
+
+g.V().Out().Map(Lambda.Groovy("it.get().value('name').length()")).Sum().ToList();
<1>
--- End diff --
I don't think that we need curly brackets here. It also looks like this for
Gremlin-Python in the docs: `map(lambda: ("it.get().value('name').length()",
"gremlin-groovy"))`
---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user FlorianHockmann commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/792#discussion_r167842126 --- Diff: gremlin-dotnet/glv/generate.groovy --- @@ -48,7 +48,7 @@ def toCSharpTypeMap = ["Long": "long", "TraversalMetrics": "E2", "Traversal": "ITraversal", "Traversal[]": "ITraversal[]", - "Predicate": "TraversalPredicate", + "Predicate": "object", --- End diff -- I don't know if you saw what I wrote in the initial post of this PR, but the problem is that those steps expect a `Function` in Java which is also implemented by enums like `T`. So, when we restrict them to something like `ILambda` in .NET then they can't take enum values anymore as enums can't implement interfaces in .NET. ---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user jorgebay commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/792#discussion_r167791719
--- Diff: docs/src/reference/gremlin-variants.asciidoc ---
@@ -436,6 +438,22 @@ NOTE: Many of the TraversalStrategy classes in
Gremlin.Net are proxies to the re
JVM-based Gremlin traversal machine. As such, their `Apply(ITraversal)`
method does nothing. However, the strategy is
encoded in the Gremlin.Net bytecode and transmitted to the Gremlin
traversal machine for re-construction machine-side.
+=== The Lambda Solution
+
+Supporting link:https://en.wikipedia.org/wiki/Anonymous_function[anonymous
functions] across languages is difficult as
+most languages do not support lambda introspection and thus, code
analysis. While Gremlin.Net doesn't support C# lambdas, it
+is still able to represent lambdas in other languages. When the lambda is
represented in `Bytecode` its language is encoded
+such that the remote connection host can infer which translator and
ultimate execution engine to use.
+
+[source,csharp]
+
+g.V().Out().Map(Lambda.Groovy("it.get().value('name').length()")).Sum().ToList();
<1>
--- End diff --
Should this be `Lambda.Groovy("{ it.get().value('name').length() }")`, with
curly brackets?
---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user jorgebay commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/792#discussion_r167790135
--- Diff:
gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Gherkin/IgnoreException.cs ---
@@ -40,9 +40,6 @@ private static string GetMessage(IgnoreReason reason)
string reasonSuffix = null;
switch (reason)
{
-case IgnoreReason.LambdaNotSupported:
--- End diff --
Can we replace it with a comment, like `// Include detailed reason for each
enum here`?
---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user jorgebay commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/792#discussion_r167789173 --- Diff: gremlin-dotnet/glv/generate.groovy --- @@ -48,7 +48,7 @@ def toCSharpTypeMap = ["Long": "long", "TraversalMetrics": "E2", "Traversal": "ITraversal", "Traversal[]": "ITraversal[]", - "Predicate": "TraversalPredicate", + "Predicate": "object", --- End diff -- I think we should use an interface `ILambda` similar to java's counterpart: https://github.com/apache/tinkerpop/blob/3.2.7/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/function/Lambda.java#L33 `Lambda.Groovy()` and `Lambda.Python()` could return `ILambda` instances. That way we can have a compile type check for methods like `Filter()`, `Until()`, etc... to accept only `ILambda` instances. ---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user jorgebay commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/792#discussion_r167789705
--- Diff:
gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphSON/LambdaSerializer.cs ---
@@ -0,0 +1,45 @@
+#region License
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#endregion
+
+using System.Collections.Generic;
+using Gremlin.Net.Process.Traversal;
+
+namespace Gremlin.Net.Structure.IO.GraphSON
+{
+internal class LambdaSerializer : IGraphSONSerializer
+{
+private const int DefaultArgument = -1;
+
+public Dictionary Dictify(dynamic objectData,
GraphSONWriter writer)
+{
+Lambda lambda = objectData;
--- End diff --
The same, rely on an interface instead of an implementation, use `ILambda`.
---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
Github user jorgebay commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/792#discussion_r167789593
--- Diff:
gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphSON/LambdaSerializer.cs ---
@@ -0,0 +1,45 @@
+#region License
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#endregion
+
+using System.Collections.Generic;
+using Gremlin.Net.Process.Traversal;
+
+namespace Gremlin.Net.Structure.IO.GraphSON
+{
+internal class LambdaSerializer : IGraphSONSerializer
+{
+private const int DefaultArgument = -1;
+
+public Dictionary Dictify(dynamic objectData,
GraphSONWriter writer)
+{
+Lambda lambda = objectData;
+var valueDict = new Dictionary
+{
+{"script", lambda.LambdaExpression},
+{"language", lambda.Language},
+{"arguments", DefaultArgument}
--- End diff --
Use `lambda.Arguments` (which for the existent implementations would return
`-1`).
---
[GitHub] tinkerpop pull request #792: TINKERPOP-1854 Add Lambdas for Gremlin.Net
GitHub user FlorianHockmann opened a pull request:
https://github.com/apache/tinkerpop/pull/792
TINKERPOP-1854 Add Lambdas for Gremlin.Net
https://issues.apache.org/jira/browse/TINKERPOP-1854
Since we don't have a way to parse C# lambdas on server side, this PR
simply allows to send lambdas in other languages with Gremlin.Net:
- For a Groovy lambda: `Lambda.Groovy("it.get().value('name').length()")`
- and for a Python lambda: `Lambda.Python("lambda x:
len(x.get().value('name'))")`
All features are passing now - no features are ignored any more. Therefore
I also removed the functionality to ignore features in Gremlin.Net. (I hope
that we won't need to ignore any features in Gremlin.Net in the future, but if
we have to then it shouldn't be an issue to get this functionality back.)
Unfortunately, it's not possible to use this new `Lambda` type for
arguments in the traversal API due to the fact that in Java the type `Function`
would correspond to this type, but Java also allows to pass for example the
enum `T` as a `Function` because `T` implements `Function`. So
we cannot simply replace all `Function` types from Gremlin-Java with `Lambda`
in Gremlin.Net as that would break for example this step:
```
public default GraphTraversal by(final Function
function,
final Comparator comparator)
```
that can be called like this: `by(T.value, Order.incr)` which wouldn't be
possible when the first argument would only take a `Lambda` in Gremlin.Net.
(Luckily, I found this thanks to a Gherkin scenario that makes exactly this
call.)
So, currently all arguments where a lambda is possible simply expect an
`object`.
VOTE +1
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/tinkerpop TINKERPOP-1854
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/792.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #792
commit 976079d6e7ded8b7f71605adba102ce530a4ecde
Author: Florian Hockmann
Date: 2018-01-27T13:41:28Z
TINKERPOP-1854 Add Lambdas for Gremlin.Net
All features are passing now (no ignored ones any more).
---
