[GitHub] tinkerpop pull request #712: TINKERPOP-1752: Gremlin.Net: Generate completel...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/tinkerpop/pull/712


---

[GitHub] tinkerpop pull request #712: TINKERPOP-1752: Gremlin.Net: Generate completel...

[FlorianHockmann]

Github user FlorianHockmann commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/712#discussion_r141376043
  
--- Diff: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/Bytecode.cs ---
@@ -79,7 +85,77 @@ public void AddSource(string sourceName, params object[] 
args)
 /// The traversal method arguments.
 public void AddStep(string stepName, params object[] args)
 {
-StepInstructions.Add(new Instruction(stepName, args));
+StepInstructions.Add(new Instruction(stepName, 
FlattenArguments(args)));
+Bindings.Clear();
--- End diff --

That's a good point. So we should probably support lambdas. To be honest, I 
haven't really looked into what would be necessary to support lambdas. If I'm 
not mistaken then we can easily support Python and Groovy lambdas when we add a 
dedicated GraphSON serializer for them and probably a class that wraps the 
lambda together with its language. Then the steps that take a lambda would 
simply accept an object of that class in Gremlin.Net. I can create a separate 
ticket for this when we agree that we want to support them.

When we want to support lambdas then we should also support `Bindings`. 
That leaves us with the question of whether we find a better implementation 
that doesn't suffer from the concurrency problems @jorgebay mentioned or if the 
current implementation in this pull request is acceptable.

Apart from that, shouldn't we clarify the section in the documentation 
about bindings? It currently states for example [for 
gremlin-python](http://tinkerpop.apache.org/docs/current/reference/#_bindings):

> If a traversal is going to be executed repeatedly, but with different 
parameters, then bindings should be used.

because:
> translation and compilation can be skipped as the previously compiled 
version should be cached

which is only true when a lambda is used if I understood it correctly.


---

[GitHub] tinkerpop pull request #712: TINKERPOP-1752: Gremlin.Net: Generate completel...

[spmallette]

Github user spmallette commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/712#discussion_r141112905
  
--- Diff: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/Bytecode.cs ---
@@ -79,7 +85,77 @@ public void AddSource(string sourceName, params object[] 
args)
 /// The traversal method arguments.
 public void AddStep(string stepName, params object[] args)
 {
-StepInstructions.Add(new Instruction(stepName, args));
+StepInstructions.Add(new Instruction(stepName, 
FlattenArguments(args)));
+Bindings.Clear();
--- End diff --

Any reason we don't support lambdas? Even if .NET can't support them 
natively for some reason wouldn't we minimally support the ability to pass a 
python/groovy/etc lambda? it's kinda weird that way, but i think back to the 
point that kuppitz made on the dev list the other day where he stated that he 
doesn't always find a way out of using lambdas in production systems he works 
on - so ultimately users will need that kind of capability i think.


---

[GitHub] tinkerpop pull request #712: TINKERPOP-1752: Gremlin.Net: Generate completel...

[jorgebay]

Github user jorgebay commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/712#discussion_r141109744
  
--- Diff: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/Bytecode.cs ---
@@ -79,7 +85,77 @@ public void AddSource(string sourceName, params object[] 
args)
 /// The traversal method arguments.
 public void AddStep(string stepName, params object[] args)
 {
-StepInstructions.Add(new Instruction(stepName, args));
+StepInstructions.Add(new Instruction(stepName, 
FlattenArguments(args)));
+Bindings.Clear();
--- End diff --

I agree that a solution could be an overkill on the API...

I'm +1 from not supporting bindings in the .NET GLV (and document it), I 
don't see much purpose.

What do you think about not supporting Bindings on the .NET GLV @okram 
@spmallette @robertdale ?


---

[GitHub] tinkerpop pull request #712: TINKERPOP-1752: Gremlin.Net: Generate completel...

[FlorianHockmann]

Github user FlorianHockmann commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/712#discussion_r141104538
  
--- Diff: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/Bytecode.cs ---
@@ -79,7 +85,77 @@ public void AddSource(string sourceName, params object[] 
args)
 /// The traversal method arguments.
 public void AddStep(string stepName, params object[] args)
 {
-StepInstructions.Add(new Instruction(stepName, args));
+StepInstructions.Add(new Instruction(stepName, 
FlattenArguments(args)));
+Bindings.Clear();
--- End diff --

This is definitely a problem, but do you see a way to fix it? 
Unfortunately, I think we have to use `static` here unless we want to create 
overloads for each parameter in the Traversal API that takes a `Binding` 
instead of the unbound variable which would result in a lot of overloads.

Since [`Bindings` don't seem to improve the performance with 
Bytecode](https://lists.apache.org/thread.html/c10baff72e67a8b7728c42b5c4e983a83e82b8f6964b3aa465f0e341@)
 when no lambdas are used which we don't support anyway, we may also remove the 
support for `Bindings` altogether from Gremlin.Net. Then we also don't need the 
`FlattenArguments` and `ConvertArgument` functions in `Bytecode` which are not 
exactly intuitive.

Or if we want to continue supporting them, we could also add a note to the 
documentation to make users aware of these concurrency problems with `Bindings`.


---

[GitHub] tinkerpop pull request #712: TINKERPOP-1752: Gremlin.Net: Generate completel...

[jorgebay]

Github user jorgebay commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/712#discussion_r140812056
  
--- Diff: gremlin-dotnet/glv/GraphTraversal.template ---
@@ -65,9 +65,17 @@ namespace Gremlin.Net.Process.Traversal
 /// 
 /// Adds the <%= method.methodName %> step to this .
 /// 
-public GraphTraversal< <%= method.t1 %> , <%= method.t2 %> > <%= 
toCSharpMethodName.call(method.methodName) %><%= method.tParam %> (params 
object[] args)
+public GraphTraversal< <%= method.t1 %> , <%= method.t2 %> > <%= 
toCSharpMethodName.call(method.methodName) %><%= method.tParam %> (<%= 
method.parameters %>)
 {
-Bytecode.AddStep("<%= method.methodName %>", args);
+<%  if (method.parameters.contains("params ")) {
+  %>var args = new List {<%= 
method.paramNames.init().join(", ") %>};
--- End diff --

We should set the initial capacity of the list to avoid unnecessary 
resizing of the underlying array.
It should be the sum of the length of `method.paramNames.init()` and 
`method.paramNames.last()`.


---

[GitHub] tinkerpop pull request #712: TINKERPOP-1752: Gremlin.Net: Generate completel...

[jorgebay]

Github user jorgebay commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/712#discussion_r140818845
  
--- Diff: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/Bindings.cs ---
@@ -29,14 +32,42 @@ namespace Gremlin.Net.Process.Traversal
 public class Bindings
 {
 /// 
+/// Gets an instance of the  class.
+/// 
+public static Bindings Instance { get; } = new Bindings();
+
+private static readonly ThreadLocal> 
BoundVariableByValue =
+new ThreadLocal>();
+
+/// 
 /// Binds the variable to the specified value.
 /// 
 /// The variable to bind.
 /// The value to which the variable should be 
bound.
 /// The bound value.
-public Binding Of(string variable, object value)
+public TV Of(string variable, TV value)
+{
+var dict = BoundVariableByValue.Value;
+if (dict == null)
+{
+dict = new Dictionary();
+BoundVariableByValue.Value = dict;
+}
+dict[value] = variable;
+return value;
+}
+
+internal static string GetBoundVariable(TV value)
+{
+var dict = BoundVariableByValue.Value;
+if (dict == null)
+return null;
+return !dict.ContainsKey(value) ? null : dict[value];
--- End diff --

Use `Dictionary{K, V}.TryGetValue()` to save one operation.


---

[GitHub] tinkerpop pull request #712: TINKERPOP-1752: Gremlin.Net: Generate completel...

[jorgebay]

Github user jorgebay commented on a diff in the pull request:

https://github.com/apache/tinkerpop/pull/712#discussion_r140818483
  
--- Diff: gremlin-dotnet/src/Gremlin.Net/Process/Traversal/Bytecode.cs ---
@@ -79,7 +85,77 @@ public void AddSource(string sourceName, params object[] 
args)
 /// The traversal method arguments.
 public void AddStep(string stepName, params object[] args)
 {
-StepInstructions.Add(new Instruction(stepName, args));
+StepInstructions.Add(new Instruction(stepName, 
FlattenArguments(args)));
+Bindings.Clear();
--- End diff --

Even though `Bindings` is based on a `ThreadLocal` instance, I think the 
implementation is not thread-safe.

For example: Multiple tasks executed serially on the same thread, modifying 
the same bindings dictionary.
Besides not being thread-safe, it doesn't support defining a binding on 1 
thread and adding the step on another, sample:

```csharp
// thread 1
// define bindings
Bindings.Instance.Of(/* */);
await SomeUnRelatedTask();
// thread 2
// Do something with that binding
g.V()/**/;
```


---

[GitHub] tinkerpop pull request #712: TINKERPOP-1752: Gremlin.Net: Generate completel...

[FlorianHockmann]

GitHub user FlorianHockmann opened a pull request:

https://github.com/apache/tinkerpop/pull/712

TINKERPOP-1752: Gremlin.Net: Generate completely type-safe methods

https://issues.apache.org/jira/browse/TINKERPOP-1752

All steps are now (basically) type-safe and the original argument names 
from Gremlin-Java are used. However, we currently don't support some Java types 
like `Comparator`. Those were simply replaced by `object` until we find a 
better solution. A problem with this workaround is that some overloads from 
Gremlin-Java are not supported in Gremlin.Net as they would result in the same 
method signature.

The implementation of `Bindings` is now basically the same as in 
Gremlin-Java. This change was necessary as `Bindings.Of()` can no longer return 
a `Binding` object

These changes also revealed a bug in the tests where the 
`WithoutStrategies` source step was called with objects of strategies instead 
of just with their types. However, `WithoutStrategies` still can't work right 
now as a GraphSON serializer is missing for `Type`.

The `Group` step for example looks now like this:
```cs
/// 
/// Adds the group step to this .
/// 
public GraphTraversal< S , IDictionary > Group ()
{
Bytecode.AddStep("group");
return Wrap< S , IDictionary >(this);
}

/// 
/// Adds the group step to this .
/// 
public GraphTraversal< S , E > Group (string sideEffectKey)
{
Bytecode.AddStep("group", sideEffectKey);
return Wrap< S , E >(this);
}
```
So this should also solve 
[TINKERPOP-1751](https://issues.apache.org/jira/browse/TINKERPOP-1751) that 
mentioned the missing overload for `Group` that returns `IDictionary`.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/apache/tinkerpop TINKERPOP-1752

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/tinkerpop/pull/712.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 #712


commit 05851764f1e20abb1a82b7d662b4681d602a5774
Author: Florian Hockmann 
Date:   2017-08-17T20:57:07Z

Make Gremlin.Net graph traversal API type-safe

All steps are now type-safe and the original argument names from
Gremlin-Java are used. However, we currently don't support some Java
types like Comparator. Those were simply replaced by object until we
find a better solution. A problem of this workaround is that some
overloads from Gremlin-Java are not supported in Gremlin.Net as they
would result in the same method signature.
This required to change how Bindings work as Bindings.Of() can no longer
return a Binding object. The implementation for Bindings is now
basically the same as in Gremlin-Java.

This also revealed a bug in the tests that called the WithoutStrategies 
source step with objects of strategies instead of just with their types. 
However, WithoutStrategies still can't work right now as a GraphSON serializer 
is missing for Type.




---