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/10/24 19:37:51 UTC
[GitHub] [tinkerpop] vkagamlyk opened a new pull request, #1843: [WIP] Properties on response elements
vkagamlyk opened a new pull request, #1843:
URL: https://github.com/apache/tinkerpop/pull/1843
Current status:
- added finalization strategy `DetachStrategy(mode, properties)`
- added unit tests for `DetachedVertex` and `DetachedEdge`
Next:
- integration tests
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1108905157
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -34,15 +34,16 @@ public class GremlinScriptChecker {
/**
* An empty result whose properties return as empty.
*/
- public static final Result EMPTY_RESULT = new Result(null, null);
+ public static final Result EMPTY_RESULT = new Result(null, null, null);
/**
* At least one of these tokens should be present somewhere in the Gremlin string for {@link #parse(String)} to
* take any action at all.
*/
+ // todo: why not used `Tokens`?
Review Comment:
not sure what this todo was for, but you should include the `Tokens` version here too in the caps form.
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1108952713
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js:
##########
@@ -131,17 +131,30 @@ Given(/^using the parameter (.+) defined as "(.+)"$/, function (paramName, strin
});
});
+var removeProperties = function(p) {
+ if (p === undefined) {
+ } else if (p instanceof graphModule.Vertex || p instanceof graphModule.Edge) {
+ p.properties = undefined;
+ } else if (p instanceof Array) {
+ p.forEach(removeProperties)
+ } else if (p instanceof Map) {
+ removeProperties(Array.from(p.keys()))
+ removeProperties(Array.from(p.values()))
+ } else if (p instanceof graphModule.Path) {
+ removeProperties(p.objects)
+ }
+
+ return p
+}
+
When('iterated to list', function () {
- return this.traversal.toList().then(list => this.result = list).catch(err => this.result = err);
+ return this.traversal.toList().then(list => this.result = removeProperties(list)).catch(err => this.result = err);
Review Comment:
could you please explain what's happening here with `removeProperties()` and related changes elsewhere referring to "properties excluded from verification"? i'm not sure i follow
--
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] kenhuuu commented on a diff in pull request #1843: [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
kenhuuu commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1004868436
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/detached/PropertyUtil.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.structure.util.detached;
+
+import org.apache.commons.lang3.ArrayUtils;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization.DetachStrategy;
+import org.apache.tinkerpop.gremlin.structure.Property;
+
+/**
+ * Helper class that is used to calculate whether a property matches a template.
+ * Can be expanded to support wildcards or Regexp.
+ *
+ * @author Valentyn Kahamlyk
+ */
+public class PropertyUtil {
+ public static boolean Match(final DetachStrategy.DetachOptions detachOptions, final Property property) {
Review Comment:
This method should be named "match".
--
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 #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1016646397
##########
docs/src/reference/the-traversal.asciidoc:
##########
@@ -4916,6 +4916,28 @@ g.V().hasLabel('person'). <1>
<8> `PathRetractionStrategy` will remove paths from the traversers and increase the likelihood of bulking as path data is not required after `select('b')`.
<9> `AdjacentToIncidentStrategy` will turn `out()` into `outE()` to increase data access locality.
+=== DetachStrategy
+
+`DetachStrategy` manages the fields that will be included in the result. The strategy adds a step to the end of the traversal
+and does not affect the previous steps.
+There are 3 options for detachMode: `ALL` for all properties, `NONE` for no properties, `CUSTOM` for a specific list of properties.
+`id` and `label` always included in response.
+Same for Vertexes and Edges.
+
+WARNING: by default Gremlin Server configured to use `ReferenceElementStrategy` which removes all properties.
Review Comment:
1. What happens if `ReferenceElementStrategy` and `DetachStrategy` are both used at the same time?
2. I wonder what should happen to `ReferenceElementStrategy` in 3.7.x...is it even relevant anymore as there is `DetachStrategy` with mode `NONE` which is the same thing basically? I guess one returns a `ReferenceElement` and the other a `DetachedElement` with no properties so there is that difference. thoughts?
--
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 #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1016646397
##########
docs/src/reference/the-traversal.asciidoc:
##########
@@ -4916,6 +4916,28 @@ g.V().hasLabel('person'). <1>
<8> `PathRetractionStrategy` will remove paths from the traversers and increase the likelihood of bulking as path data is not required after `select('b')`.
<9> `AdjacentToIncidentStrategy` will turn `out()` into `outE()` to increase data access locality.
+=== DetachStrategy
+
+`DetachStrategy` manages the fields that will be included in the result. The strategy adds a step to the end of the traversal
+and does not affect the previous steps.
+There are 3 options for detachMode: `ALL` for all properties, `NONE` for no properties, `CUSTOM` for a specific list of properties.
+`id` and `label` always included in response.
+Same for Vertexes and Edges.
+
+WARNING: by default Gremlin Server configured to use `ReferenceElementStrategy` which removes all properties.
Review Comment:
1. What happens if `ReferenceElementStrategy` and `DetachStrategy` are both used at the same time?
2. I wonder what should happen to `ReferenceElementStrategy` in 3.7.x...is it even relevant anymore as there is `DetachStrategy` with mode `NONE` which is the same thing basically? I guess one returns a `ReferenceElement` and the other a `DetachedElement` with no properties so there is that difference. thoughts?
3. what should the new default for Gremlin Server be?
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1108901855
##########
gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/Tokens.java:
##########
@@ -79,6 +79,11 @@ private Tokens() {}
public static final String ARGS_HOST = "host";
public static final String ARGS_SESSION = "session";
public static final String ARGS_MANAGE_TRANSACTION = "manageTransaction";
+ public static final String ARGS_MATERIALIZE_PROPERTIES = "materializeProperties";
+
+ // todo: discuss: keep here or move to {@code T}?
Review Comment:
i guess we've relegated "properties on elements" to a feature peculiar to remoting (meaning the `materializeProperties` is ignored for embedded use cases). it seems more like `RequestOptions` than a `T` in my book. as a result, i think it should stay bound to `Tokens` and not `T` which is more an element of Gremlin Structure API and Language itself. unfortunately it does mean that you will need to document this subtle difference.
--
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] FlorianHockmann commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "FlorianHockmann (via GitHub)" <gi...@apache.org>.
FlorianHockmann commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1108148751
##########
gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphBinary/Types/EdgeSerializer.cs:
##########
@@ -45,15 +46,17 @@ public EdgeSerializer() : base(DataType.Edge)
{
await writer.WriteAsync(value.Id, stream, cancellationToken).ConfigureAwait(false);
await writer.WriteNonNullableValueAsync(value.Label, stream, cancellationToken).ConfigureAwait(false);
-
+
+ // !!! await writer.WriteValueAsync(value.Label, stream, false, cancellationToken).ConfigureAwait(false);
Review Comment:
?
##########
gremlin-dotnet/src/Gremlin.Net/Structure/Edge.cs:
##########
@@ -52,6 +55,15 @@ public Edge(object? id, Vertex outV, string label, Vertex inV)
/// </summary>
public Vertex OutV { get; }
+ /// <summary>
+ /// Get property by key
+ /// </summary>
+ /// <returns>property or null when not found</returns>
+ public Property? Property(string key)
Review Comment:
Any reason against implementing this in `Element` instead of here and in `Vertex`?
##########
gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/RemoteConnectionFactory.cs:
##########
@@ -49,19 +49,32 @@ public IRemoteConnection CreateRemoteConnection(int connectionPoolSize = 2)
return CreateRemoteConnection("gmodern", connectionPoolSize);
}
- public IRemoteConnection CreateRemoteConnection(string traversalSource, int connectionPoolSize = 2)
+ public IRemoteConnection CreateRemoteConnection(string traversalSource,
+ int connectionPoolSize = 2,
+ IMessageSerializer? messageSerializer = null)
{
var c = new DriverRemoteConnectionImpl(
- new GremlinClient(new GremlinServer(TestHost, TestPort), _messageSerializer,
+ new GremlinClient(new GremlinServer(TestHost, TestPort),
Review Comment:
(nitpick) Why not call the newly added method `CreateClient` here?
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/client-tests.js:
##########
@@ -75,6 +75,68 @@ describe('Client', function () {
});
});
+ it('should handle properties for bytecode request', function () {
+ return client.submit(new Bytecode().addStep('V', [1]))
+ .then(function (result) {
+ assert.ok(result);
+ assert.strictEqual(result.length, 1);
+ const vertex = result.first().object;
+ assert.ok(vertex instanceof graphModule.Vertex);
+ let age, name
+ if (vertex.properties instanceof Array) {
+ age = vertex.properties[1]
+ name = vertex.properties[0]
+ } else {
+ age = vertex.properties.age[0]
+ name = vertex.properties.name[0]
+ }
+ assert.strictEqual(age.value, 29);
+ assert.strictEqual(name.value, 'marko');
+ });
+ });
+
+ it('should skip properties for bytecode request', function () {
Review Comment:
(nitpick)
```suggestion
it('should skip properties for bytecode request with tokens', function () {
```
##########
gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Process/Traversal/DriverRemoteConnection/RemoteConnectionFactory.cs:
##########
@@ -49,19 +49,32 @@ public IRemoteConnection CreateRemoteConnection(int connectionPoolSize = 2)
return CreateRemoteConnection("gmodern", connectionPoolSize);
}
- public IRemoteConnection CreateRemoteConnection(string traversalSource, int connectionPoolSize = 2)
+ public IRemoteConnection CreateRemoteConnection(string traversalSource,
+ int connectionPoolSize = 2,
+ IMessageSerializer? messageSerializer = null)
{
var c = new DriverRemoteConnectionImpl(
- new GremlinClient(new GremlinServer(TestHost, TestPort), _messageSerializer,
+ new GremlinClient(new GremlinServer(TestHost, TestPort),
+ messageSerializer ?? _messageSerializer,
connectionPoolSettings: new ConnectionPoolSettings { PoolSize = connectionPoolSize }),
traversalSource);
- _connections.Add(c);
+ _cleanUp.Add(c);
+ return c;
+ }
+
+ public IGremlinClient CreateClient(IMessageSerializer messageSerializer = null)
Review Comment:
Please add a nullable annotation:
```suggestion
public IGremlinClient CreateClient(IMessageSerializer? messageSerializer = null)
```
##########
gremlin-dotnet/test/Gremlin.Net.IntegrationTest/Driver/PropertyDeserializationTests.cs:
##########
@@ -0,0 +1,247 @@
+#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 Gremlin.Net.Driver;
+using Gremlin.Net.IntegrationTest.Process.Traversal.DriverRemoteConnection;
+using Gremlin.Net.Process.Traversal;
+using Gremlin.Net.Structure;
+using Gremlin.Net.Structure.IO.GraphBinary;
+using Gremlin.Net.Structure.IO.GraphSON;
+using System.Collections.Generic;
+using System.Linq;
+using System.Threading.Tasks;
+using Xunit;
+
+namespace Gremlin.Net.IntegrationTest.Driver
+{
+ public class PropertyDeserializationTests
+ {
+ private readonly RemoteConnectionFactory _connectionFactory = new();
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public void ShouldDeserializeVertexPropertiesForBytecode(IMessageSerializer serializer)
+ {
+ var connection = _connectionFactory.CreateRemoteConnection("gmodern", 2, serializer);
+ var g = AnonymousTraversalSource.Traversal().WithRemote(connection);
+
+ var vertex = g.V(1).Next();
+
+ VerifyVertexProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public void ShouldRespectMaterializePropertiesTokensForBytecode(IMessageSerializer serializer)
+ {
+ var connection = _connectionFactory.CreateRemoteConnection("gmodern", 2, serializer);
+ var g = AnonymousTraversalSource.Traversal().WithRemote(connection);
+
+ var vertex = g.With(Tokens.ArgMaterializeProperties, "tokens").V(1).Next();
+
+ VerifyEmptyProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public void ShouldRespectMaterializePropertiesAllForBytecode(IMessageSerializer serializer)
+ {
+ var connection = _connectionFactory.CreateRemoteConnection("gmodern", 2, serializer);
+ var g = AnonymousTraversalSource.Traversal().WithRemote(connection);
+
+ var vertex = g.With(Tokens.ArgMaterializeProperties, "all").V(1).Next();
+
+ VerifyVertexProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public void ShouldHandleEmptyVertexPropertiesForBytecode(IMessageSerializer serializer)
+ {
+ var connection = _connectionFactory.CreateRemoteConnection("gimmutable", 2, serializer);
+ var g = AnonymousTraversalSource.Traversal().WithRemote(connection);
+
+ var vertex = g.AddV("test").Next();
+
+ VerifyEmptyProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public async Task ShouldDeserializeVertexPropertiesForGremlin(IMessageSerializer serializer)
+ {
+ var client = _connectionFactory.CreateClient(serializer);
+
+ var vertex = await client.SubmitWithSingleResultAsync<Vertex>("gmodern.V(1)");
+
+ VerifyVertexProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public async Task ShouldHandleEmptyVertexPropertiesForGremlin(IMessageSerializer serializer)
+ {
+ var client = _connectionFactory.CreateClient(serializer);
+
+ var vertex = await client.SubmitWithSingleResultAsync<Vertex>("gimmutable.addV('test')");
+
+ VerifyEmptyProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public async Task ShouldRespectMaterializePropertiesAllForGremlin(IMessageSerializer serializer)
+ {
+ var client = _connectionFactory.CreateClient(serializer);
+
+ var vertex = await client.SubmitWithSingleResultAsync<Vertex>("gmodern.with('materializeProperties', 'all').V(1)");
+
+ VerifyVertexProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public async Task ShouldRespectMaterializePropertiesTokensForGremlin(IMessageSerializer serializer)
+ {
+ var client = _connectionFactory.CreateClient(serializer);
+
+ var vertex = await client.SubmitWithSingleResultAsync<Vertex>("gmodern.with('materializeProperties', 'tokens').V(1)");
+
+ VerifyEmptyProperties(vertex);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public void ShouldDeserializeEdgePropertiesForBytecode(IMessageSerializer serializer)
+ {
+ var connection = _connectionFactory.CreateRemoteConnection("gmodern", 2, serializer);
+ var g = AnonymousTraversalSource.Traversal().WithRemote(connection);
+
+ var edge = g.E(7).Next();
+
+ VerifyEdgeProperties(edge);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public void ShouldHandleEmptyEdgePropertiesForBytecode(IMessageSerializer serializer)
+ {
+ var connection = _connectionFactory.CreateRemoteConnection("gimmutable", 2, serializer);
+ var g = AnonymousTraversalSource.Traversal().WithRemote(connection);
+
+ var v1 = g.AddV("v1").Next();
+ var v2 = g.AddV("v2").Next();
+ var edge = g.AddE("test").From(v1).To(v2).Next();
+
+ VerifyEmptyProperties(edge);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public async Task ShouldDeserializeEdgePropertiesForGremlin(IMessageSerializer serializer)
+ {
+ var client = _connectionFactory.CreateClient(serializer);
+
+ var edge = await client.SubmitWithSingleResultAsync<Edge>("gmodern.E(7)");
+
+ VerifyEdgeProperties(edge);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public async Task ShouldHandleEmptyEdgePropertiesForGremlin(IMessageSerializer serializer)
+ {
+ var client = _connectionFactory.CreateClient(serializer);
+
+ var edge = await client.SubmitWithSingleResultAsync<Edge>(
+ "gimmutable.addV().as('v1').addV().as('v2').addE('test').from('v1').to('v2')");
+
+ VerifyEmptyProperties(edge);
+ }
+
+ [Theory]
+ [MemberData(nameof(Serializers))]
+ public void ShouldHandleMultiplePropertiesWithSameNameForVertex(IMessageSerializer serializer)
+ {
+ var connection = _connectionFactory.CreateRemoteConnection("gimmutable", 2, serializer);
+ var g = AnonymousTraversalSource.Traversal().WithRemote(connection);
+
+ var vertex = g.AddV()
+ .Property(Cardinality.List, "test", "value1")
+ .Property(Cardinality.List, "test", "value2")
+ .Property(Cardinality.List, "test", "value3")
+ .Next()!;
+
+ vertex = g.V(vertex.Id).Next();
+
+ Assert.NotNull(vertex);
+
+ var properties = vertex.Properties!;
+ Assert.Equal(3, properties.Length);
+ var propertyValues = properties.Cast<VertexProperty>().Where(p => p.Key == "test").Select(p => p.Value).ToArray();
+ Assert.Equal(3, propertyValues.Length);
+ Assert.Equal(new[] { "value1", "value2", "value3" }, propertyValues);
+ }
+
+ private static void VerifyVertexProperties(Vertex? vertex)
+ {
+ Assert.NotNull(vertex);
+ Assert.Equal(1, vertex.Id);
+
+ Assert.Equal(2, vertex.Properties!.Length);
+ var age = vertex.Property("age");
+ Assert.NotNull(age);
+ Assert.Equal(29, age.Value);
+ }
+
+ private static void VerifyEdgeProperties(Edge? edge)
+ {
+ Assert.NotNull(edge);
+ Assert.Equal(7, edge.Id);
+
+ Assert.Single(edge.Properties!);
+ var weight = edge.Property("weight");
+ Assert.NotNull(weight);
+ Assert.Equal(0.5, weight.Value);
+ }
+
+ private static void VerifyEmptyProperties(Vertex? vertex)
Review Comment:
Not that important but you could easily let this take an `Element` instead and then remove the method below.
##########
gremlin-go/driver/graphBinary.go:
##########
@@ -1052,8 +1052,8 @@ func vertexReader(data *[]byte, i *int) (interface{}, error) {
return vertexReaderNullByte(data, i, true)
}
-// {fully qualified id}{unqualified label}{[unused null byte]}
-func vertexReaderNullByte(data *[]byte, i *int, unusedByte bool) (interface{}, error) {
+// {fully qualified id}{unqualified label}{fully qualified properties}
+func vertexReaderNullByte(data *[]byte, i *int, readProperties bool) (interface{}, error) {
Review Comment:
Shouldn't this now be named `vertexReaderReadingProperties` or something like that?
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/test/integration/client-tests.js:
##########
@@ -75,6 +75,68 @@ describe('Client', function () {
});
});
+ it('should handle properties for bytecode request', function () {
+ return client.submit(new Bytecode().addStep('V', [1]))
+ .then(function (result) {
+ assert.ok(result);
+ assert.strictEqual(result.length, 1);
+ const vertex = result.first().object;
+ assert.ok(vertex instanceof graphModule.Vertex);
+ let age, name
+ if (vertex.properties instanceof Array) {
+ age = vertex.properties[1]
+ name = vertex.properties[0]
+ } else {
+ age = vertex.properties.age[0]
+ name = vertex.properties.name[0]
+ }
+ assert.strictEqual(age.value, 29);
+ assert.strictEqual(name.value, 'marko');
+ });
+ });
+
+ it('should skip properties for bytecode request', function () {
+ return client.submit(new Bytecode().addStep('V', [1]), null, {'materializeProperties': 'tokens'})
+ .then(function (result) {
+ assert.ok(result);
+ assert.strictEqual(result.length, 1);
+ const vertex = result.first().object;
+ assert.ok(vertex instanceof graphModule.Vertex);
+ assert.ok(vertex.properties === undefined || vertex.properties.length === 0);
+ });
+ });
+
+ it('should handle properties for gremlin request', function () {
+ return client.submit('g.V(1)')
+ .then(function (result) {
+ assert.ok(result);
+ assert.strictEqual(result.length, 1);
+ const vertex = result.first();
+ assert.ok(vertex instanceof graphModule.Vertex);
+ let age, name
+ if (vertex.properties instanceof Array) {
+ age = vertex.properties[1]
+ name = vertex.properties[0]
+ } else {
+ age = vertex.properties.age[0]
+ name = vertex.properties.name[0]
+ }
+ assert.strictEqual(age.value, 29);
+ assert.strictEqual(name.value, 'marko');
+ });
+ });
+
+ it('should skip properties for gremlin request', function () {
Review Comment:
```suggestion
it('should skip properties for gremlin request with tokens', function () {
```
##########
gremlin-python/src/main/python/tests/structure/io/test_functionalityio.py:
##########
@@ -25,6 +25,34 @@
from gremlin_python.statics import *
+def test_vertex(remote_connection):
+ g = Graph().traversal().withRemote(remote_connection)
+ vertex = g.V(1).next()
+ assert vertex.id == 1
+ assert len(vertex.properties) == 2
+ assert vertex.properties[0].key == 'name'
+ assert vertex.properties[0].value == 'marko'
+ assert vertex.properties[1].key == 'age'
+ assert vertex.properties[1].value == 29
+
+
+def test_vertex_without_properties(remote_connection):
+ g = Graph().traversal().withRemote(remote_connection)
+ vertex = g.with_('materializeProperties', 'tokens').V(1).next()
+ assert vertex.id == 1
+ # empty array for GraphBinary and missing field for GraphSON
+ assert vertex.properties is None or len(vertex.properties) == 0
+
+
+def test_edge(remote_connection):
Review Comment:
(nitpick) You could also add a test for an edge with `tokens`.
##########
gremlin-server/src/test/scripts/generate-all.groovy:
##########
@@ -61,7 +61,7 @@ globals << [hook : [
// add default TraversalSource instances for each graph instance
globals << [gclassic : traversal().withEmbedded(classic).withStrategies(ReferenceElementStrategy)]
-globals << [gmodern : traversal().withEmbedded(modern).withStrategies(ReferenceElementStrategy)]
+globals << [gmodern : traversal().withEmbedded(modern)]
Review Comment:
Just for my own understanding: You're removing this here so `gmodern` can be used by tests where we want to get properties back, right?
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js:
##########
@@ -131,17 +131,30 @@ Given(/^using the parameter (.+) defined as "(.+)"$/, function (paramName, strin
});
});
+var removeProperties = function(p) {
+ if (p === undefined) {
+ } else if (p instanceof graphModule.Vertex || p instanceof graphModule.Edge) {
+ p.properties = undefined;
+ } else if (p instanceof Array) {
+ p.forEach(removeProperties)
+ } else if (p instanceof Map) {
+ removeProperties(Array.from(p.keys()))
+ removeProperties(Array.from(p.values()))
+ } else if (p instanceof graphModule.Path) {
+ removeProperties(p.objects)
+ }
+
+ return p
+}
+
When('iterated to list', function () {
- return this.traversal.toList().then(list => this.result = list).catch(err => this.result = err);
+ return this.traversal.toList().then(list => this.result = removeProperties(list)).catch(err => this.result = err);
Review Comment:
Shouldn't we make sure that things like element equality is handled the same way across GLVs? I think JS should also treat elements as equal if their IDs are equal to stay consistent with Java & .NET.
But that's probably out of scope for this PR. We can also create an issue for this if we agree that all GLVs should behave the same in this regard.
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1110225081
##########
gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphBinary/Types/EdgeSerializer.cs:
##########
@@ -45,15 +46,17 @@ public EdgeSerializer() : base(DataType.Edge)
{
await writer.WriteAsync(value.Id, stream, cancellationToken).ConfigureAwait(false);
await writer.WriteNonNullableValueAsync(value.Label, stream, cancellationToken).ConfigureAwait(false);
-
+
+ // !!! await writer.WriteValueAsync(value.Label, stream, false, cancellationToken).ConfigureAwait(false);
Review Comment:
I forgot to remove this like
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1110222260
##########
gremlin-dotnet/src/Gremlin.Net/Structure/Edge.cs:
##########
@@ -52,6 +55,15 @@ public Edge(object? id, Vertex outV, string label, Vertex inV)
/// </summary>
public Vertex OutV { get; }
+ /// <summary>
+ /// Get property by key
+ /// </summary>
+ /// <returns>property or null when not found</returns>
+ public Property? Property(string key)
Review Comment:
I considered 3 options:
1. minimal change, just extend `Vertex` and `Edge` classes with new typed `Property` method like `public Property? Property(string key)`
2. non-breaking change, add dynamic `Property` method to the base `Element` class like `public dynamic? Property(string key)`, it's less convenient for users
3. breaking change, make `Element' type generic `Element<PropertyType>` and method like `public T? Property(string key)`
first one is safest and at the same time user-friendly, so I chose it
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#issuecomment-1456721116
it seems like existing tests are running and passing, but none of the existing tests validate properties coming back. so, what tests ensure that `materializeProperties` is behaving properly?
--
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 #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1017863078
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/DetachStrategy.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization;
+
+import org.apache.commons.configuration2.Configuration;
+import org.apache.commons.configuration2.MapConfiguration;
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.ScalarMapStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.ProfileSideEffectStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
+import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedFactory;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A strategy that manages the properties that will be in the result.
+ *
+ * @author Valentyn Kahamlyk
+ */
+public final class DetachStrategy extends AbstractTraversalStrategy<TraversalStrategy.FinalizationStrategy> implements TraversalStrategy.FinalizationStrategy {
+
+ private static final DetachStrategy INSTANCE = new DetachStrategy(DetachMode.ALL, null);
+ private DetachMode detachMode = DetachMode.ALL;
+ private List<String> properties;
+
+ private DetachStrategy() {}
+
+ private DetachStrategy(final DetachMode detachMode, final List<String> properties) {
+ this.detachMode = detachMode;
+ this.properties = properties;
+ }
+
+ private DetachStrategy(final Builder builder) {
+ this(builder.detachMode, builder.properties);
+ }
+
+ @Override
+ public void apply(final Traversal.Admin<?, ?> traversal) {
+ if (traversal.getParent() == EmptyStep.instance()) {
+ final Optional<ProfileSideEffectStep> profileStep = TraversalHelper.getFirstStepOfAssignableClass(ProfileSideEffectStep.class, traversal);
+ final int index = profileStep.map(step -> traversal.getSteps().indexOf(step))
+ .orElseGet(() -> traversal.getSteps().size());
+ traversal.addStep(index,
+ new DetachElementStep<>(traversal, new DetachOptions(detachMode, properties)));
+ }
+ }
+
+ public static DetachStrategy create(final Configuration configuration) {
+ return new DetachStrategy(DetachMode.valueOf(configuration.getString(ID_MODE)),
+ new ArrayList<>((Collection<String>) configuration.getProperty(ID_PROPERTIES)));
+ }
+
+ /**
+ * Gets the standard configuration of this strategy that will return all properties.
+ */
+ public static DetachStrategy instance() {
+ return INSTANCE;
+ }
+
+ public static final String ID_MODE = "detachMode";
+ public static final String ID_PROPERTIES = "properties";
+
+ @Override
+ public Configuration getConfiguration() {
+ final Map<String, Object> map = new HashMap<>();
+ map.put(STRATEGY, DetachStrategy.class.getCanonicalName());
+ map.put(ID_MODE, this.detachMode.toString());
+ map.put(ID_PROPERTIES, this.properties);
+ return new MapConfiguration(map);
+ }
+
+ public enum DetachMode {
Review Comment:
we do have a precedent for using integer constants for `with()`-step: https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/util/WithOptions.java
maybe we should follow that pattern here and save the serialization expansion. maybe the rule is to save enum for core language elements, like `Scope`, `Direction`, etc.
--
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] FlorianHockmann commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "FlorianHockmann (via GitHub)" <gi...@apache.org>.
FlorianHockmann commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1111614272
##########
gremlin-dotnet/src/Gremlin.Net/Structure/Edge.cs:
##########
@@ -52,6 +55,15 @@ public Edge(object? id, Vertex outV, string label, Vertex inV)
/// </summary>
public Vertex OutV { get; }
+ /// <summary>
+ /// Get property by key
+ /// </summary>
+ /// <returns>property or null when not found</returns>
+ public Property? Property(string key)
Review Comment:
Sorry, I didn't look closely enough and completely forgot that a `Vertex` has a `VertexProperty` which isn't the same as the `Property` of an `Edge`. Then I agree with your approach 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] spmallette commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1112971728
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js:
##########
@@ -131,17 +131,30 @@ Given(/^using the parameter (.+) defined as "(.+)"$/, function (paramName, strin
});
});
+var removeProperties = function(p) {
+ if (p === undefined) {
+ } else if (p instanceof graphModule.Vertex || p instanceof graphModule.Edge) {
+ p.properties = undefined;
+ } else if (p instanceof Array) {
+ p.forEach(removeProperties)
+ } else if (p instanceof Map) {
+ removeProperties(Array.from(p.keys()))
+ removeProperties(Array.from(p.values()))
+ } else if (p instanceof graphModule.Path) {
+ removeProperties(p.objects)
+ }
+
+ return p
+}
+
When('iterated to list', function () {
- return this.traversal.toList().then(list => this.result = list).catch(err => this.result = err);
+ return this.traversal.toList().then(list => this.result = removeProperties(list)).catch(err => this.result = err);
Review Comment:
ugh - that's really bad. i agree with florian that needs to be fixed. if no other GLVs need this special handling then i'd say create a JIRA to track explicitly since this is a behavioral change and just include the fix here in this PR so we don't need this `removeProperties()` workaround.
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1112966853
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -34,15 +34,16 @@ public class GremlinScriptChecker {
/**
* An empty result whose properties return as empty.
*/
- public static final Result EMPTY_RESULT = new Result(null, null);
+ public static final Result EMPTY_RESULT = new Result(null, null, null);
/**
* At least one of these tokens should be present somewhere in the Gremlin string for {@link #parse(String)} to
* take any action at all.
*/
+ // todo: why not used `Tokens`?
Review Comment:
I suppose you could do this:
```java
private static final Set<String> tokens = new HashSet<>(Arrays.asList(Tokens.ARGS_EVAL_TIMEOUT, Tokens.ARGS_SCRIPT_EVAL_TIMEOUT,
"ARGS_EVAL_TIMEOUT", "ARGS_SCRIPT_EVAL_TIMEOUT",
Tokens.REQUEST_ID, Tokens.ARGS_MATERIALIZE_PROPERTIES
"REQUEST_ID", "ARGS_MATERIALIZE_PROPERTIES"));
```
But then the other problem is that the script checker is in `gremlin-core` and `Tokens` is in `gremlin-driver`. It's a little messed up. I suppose the right way to resolve it would be to officially make these settings part of `gremlin-language` but that seems out of scope for your work. You could create something in JIRA for that if you want to come back to it.
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1132733325
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerSerializationIntegrateTest.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.server;
+
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.apache.tinkerpop.gremlin.util.ser.AbstractMessageSerializer;
+import org.apache.tinkerpop.gremlin.util.ser.GraphBinaryMessageSerializerV1;
+import org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV2d0;
+import org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV3d0;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+
+import static org.junit.Assert.assertEquals;
+
+@RunWith(Parameterized.class)
+public class GremlinServerSerializationIntegrateTest extends AbstractGremlinServerIntegrationTest {
+
+ private AbstractMessageSerializer serializer;
+ private Cluster cluster = null;
+ private Client client = null;
+ private GraphTraversalSource g = null;
+
+ public GremlinServerSerializationIntegrateTest(AbstractMessageSerializer serializer) {
+ this.serializer = serializer;
+ }
+
+ @Parameterized.Parameters
+ public static Collection serializers() {
+ return Arrays.asList(new Object[][] {
+ { new GraphBinaryMessageSerializerV1() },
+ { new GraphSONMessageSerializerV3d0() },
+ { new GraphSONMessageSerializerV2d0() }
+ });
+ }
+
+ @Before
+ public void openConnection() {
+ cluster = TestClientFactory.build().serializer(serializer).create();
+ client = cluster.connect();
+ g = AnonymousTraversalSource.traversal().withRemote(
+ DriverRemoteConnection.using(cluster, "gmodern"));
+ }
+
+ @After
+ public void closeConnection() {
+ if (cluster != null) cluster.close();
+ }
+
+ @Test
+ public void shouldDeserializeVertexPropertiesForGremlin() {
+ final Vertex vertex = client.submit("gmodern.V(1)").one().getVertex();
+
+ assertVertexWithProperties(vertex);
+ }
+
+ @Test
+ public void shouldSkipVertexPropertiesForGremlin() {
+ final Vertex vertex = client.submit("gmodern.with('materializeProperties', 'tokens').V(1)").one().getVertex();
+
+ assertEquals(Collections.emptyIterator(), vertex.properties());
Review Comment:
added in https://github.com/apache/tinkerpop/pull/1843/commits/6884272f19b8ed97a9de3f53033e651b3fa437bf
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1126854805
##########
CHANGELOG.asciidoc:
##########
@@ -38,6 +38,9 @@ This release also includes changes from <<release-3-6-XXX, 3.6.XXX>>.
* Reduces dependency from `gremlin-server` onto `gremlin-driver` to a test scope only.
* Added `RequestOptions` and `RequestOptionsBuilder` types to Go GLV to encapsulate per-request settings and bindings.
* Added `SubmitWithOptions()` methods to `Client` and `DriverRemoteConnection` in Go GLV to pass `RequestOptions` to the server.
+* Changed default behavior for returning properties on Elements for OLTP queries. Properties now returned.
+* Detachment is no longer performed in `TraverserIterator`.
+* Added `materializeProperties` request option to control properties serialization.
Review Comment:
I'd include:
* Modified serializers in <languages> to handle serialization and deserialization of properties.
* Added functional properties to the graph structure components for <languages or just "non-JVM languages" if you did them all but Java>.
* Modified the `GremlinScriptChecker` to extract the `materializeProperties` request option.
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1016933749
##########
docs/src/reference/the-traversal.asciidoc:
##########
@@ -4916,6 +4916,28 @@ g.V().hasLabel('person'). <1>
<8> `PathRetractionStrategy` will remove paths from the traversers and increase the likelihood of bulking as path data is not required after `select('b')`.
<9> `AdjacentToIncidentStrategy` will turn `out()` into `outE()` to increase data access locality.
+=== DetachStrategy
+
+`DetachStrategy` manages the fields that will be included in the result. The strategy adds a step to the end of the traversal
+and does not affect the previous steps.
+There are 3 options for detachMode: `ALL` for all properties, `NONE` for no properties, `CUSTOM` for a specific list of properties.
+`id` and `label` always included in response.
+Same for Vertexes and Edges.
+
+WARNING: by default Gremlin Server configured to use `ReferenceElementStrategy` which removes all properties.
Review Comment:
1. The result will be Vertex without properties. I don't think it makes sense to use them at the same time.
2. I think `ReferenceElementStrategy` may be deprecated in 3.7.
There are some small differences between ReferenceElement and DetachedElement that need to be aligned before. For example https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/reference/ReferenceElement.java#L50 and https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/detached/DetachedElement.java#L49
3. I consider default may be `DetachStrategy` configured to skip properties. I think users who want to get properties can remove the strategy from server startup or use it from the client when needed. Most current users don't expect to get properties and it doesn't make sense to load the server and network.
Another option is to remove `ReferenceElementStrategy` and allow users to choose the settings themselves, with the recommendation to skip properties 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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1113628989
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -162,7 +168,10 @@ public class GremlinScriptChecker {
* </pre>
*/
private static final Pattern patternWithOptions =
- Pattern.compile("\\.with\\(((?:" + timeoutTokens + "),(?<to>\\d*)(:?L|l)?)|((?:" + requestIdTokens + "),[\"'](?<rid>.*?))[\"']\\)");
+ Pattern.compile("\\.with\\((((?:"
Review Comment:
"explain" updated in https://github.com/apache/tinkerpop/pull/1843/commits/b8582c750db1982a33ba41f4a9fda976ecc31eaa
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1108905595
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -94,6 +95,11 @@ public class GremlinScriptChecker {
*/
private static final String requestIdTokens = "[\"']requestId[\"']|(?:Tokens\\.)?REQUEST_ID";
+ /**
+ * Regex fragment for the materializeProperties to look for.
+ */
+ private static final String materializePropertiesTokens = "[\"']materializeProperties[\"']";
Review Comment:
and here include the `Tokens.` form
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1122102698
##########
docs/src/upgrade/release-3.7.x.asciidoc:
##########
@@ -29,6 +29,28 @@ Please see the link:https://github.com/apache/tinkerpop/blob/3.7.0/CHANGELOG.asc
=== Upgrading for Users
+==== Properties on Elements
Review Comment:
i can't recall, but are there any compatibility issues to call attention to here? like can a 3.6.x driver work nicely with 3.7.x or the reverse? maybe there is something to call attention to here with that.
--
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] vkagamlyk closed pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk closed pull request #1843: TINKERPOP-2824 Properties on response elements
URL: https://github.com/apache/tinkerpop/pull/1843
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1131927216
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/types/VertexPropertySerializer.java:
##########
@@ -66,7 +66,10 @@ protected void writeValue(final VertexProperty value, final Buffer buffer, final
// we don't serialize the parent vertex, let's hold a place for it
context.write(null, buffer);
- final List<?> asList = IteratorUtils.toList(value.properties());
- context.write(asList, buffer);
+ // Neo4jVertexProperty throw UnsupportedOperationException for properties,
+ // so for now VertexProperty will be serialized without inner properties
+ // final List<?> asList = IteratorUtils.toList(value.properties());
+ // context.write(asList, buffer);
+ context.write(null, buffer);
Review Comment:
`VertexProperty.Exception.metaPropertiesNotSupported` used when `VertexProperty` doesn't support getting and setting meta-properties. I changed getter behavior to return empty Iterable, but kept Exception for setter.
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1126849283
##########
gremlin-server/src/test/scripts/generate-all.groovy:
##########
@@ -61,7 +61,7 @@ globals << [hook : [
// add default TraversalSource instances for each graph instance
globals << [gclassic : traversal().withEmbedded(classic).withStrategies(ReferenceElementStrategy)]
Review Comment:
any reason you didn't remove `ReferenceElementStrategy` for all the graphs?
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1131240801
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/types/VertexPropertySerializer.java:
##########
@@ -66,7 +66,10 @@ protected void writeValue(final VertexProperty value, final Buffer buffer, final
// we don't serialize the parent vertex, let's hold a place for it
context.write(null, buffer);
- final List<?> asList = IteratorUtils.toList(value.properties());
- context.write(asList, buffer);
+ // Neo4jVertexProperty throw UnsupportedOperationException for properties,
+ // so for now VertexProperty will be serialized without inner properties
+ // final List<?> asList = IteratorUtils.toList(value.properties());
+ // context.write(asList, buffer);
+ context.write(null, buffer);
Review Comment:
GraphSON uses workaround by getting properties starting from `keys`. It only works when `Neo4jVertexProperty.vertexPropertyNode` is null or empty.
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1109001720
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -34,15 +34,16 @@ public class GremlinScriptChecker {
/**
* An empty result whose properties return as empty.
*/
- public static final Result EMPTY_RESULT = new Result(null, null);
+ public static final Result EMPTY_RESULT = new Result(null, null, null);
/**
* At least one of these tokens should be present somewhere in the Gremlin string for {@link #parse(String)} to
* take any action at all.
*/
+ // todo: why not used `Tokens`?
Review Comment:
There's a bit of a mess with names for this constants.
For example we have constant for [evaluationTimeout](https://github.com/apache/tinkerpop/blob/master/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/Tokens.java#L78) and can use `Token.ARGS_EVAL_TIMEOUT`, not plain string. Same for `requestId`, but not for `scriptEvaluationTimeout`. Also `Tokens.ARGS_SCRIPT_EVAL_TIMEOUT` is not defined and I don't understand why.
I think better to use constants when possible and this like can looks like
```
private static final Set<String> tokens = new HashSet<>(Arrays.asList(Tokens.ARGS_EVAL_TIMEOUT, Tokens.ARGS_SCRIPT_EVAL_TIMEOUT,
Tokens.REQUEST_ID, Tokens.ARGS_MATERIALIZE_PROPERTIES));
```
but made in the same style as before.
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1130940538
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerSerializationIntegrateTest.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.server;
+
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.apache.tinkerpop.gremlin.util.ser.AbstractMessageSerializer;
+import org.apache.tinkerpop.gremlin.util.ser.GraphBinaryMessageSerializerV1;
+import org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV2d0;
+import org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV3d0;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+
+import static org.junit.Assert.assertEquals;
+
+@RunWith(Parameterized.class)
+public class GremlinServerSerializationIntegrateTest extends AbstractGremlinServerIntegrationTest {
+
+ private AbstractMessageSerializer serializer;
+ private Cluster cluster = null;
+ private Client client = null;
+ private GraphTraversalSource g = null;
+
+ public GremlinServerSerializationIntegrateTest(AbstractMessageSerializer serializer) {
+ this.serializer = serializer;
+ }
+
+ @Parameterized.Parameters
+ public static Collection serializers() {
+ return Arrays.asList(new Object[][] {
+ { new GraphBinaryMessageSerializerV1() },
+ { new GraphSONMessageSerializerV3d0() },
+ { new GraphSONMessageSerializerV2d0() }
+ });
+ }
+
+ @Before
+ public void openConnection() {
+ cluster = TestClientFactory.build().serializer(serializer).create();
+ client = cluster.connect();
+ g = AnonymousTraversalSource.traversal().withRemote(
+ DriverRemoteConnection.using(cluster, "gmodern"));
+ }
+
+ @After
+ public void closeConnection() {
+ if (cluster != null) cluster.close();
+ }
+
+ @Test
+ public void shouldDeserializeVertexPropertiesForGremlin() {
+ final Vertex vertex = client.submit("gmodern.V(1)").one().getVertex();
+
+ assertVertexWithProperties(vertex);
+ }
+
+ @Test
+ public void shouldSkipVertexPropertiesForGremlin() {
+ final Vertex vertex = client.submit("gmodern.with('materializeProperties', 'tokens').V(1)").one().getVertex();
+
+ assertEquals(Collections.emptyIterator(), vertex.properties());
Review Comment:
at risk of redundancy, but for sanity, please assert that the `T` values are present in addition to asserting the properties are not (and do the same elsewhere of course)
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#issuecomment-1470242578
could you please rebase to get rid of the conflicts and squash to clean up the commit history (you don't need to do this as one commit i guess, but 80+ tiny commits is a bit much 🙂)? other than that, I think this is generally ready to merge. let's see how it looks after rebase for a final review.
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1117575914
##########
gremlin-python/src/main/python/tests/structure/io/test_functionalityio.py:
##########
@@ -25,6 +25,34 @@
from gremlin_python.statics import *
+def test_vertex(remote_connection):
+ g = Graph().traversal().withRemote(remote_connection)
+ vertex = g.V(1).next()
+ assert vertex.id == 1
+ assert len(vertex.properties) == 2
+ assert vertex.properties[0].key == 'name'
+ assert vertex.properties[0].value == 'marko'
+ assert vertex.properties[1].key == 'age'
+ assert vertex.properties[1].value == 29
+
+
+def test_vertex_without_properties(remote_connection):
+ g = Graph().traversal().withRemote(remote_connection)
+ vertex = g.with_('materializeProperties', 'tokens').V(1).next()
+ assert vertex.id == 1
+ # empty array for GraphBinary and missing field for GraphSON
+ assert vertex.properties is None or len(vertex.properties) == 0
+
+
+def test_edge(remote_connection):
Review Comment:
added in https://github.com/apache/tinkerpop/pull/1843/commits/9e9520b1c4aefa027445a351af19900cdd0bdeb9
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1116070137
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -94,6 +95,11 @@ public class GremlinScriptChecker {
*/
private static final String requestIdTokens = "[\"']requestId[\"']|(?:Tokens\\.)?REQUEST_ID";
+ /**
+ * Regex fragment for the materializeProperties to look for.
+ */
+ private static final String materializePropertiesTokens = "[\"']materializeProperties[\"']";
Review Comment:
added in https://github.com/apache/tinkerpop/pull/1843/commits/8c36b01b673982875d9d9e04d91f3ddce73860f5
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1114680661
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js:
##########
@@ -131,17 +131,30 @@ Given(/^using the parameter (.+) defined as "(.+)"$/, function (paramName, strin
});
});
+var removeProperties = function(p) {
+ if (p === undefined) {
+ } else if (p instanceof graphModule.Vertex || p instanceof graphModule.Edge) {
+ p.properties = undefined;
+ } else if (p instanceof Array) {
+ p.forEach(removeProperties)
+ } else if (p instanceof Map) {
+ removeProperties(Array.from(p.keys()))
+ removeProperties(Array.from(p.values()))
+ } else if (p instanceof graphModule.Path) {
+ removeProperties(p.objects)
+ }
+
+ return p
+}
+
When('iterated to list', function () {
- return this.traversal.toList().then(list => this.result = list).catch(err => this.result = err);
+ return this.traversal.toList().then(list => this.result = removeProperties(list)).catch(err => this.result = err);
Review Comment:
This issue is related to the internal representation of the data in JS client.
When data comes in GraphSON, when properties is stored as Object where key is property name and value are array of properties, like described in GraphSON specification. To get property used code `age = vertex.properties.age[0]`.
For GraphBinary used simple array `property[]`. To get same property 'age = vertex.properties[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 a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1114446500
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js:
##########
@@ -131,17 +131,30 @@ Given(/^using the parameter (.+) defined as "(.+)"$/, function (paramName, strin
});
});
+var removeProperties = function(p) {
+ if (p === undefined) {
+ } else if (p instanceof graphModule.Vertex || p instanceof graphModule.Edge) {
+ p.properties = undefined;
+ } else if (p instanceof Array) {
+ p.forEach(removeProperties)
+ } else if (p instanceof Map) {
+ removeProperties(Array.from(p.keys()))
+ removeProperties(Array.from(p.values()))
+ } else if (p instanceof graphModule.Path) {
+ removeProperties(p.objects)
+ }
+
+ return p
+}
+
When('iterated to list', function () {
- return this.traversal.toList().then(list => this.result = list).catch(err => this.result = err);
+ return this.traversal.toList().then(list => this.result = removeProperties(list)).catch(err => this.result = err);
Review Comment:
as long as javascript matches the defined format for GraphSON i don't think we need to change anything: https://tinkerpop.apache.org/docs/current/dev/io/#_vertex_3
--
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 #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1016715423
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/DetachStrategyProcessTest.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization;
+
+import org.apache.tinkerpop.gremlin.LoadGraphWith;
+import org.apache.tinkerpop.gremlin.process.AbstractGremlinProcessTest;
+import org.apache.tinkerpop.gremlin.process.IgnoreEngine;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalEngine;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.junit.Test;
+
+import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.MODERN;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+
+/**
+ * @author Valentyn Kahamlyk
+ */
+public class DetachStrategyProcessTest extends AbstractGremlinProcessTest {
+
+ @Test
+ @LoadGraphWith(MODERN)
+ @IgnoreEngine(TraversalEngine.Type.COMPUTER)
+ public void shouldReturnOnlyRequestedFields() {
+
+ final Vertex v = g.V(1).next();
+ final Vertex vWithAllProps = g
+ .withStrategies(DetachStrategy.build().detachMode(DetachStrategy.DetachMode.ALL).properties("name").create())
Review Comment:
did you think about not allowing invalid configurations in the builder?
--
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] kenhuuu commented on a diff in pull request #1843: [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
kenhuuu commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1004862664
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/DetachStrategy.java:
##########
@@ -0,0 +1,156 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization;
+
+import org.apache.commons.configuration2.Configuration;
+import org.apache.commons.configuration2.MapConfiguration;
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.ScalarMapStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.ProfileSideEffectStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
+import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedFactory;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A strategy that manages the properties that will be in the result.
+ *
+ * @author Valentyn Kahamlyk
+ */
+public final class DetachStrategy extends AbstractTraversalStrategy<TraversalStrategy.FinalizationStrategy> implements TraversalStrategy.FinalizationStrategy {
+
+ private DetachOptions detachOptions;
+
+ // todo: is public constructor ok or better make private?
+ public DetachStrategy(final DetachMode detachMode, final String[] properties) {
+ detachOptions = DetachOptions.build().detachMode(detachMode).properties(properties).create();
+ }
+
+ @Override
+ public void apply(final Traversal.Admin<?, ?> traversal) {
+ if (traversal.getParent() == EmptyStep.instance()) {
+ final Optional<ProfileSideEffectStep> profileStep = TraversalHelper.getFirstStepOfAssignableClass(ProfileSideEffectStep.class, traversal);
+ final int index = profileStep.map(step -> traversal.getSteps().indexOf(step))
+ .orElseGet(() -> traversal.getSteps().size());
+ traversal.addStep(index, new DetachElementStep<>(traversal, detachOptions));
+ }
+ }
+
+ public static DetachStrategy create(final Configuration configuration) {
+ return new DetachStrategy(DetachMode.valueOf(configuration.getString(ID_MODE)), configuration.getStringArray(ID_PROPERTIES));
+ }
+
+ public static final String ID_MODE = "mode";
Review Comment:
Static variables generally go near the top of the file.
--
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] kenhuuu commented on a diff in pull request #1843: [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
kenhuuu commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1004867278
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/DetachStrategy.java:
##########
@@ -0,0 +1,156 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization;
+
+import org.apache.commons.configuration2.Configuration;
+import org.apache.commons.configuration2.MapConfiguration;
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.ScalarMapStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.ProfileSideEffectStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
+import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedFactory;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A strategy that manages the properties that will be in the result.
+ *
+ * @author Valentyn Kahamlyk
+ */
+public final class DetachStrategy extends AbstractTraversalStrategy<TraversalStrategy.FinalizationStrategy> implements TraversalStrategy.FinalizationStrategy {
+
+ private DetachOptions detachOptions;
+
+ // todo: is public constructor ok or better make private?
+ public DetachStrategy(final DetachMode detachMode, final String[] properties) {
+ detachOptions = DetachOptions.build().detachMode(detachMode).properties(properties).create();
+ }
+
+ @Override
+ public void apply(final Traversal.Admin<?, ?> traversal) {
+ if (traversal.getParent() == EmptyStep.instance()) {
+ final Optional<ProfileSideEffectStep> profileStep = TraversalHelper.getFirstStepOfAssignableClass(ProfileSideEffectStep.class, traversal);
+ final int index = profileStep.map(step -> traversal.getSteps().indexOf(step))
+ .orElseGet(() -> traversal.getSteps().size());
+ traversal.addStep(index, new DetachElementStep<>(traversal, detachOptions));
+ }
+ }
+
+ public static DetachStrategy create(final Configuration configuration) {
+ return new DetachStrategy(DetachMode.valueOf(configuration.getString(ID_MODE)), configuration.getStringArray(ID_PROPERTIES));
+ }
+
+ public static final String ID_MODE = "mode";
+ public static final String ID_PROPERTIES = "properties";
+
+ @Override
+ public Configuration getConfiguration() {
+ final Map<String, Object> map = new HashMap<>();
+ map.put(STRATEGY, DetachStrategy.class.getCanonicalName());
+ map.put(ID_MODE, this.detachOptions.detachMode);
+ map.put(ID_PROPERTIES, this.detachOptions.properties);
+ return new MapConfiguration(map);
+ }
+
+ public enum DetachMode {
+
+ ALL,
+ CUSTOM,
+ NONE;
+ }
+
+ public static class DetachOptions {
+ private DetachMode detachMode;
+ private String[] properties;
+
+ private DetachOptions(final DetachOptions.Builder builder) {
+ this.detachMode = builder.detachMode;
+ this.properties = builder.properties;
+ }
+
+ public static DetachOptions.Builder build() {
+ return new DetachOptions.Builder();
+ }
+
+ public DetachMode getDetachMode() {
+ return this.detachMode;
+ }
+
+ public String[] getProperties() {
+ return this.properties;
+ }
+
+ public static final class Builder {
+
+ private DetachMode detachMode = DetachMode.NONE;
+ private String[] properties;
Review Comment:
No default value may lead to a potential null pointer exception.
--
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] vkagamlyk commented on a diff in pull request #1843: [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1004870555
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/DetachStrategy.java:
##########
@@ -0,0 +1,156 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization;
+
+import org.apache.commons.configuration2.Configuration;
+import org.apache.commons.configuration2.MapConfiguration;
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.ScalarMapStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.ProfileSideEffectStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
+import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedFactory;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A strategy that manages the properties that will be in the result.
+ *
+ * @author Valentyn Kahamlyk
+ */
+public final class DetachStrategy extends AbstractTraversalStrategy<TraversalStrategy.FinalizationStrategy> implements TraversalStrategy.FinalizationStrategy {
+
+ private DetachOptions detachOptions;
+
+ // todo: is public constructor ok or better make private?
+ public DetachStrategy(final DetachMode detachMode, final String[] properties) {
+ detachOptions = DetachOptions.build().detachMode(detachMode).properties(properties).create();
+ }
+
+ @Override
+ public void apply(final Traversal.Admin<?, ?> traversal) {
+ if (traversal.getParent() == EmptyStep.instance()) {
+ final Optional<ProfileSideEffectStep> profileStep = TraversalHelper.getFirstStepOfAssignableClass(ProfileSideEffectStep.class, traversal);
+ final int index = profileStep.map(step -> traversal.getSteps().indexOf(step))
+ .orElseGet(() -> traversal.getSteps().size());
+ traversal.addStep(index, new DetachElementStep<>(traversal, detachOptions));
+ }
+ }
+
+ public static DetachStrategy create(final Configuration configuration) {
+ return new DetachStrategy(DetachMode.valueOf(configuration.getString(ID_MODE)), configuration.getStringArray(ID_PROPERTIES));
+ }
+
+ public static final String ID_MODE = "mode";
Review Comment:
yes, but https://github.com/apache/tinkerpop/blob/master/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/strategy/decoration/VertexProgramStrategy.java#L189
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1108921425
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -228,11 +244,20 @@ public Optional<String> getRequestId() {
return null == requestId ? Optional.empty() : Optional.of(requestId);
}
+ /**
+ * Gets the value of the request identifier supplied using the {@code with()} source step. If there are
Review Comment:
nit: prefer `link` for javadoc when referring elsewhere in the codebase that we can hyperlink to and `code` when referring to the item being documented, external libs or jdk classes especially on public methods like this one.
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1113651772
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -228,11 +244,20 @@ public Optional<String> getRequestId() {
return null == requestId ? Optional.empty() : Optional.of(requestId);
}
+ /**
+ * Gets the value of the request identifier supplied using the {@code with()} source step. If there are
Review Comment:
ha - i code reviewed myself i guess
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1114680661
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js:
##########
@@ -131,17 +131,30 @@ Given(/^using the parameter (.+) defined as "(.+)"$/, function (paramName, strin
});
});
+var removeProperties = function(p) {
+ if (p === undefined) {
+ } else if (p instanceof graphModule.Vertex || p instanceof graphModule.Edge) {
+ p.properties = undefined;
+ } else if (p instanceof Array) {
+ p.forEach(removeProperties)
+ } else if (p instanceof Map) {
+ removeProperties(Array.from(p.keys()))
+ removeProperties(Array.from(p.values()))
+ } else if (p instanceof graphModule.Path) {
+ removeProperties(p.objects)
+ }
+
+ return p
+}
+
When('iterated to list', function () {
- return this.traversal.toList().then(list => this.result = list).catch(err => this.result = err);
+ return this.traversal.toList().then(list => this.result = removeProperties(list)).catch(err => this.result = err);
Review Comment:
This issue is related to the internal representation of the data in JS client.
When data comes in GraphSON, when properties is stored as Object where key is property name and value are array of properties, like described in GraphSON specification. To get property used code `age = vertex.properties.age[0]`.
For GraphBinary used simple array `property[]`. To get same property `age = vertex.properties[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] FlorianHockmann commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "FlorianHockmann (via GitHub)" <gi...@apache.org>.
FlorianHockmann commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1121982613
##########
docs/src/reference/gremlin-applications.asciidoc:
##########
@@ -2250,50 +2250,36 @@ to as "detached elements" and cases where properties are not included are "refer
With the type of request and detachment model in mind, it is now possible to discuss how best to consider element
properties in relation to them all in concert.
-By default, Gremlin Server sample configurations utilize `ReferenceElementStrategy` when creating the out-of-the-box
-`GraphTraversalSource`. As the name suggests, this means that elements will be detached by reference and will
-therefore not have properties included. The relevant configuration from the Gremlin Server initialization script looks
-like this:
+By default, Gremlin Server sample configurations return all properties.
+
+To manage properties for each request you can use <<configuration-steps-with,with()>> configuration option
+`materializeProperties`
[source,groovy]
----
-globals << [g : traversal().withEmbedded(graph).withStrategies(ReferenceElementStrategy)]
+g.with('materializeProperties', 'tokens').V()
----
-This configuration is global to Gremlin Server and therefore all methods of connection will always return elements
-without properties. If this strategy is not included, then there are other considerations to take into account such as
-the connection type (i.e. script or bytecode) and the serializer.
+The `tokens` value for the `materializeProperties` means that need to return only `id` and `label`. Another option
Review Comment:
```suggestion
The `tokens` value for the `materializeProperties` means that only `id` and `label` should be returned. Another option
```
##########
docs/src/reference/gremlin-applications.asciidoc:
##########
@@ -2250,50 +2250,36 @@ to as "detached elements" and cases where properties are not included are "refer
With the type of request and detachment model in mind, it is now possible to discuss how best to consider element
properties in relation to them all in concert.
-By default, Gremlin Server sample configurations utilize `ReferenceElementStrategy` when creating the out-of-the-box
-`GraphTraversalSource`. As the name suggests, this means that elements will be detached by reference and will
-therefore not have properties included. The relevant configuration from the Gremlin Server initialization script looks
-like this:
+By default, Gremlin Server sample configurations return all properties.
+
+To manage properties for each request you can use <<configuration-steps-with,with()>> configuration option
+`materializeProperties`
[source,groovy]
----
-globals << [g : traversal().withEmbedded(graph).withStrategies(ReferenceElementStrategy)]
+g.with('materializeProperties', 'tokens').V()
----
-This configuration is global to Gremlin Server and therefore all methods of connection will always return elements
-without properties. If this strategy is not included, then there are other considerations to take into account such as
-the connection type (i.e. script or bytecode) and the serializer.
+The `tokens` value for the `materializeProperties` means that need to return only `id` and `label`. Another option
+`all` used to indicate that all properties should be returned and is the default value.
Review Comment:
I think this needs to be:
> Another option, `all`, _can be_ used [...]
##########
docs/src/reference/gremlin-applications.asciidoc:
##########
@@ -2307,10 +2293,8 @@ List<Vertex> results = g.V().hasLabel("person").elementMap('name').toList();
Both of the above requests return a list of `Map` instances that contain the `id`, `label` and the "name" property.
-TIP: The example graph configurations pre-packaged with Gremlin Server utilize `ReferenceElementStrategy`
-which convert all graph elements to references by initializing "g" using
-`withStrategies(ReferenceElementStrategy.instance()`. Consider utilizing `ReferenceElementStrategy` whenever creating
-a `GraphTraversalSource` in Java to ensure the most portable Gremlin.
+TIP: Consider utilizing `ReferenceElementStrategy` whenever creating a `GraphTraversalSource` in Java to ensure
+the most portable Gremlin.
NOTE: For those interested, please see link:https://lists.apache.org/thread.html/e959e85d4f8b3d46d281f2742a6e574c7d27c54bfc52f802f7c04af3%40%3Cdev.tinkerpop.apache.org%3E[this post]
Review Comment:
I think we should remove this thread here or at least mention that it discusses the history of this and gives pros/cons, but it doesn't reflect the current situation any more as properties are now returned by default.
##########
docs/src/reference/gremlin-applications.asciidoc:
##########
@@ -2307,10 +2293,8 @@ List<Vertex> results = g.V().hasLabel("person").elementMap('name').toList();
Both of the above requests return a list of `Map` instances that contain the `id`, `label` and the "name" property.
-TIP: The example graph configurations pre-packaged with Gremlin Server utilize `ReferenceElementStrategy`
-which convert all graph elements to references by initializing "g" using
-`withStrategies(ReferenceElementStrategy.instance()`. Consider utilizing `ReferenceElementStrategy` whenever creating
-a `GraphTraversalSource` in Java to ensure the most portable Gremlin.
+TIP: Consider utilizing `ReferenceElementStrategy` whenever creating a `GraphTraversalSource` in Java to ensure
Review Comment:
Is this still a suggestion that we want to make? I'm not sure what is meant here by "portable Gremlin". Should it be easy to port a traversal to another GLV? In that case, we can remove the tip now as GLVs can now also deserialize properties.
Or portable between different graph providers? In that case, it might still be better to use `ReferenceElementStrategy` maybe, but I'm not sure how important this consideration is.
##########
docs/src/upgrade/release-3.7.x.asciidoc:
##########
@@ -29,6 +29,28 @@ Please see the link:https://github.com/apache/tinkerpop/blob/3.7.0/CHANGELOG.asc
=== Upgrading for Users
+==== Properties on Elements
+
+By default properties on Elements are now returned.
+If you don't need to get properties then you can do one of the following ways:
+
+* Configure server to not return properties, update groovy configuration file with `ReferenceElementStrategy`.
Review Comment:
I think _groovy configuration file_ isn't clear enough for users to understand which Groovy file they need to edit. Since it's named _Gremlin Server initialization script_ in the reference docs, I would also use that term here.
`server` should probably also be renamed to `Gremlin Server`. That way, users of graphs that don't include Gremlin Server know that this doesn't apply to them.
##########
docs/src/reference/gremlin-applications.asciidoc:
##########
@@ -2250,50 +2250,36 @@ to as "detached elements" and cases where properties are not included are "refer
With the type of request and detachment model in mind, it is now possible to discuss how best to consider element
properties in relation to them all in concert.
-By default, Gremlin Server sample configurations utilize `ReferenceElementStrategy` when creating the out-of-the-box
-`GraphTraversalSource`. As the name suggests, this means that elements will be detached by reference and will
-therefore not have properties included. The relevant configuration from the Gremlin Server initialization script looks
-like this:
+By default, Gremlin Server sample configurations return all properties.
+
+To manage properties for each request you can use <<configuration-steps-with,with()>> configuration option
+`materializeProperties`
[source,groovy]
----
-globals << [g : traversal().withEmbedded(graph).withStrategies(ReferenceElementStrategy)]
+g.with('materializeProperties', 'tokens').V()
----
-This configuration is global to Gremlin Server and therefore all methods of connection will always return elements
-without properties. If this strategy is not included, then there are other considerations to take into account such as
-the connection type (i.e. script or bytecode) and the serializer.
+The `tokens` value for the `materializeProperties` means that need to return only `id` and `label`. Another option
+`all` used to indicate that all properties should be returned and is the default value.
-For script-based requests, users should take care when returning graph elements. By default, elements will be returned
-as "detached elements" and depending on the serializer being used those detached elements may or may not have their
-properties carried with them. Gryo and GraphSON serializers will write all properties in the return payload in this
-case but GraphBinary will not. Therefore, script-based requests that use Gryo or GraphSON should definitely follow the
-best practice of only returning the data required by the application.
+In some cases it can be inconvenient to load vertices with properties due to fat data or for compatibility reasons.
+That can be solved by utilize `ReferenceElementStrategy` when creating the out-of-the-box `GraphTraversalSource`.
+As the name suggests, this means that elements will be detached by reference and will therefore not have properties
+included. The relevant configuration from the Gremlin Server initialization script looks like this:
-For bytecode-based requests, graph elements have reference detachment and thus only return the `id` and `label` of
-the elements. While this approach alleviates a potential performance problem that the script approach exposes, it is
-still important to follow the practice of being specific about the data that is required by the requesting application
-as it won't arrive on the client side without that declaration.
-
-Ultimately, the detachment model should have little impact to Gremlin usage if the best practice of specifying only
-the data required by the application is adhered to. In other words, while there may be a difference in the contents
-of return values for these traversals:
-
-[source,java]
+[source,groovy]
----
-// properties returned from g.V().hasLabel('person') because this is using the
-// Script API with full detachment
-Cluster cluster = Cluster.open();
-Client client = cluster.connect();
-ResultSet results = client.submit("g.V().hasLabel('person')");
-
-// no properties returned from g.V().hasLabel("person") because this is using
-// Bytecode API with reference detachment
-GraphTraversalSource g = traversal().withRemote('conf/remote-graph.properties');
-List<Vertex> results = g.V().hasLabel("person").toList();
+globals << [g : traversal().withEmbedded(graph).withStrategies(ReferenceElementStrategy)]
----
-There is no difference if re-written using the best practice of requesting only the data the application needs:
+This configuration is global to Gremlin Server and therefore all methods of connection will always return elements
+without properties. If this strategy is not included, then elements returned with properties.
Review Comment:
```suggestion
without properties. If this strategy is not included, then elements will be returned with properties.
```
##########
docs/src/reference/gremlin-applications.asciidoc:
##########
@@ -2250,50 +2250,36 @@ to as "detached elements" and cases where properties are not included are "refer
With the type of request and detachment model in mind, it is now possible to discuss how best to consider element
properties in relation to them all in concert.
-By default, Gremlin Server sample configurations utilize `ReferenceElementStrategy` when creating the out-of-the-box
-`GraphTraversalSource`. As the name suggests, this means that elements will be detached by reference and will
-therefore not have properties included. The relevant configuration from the Gremlin Server initialization script looks
-like this:
+By default, Gremlin Server sample configurations return all properties.
+
+To manage properties for each request you can use <<configuration-steps-with,with()>> configuration option
+`materializeProperties`
[source,groovy]
----
-globals << [g : traversal().withEmbedded(graph).withStrategies(ReferenceElementStrategy)]
+g.with('materializeProperties', 'tokens').V()
----
-This configuration is global to Gremlin Server and therefore all methods of connection will always return elements
-without properties. If this strategy is not included, then there are other considerations to take into account such as
-the connection type (i.e. script or bytecode) and the serializer.
+The `tokens` value for the `materializeProperties` means that need to return only `id` and `label`. Another option
+`all` used to indicate that all properties should be returned and is the default value.
-For script-based requests, users should take care when returning graph elements. By default, elements will be returned
-as "detached elements" and depending on the serializer being used those detached elements may or may not have their
-properties carried with them. Gryo and GraphSON serializers will write all properties in the return payload in this
-case but GraphBinary will not. Therefore, script-based requests that use Gryo or GraphSON should definitely follow the
-best practice of only returning the data required by the application.
+In some cases it can be inconvenient to load vertices with properties due to fat data or for compatibility reasons.
+That can be solved by utilize `ReferenceElementStrategy` when creating the out-of-the-box `GraphTraversalSource`.
Review Comment:
```suggestion
That can be solved by utilizing `ReferenceElementStrategy` when creating the out-of-the-box `GraphTraversalSource`.
```
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1130886238
##########
docs/src/upgrade/release-3.7.x.asciidoc:
##########
@@ -29,6 +29,28 @@ Please see the link:https://github.com/apache/tinkerpop/blob/3.7.0/CHANGELOG.asc
=== Upgrading for Users
+==== Properties on Elements
Review Comment:
your recent work shows `ReferenceElement`-type objects are no longer returned - you get a `DetachedElement` from remote requests. Folks may not have been coding to the `Element` interfaces as they likely should have been and therefore could been working directly with "reference" implementations. I think upgrade docs should point out the change and the appropriate way to avoid problems in the future - i.e. use the interfaces like `Vertex` and `Edge`.
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1130235764
##########
gremlin-server/src/test/scripts/generate-all.groovy:
##########
@@ -61,7 +61,7 @@ globals << [hook : [
// add default TraversalSource instances for each graph instance
globals << [gclassic : traversal().withEmbedded(classic).withStrategies(ReferenceElementStrategy)]
Review Comment:
sorry - what am i forgetting? what tests rely on `ReferenceElementStrategy` to be present?
ideally, `ReferenceElementStrategy` should be a thing of the past, no?
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1018701870
##########
docs/src/reference/the-traversal.asciidoc:
##########
@@ -4916,6 +4916,28 @@ g.V().hasLabel('person'). <1>
<8> `PathRetractionStrategy` will remove paths from the traversers and increase the likelihood of bulking as path data is not required after `select('b')`.
<9> `AdjacentToIncidentStrategy` will turn `out()` into `outE()` to increase data access locality.
+=== DetachStrategy
+
+`DetachStrategy` manages the fields that will be included in the result. The strategy adds a step to the end of the traversal
+and does not affect the previous steps.
+There are 3 options for detachMode: `ALL` for all properties, `NONE` for no properties, `CUSTOM` for a specific list of properties.
+`id` and `label` always included in response.
+Same for Vertexes and Edges.
+
+WARNING: by default Gremlin Server configured to use `ReferenceElementStrategy` which removes all properties.
Review Comment:
I can change detach mode `NONE` or add detach mode `REFERENCE` to build `ReferenceElement`.
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1016951631
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/DetachStrategyProcessTest.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization;
+
+import org.apache.tinkerpop.gremlin.LoadGraphWith;
+import org.apache.tinkerpop.gremlin.process.AbstractGremlinProcessTest;
+import org.apache.tinkerpop.gremlin.process.IgnoreEngine;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalEngine;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.junit.Test;
+
+import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.MODERN;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+
+/**
+ * @author Valentyn Kahamlyk
+ */
+public class DetachStrategyProcessTest extends AbstractGremlinProcessTest {
+
+ @Test
+ @LoadGraphWith(MODERN)
+ @IgnoreEngine(TraversalEngine.Type.COMPUTER)
+ public void shouldReturnOnlyRequestedFields() {
+
+ final Vertex v = g.V(1).next();
+ final Vertex vWithAllProps = g
+ .withStrategies(DetachStrategy.build().detachMode(DetachStrategy.DetachMode.ALL).properties("name").create())
Review Comment:
throw exception when non-empty `properties` and detachment mode is not CUSTOM?
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1016949579
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/detached/DetachedFactory.java:
##########
@@ -115,4 +116,74 @@ public static <D> D detach(final Object object, final boolean withProperties) {
return (D) object;
}
}
+
+ // detached with options
+ public static DetachedVertex detach(final Vertex vertex, final DetachStrategy.DetachOptions detachOptions) {
Review Comment:
This overloads used in some other cases like Gryo Serializers, `DedupGlobalStep`, `EventStrategy`, etc.
So it seems to me that it is more convenient to keep `withProperties` as shortcut
--
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 #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1016676573
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/DetachStrategy.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization;
+
+import org.apache.commons.configuration2.Configuration;
+import org.apache.commons.configuration2.MapConfiguration;
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.ScalarMapStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.ProfileSideEffectStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
+import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedFactory;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A strategy that manages the properties that will be in the result.
+ *
+ * @author Valentyn Kahamlyk
+ */
+public final class DetachStrategy extends AbstractTraversalStrategy<TraversalStrategy.FinalizationStrategy> implements TraversalStrategy.FinalizationStrategy {
+
+ private static final DetachStrategy INSTANCE = new DetachStrategy(DetachMode.ALL, null);
+ private DetachMode detachMode = DetachMode.ALL;
+ private List<String> properties;
+
+ private DetachStrategy() {}
+
+ private DetachStrategy(final DetachMode detachMode, final List<String> properties) {
+ this.detachMode = detachMode;
+ this.properties = properties;
+ }
+
+ private DetachStrategy(final Builder builder) {
+ this(builder.detachMode, builder.properties);
+ }
+
+ @Override
+ public void apply(final Traversal.Admin<?, ?> traversal) {
+ if (traversal.getParent() == EmptyStep.instance()) {
+ final Optional<ProfileSideEffectStep> profileStep = TraversalHelper.getFirstStepOfAssignableClass(ProfileSideEffectStep.class, traversal);
+ final int index = profileStep.map(step -> traversal.getSteps().indexOf(step))
+ .orElseGet(() -> traversal.getSteps().size());
+ traversal.addStep(index,
+ new DetachElementStep<>(traversal, new DetachOptions(detachMode, properties)));
+ }
+ }
+
+ public static DetachStrategy create(final Configuration configuration) {
+ return new DetachStrategy(DetachMode.valueOf(configuration.getString(ID_MODE)),
+ new ArrayList<>((Collection<String>) configuration.getProperty(ID_PROPERTIES)));
+ }
+
+ /**
+ * Gets the standard configuration of this strategy that will return all properties.
+ */
+ public static DetachStrategy instance() {
+ return INSTANCE;
+ }
+
+ public static final String ID_MODE = "detachMode";
+ public static final String ID_PROPERTIES = "properties";
+
+ @Override
+ public Configuration getConfiguration() {
+ final Map<String, Object> map = new HashMap<>();
+ map.put(STRATEGY, DetachStrategy.class.getCanonicalName());
+ map.put(ID_MODE, this.detachMode.toString());
+ map.put(ID_PROPERTIES, this.properties);
+ return new MapConfiguration(map);
+ }
+
+ public enum DetachMode {
Review Comment:
it's too bad this enum addition forces all those serializer changes. is there other precedent for this in other strategies? in other words, do any other strategies introduce their own enums? maybe we can avoid this with some shift in the way we do the syntax?
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1116069853
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptCheckerTest.java:
##########
@@ -192,12 +194,41 @@ public void shouldIdentifyMultipleRequestIds() {
getRequestId().get());
}
+ @Test
Review Comment:
added in https://github.com/apache/tinkerpop/pull/1843/commits/8c36b01b673982875d9d9e04d91f3ddce73860f5
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -162,7 +168,10 @@ public class GremlinScriptChecker {
* </pre>
*/
private static final Pattern patternWithOptions =
- Pattern.compile("\\.with\\(((?:" + timeoutTokens + "),(?<to>\\d*)(:?L|l)?)|((?:" + requestIdTokens + "),[\"'](?<rid>.*?))[\"']\\)");
+ Pattern.compile("\\.with\\((((?:"
Review Comment:
benchmark added in https://github.com/apache/tinkerpop/pull/1843/commits/8c36b01b673982875d9d9e04d91f3ddce73860f5
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1130940538
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerSerializationIntegrateTest.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.server;
+
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.apache.tinkerpop.gremlin.util.ser.AbstractMessageSerializer;
+import org.apache.tinkerpop.gremlin.util.ser.GraphBinaryMessageSerializerV1;
+import org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV2d0;
+import org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV3d0;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+
+import static org.junit.Assert.assertEquals;
+
+@RunWith(Parameterized.class)
+public class GremlinServerSerializationIntegrateTest extends AbstractGremlinServerIntegrationTest {
+
+ private AbstractMessageSerializer serializer;
+ private Cluster cluster = null;
+ private Client client = null;
+ private GraphTraversalSource g = null;
+
+ public GremlinServerSerializationIntegrateTest(AbstractMessageSerializer serializer) {
+ this.serializer = serializer;
+ }
+
+ @Parameterized.Parameters
+ public static Collection serializers() {
+ return Arrays.asList(new Object[][] {
+ { new GraphBinaryMessageSerializerV1() },
+ { new GraphSONMessageSerializerV3d0() },
+ { new GraphSONMessageSerializerV2d0() }
+ });
+ }
+
+ @Before
+ public void openConnection() {
+ cluster = TestClientFactory.build().serializer(serializer).create();
+ client = cluster.connect();
+ g = AnonymousTraversalSource.traversal().withRemote(
+ DriverRemoteConnection.using(cluster, "gmodern"));
+ }
+
+ @After
+ public void closeConnection() {
+ if (cluster != null) cluster.close();
+ }
+
+ @Test
+ public void shouldDeserializeVertexPropertiesForGremlin() {
+ final Vertex vertex = client.submit("gmodern.V(1)").one().getVertex();
+
+ assertVertexWithProperties(vertex);
+ }
+
+ @Test
+ public void shouldSkipVertexPropertiesForGremlin() {
+ final Vertex vertex = client.submit("gmodern.with('materializeProperties', 'tokens').V(1)").one().getVertex();
+
+ assertEquals(Collections.emptyIterator(), vertex.properties());
Review Comment:
at risk of redundancy, but for sanity, please assert that the `T` values are present in addition to asserting the properties are not.
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1130920218
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerSerializationIntegrateTest.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.server;
+
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.apache.tinkerpop.gremlin.util.ser.AbstractMessageSerializer;
+import org.apache.tinkerpop.gremlin.util.ser.GraphBinaryMessageSerializerV1;
+import org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV2d0;
+import org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV3d0;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+
+import static org.junit.Assert.assertEquals;
+
+@RunWith(Parameterized.class)
+public class GremlinServerSerializationIntegrateTest extends AbstractGremlinServerIntegrationTest {
Review Comment:
given the issues with metaproperties/neo4j that i noted elsewhere, I think this test needs to include assertions for metaproperties and multiproperties to ensure all of that is working. Use the "crew" rather than "modern" graph for this since "crew" contains sample data for those things.
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1130915260
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/types/VertexPropertySerializer.java:
##########
@@ -66,7 +66,10 @@ protected void writeValue(final VertexProperty value, final Buffer buffer, final
// we don't serialize the parent vertex, let's hold a place for it
context.write(null, buffer);
- final List<?> asList = IteratorUtils.toList(value.properties());
- context.write(asList, buffer);
+ // Neo4jVertexProperty throw UnsupportedOperationException for properties,
+ // so for now VertexProperty will be serialized without inner properties
+ // final List<?> asList = IteratorUtils.toList(value.properties());
+ // context.write(asList, buffer);
+ context.write(null, buffer);
Review Comment:
I assume that this is temporary? not all graphs support meta/multiproperties so this is a likely to be a general problem. if you had the `Graph` or `g` you could test `features()` to see what the graph supported and then either write the metaproperties or not, but i don't think you have access to that here.
Any idea why this works for GraphSON without omitting the metaproperties?
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1131292023
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/types/VertexPropertySerializer.java:
##########
@@ -66,7 +66,10 @@ protected void writeValue(final VertexProperty value, final Buffer buffer, final
// we don't serialize the parent vertex, let's hold a place for it
context.write(null, buffer);
- final List<?> asList = IteratorUtils.toList(value.properties());
- context.write(asList, buffer);
+ // Neo4jVertexProperty throw UnsupportedOperationException for properties,
+ // so for now VertexProperty will be serialized without inner properties
+ // final List<?> asList = IteratorUtils.toList(value.properties());
+ // context.write(asList, buffer);
+ context.write(null, buffer);
Review Comment:
I'm not sure i understand the nature of the risk you're describing, but the `keys()` implementation for `Neo4jGraph` is strange anyway. It returns an empty `Set` while `properties()` throws an exception...that's pretty inconsistent. I"m not even sure `vertexPropertyNode` is relevant - i have a feeling it is dead code and `keys()` was missed in some refactoring at some point.
Neo4j really isn't the issue though. The real issue is how it works for any `Graph` implementation that doesn't support metaproperties. As the current approach has worked for years in GraphSON/Gryo I feel like we can fairly assume it should be safe for GraphBinary.
That said, perhaps we should do better than assume. You could encode the idea that `properties()` / `keys()` should not throw exceptions but instead return empty iterables. You'd do that by modifying test semantics for `FeatureSupportTest` and probably `VertexPropertyTest` (there might be others that fail as a result of this change). This change would require Upgrade Docs for providers to take note of. I think you'd want to wholly remove `VertexProperty.Exception.metaPropertiesNotSupported` basically and see what breaks.
--
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] vkagamlyk commented on a diff in pull request #1843: [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1004885773
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/DetachStrategy.java:
##########
@@ -0,0 +1,156 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization;
+
+import org.apache.commons.configuration2.Configuration;
+import org.apache.commons.configuration2.MapConfiguration;
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.ScalarMapStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.ProfileSideEffectStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
+import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedFactory;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A strategy that manages the properties that will be in the result.
+ *
+ * @author Valentyn Kahamlyk
+ */
+public final class DetachStrategy extends AbstractTraversalStrategy<TraversalStrategy.FinalizationStrategy> implements TraversalStrategy.FinalizationStrategy {
+
+ private DetachOptions detachOptions;
+
+ // todo: is public constructor ok or better make private?
+ public DetachStrategy(final DetachMode detachMode, final String[] properties) {
+ detachOptions = DetachOptions.build().detachMode(detachMode).properties(properties).create();
+ }
+
+ @Override
+ public void apply(final Traversal.Admin<?, ?> traversal) {
+ if (traversal.getParent() == EmptyStep.instance()) {
+ final Optional<ProfileSideEffectStep> profileStep = TraversalHelper.getFirstStepOfAssignableClass(ProfileSideEffectStep.class, traversal);
+ final int index = profileStep.map(step -> traversal.getSteps().indexOf(step))
+ .orElseGet(() -> traversal.getSteps().size());
+ traversal.addStep(index, new DetachElementStep<>(traversal, detachOptions));
+ }
+ }
+
+ public static DetachStrategy create(final Configuration configuration) {
+ return new DetachStrategy(DetachMode.valueOf(configuration.getString(ID_MODE)), configuration.getStringArray(ID_PROPERTIES));
+ }
+
+ public static final String ID_MODE = "mode";
+ public static final String ID_PROPERTIES = "properties";
+
+ @Override
+ public Configuration getConfiguration() {
+ final Map<String, Object> map = new HashMap<>();
+ map.put(STRATEGY, DetachStrategy.class.getCanonicalName());
+ map.put(ID_MODE, this.detachOptions.detachMode);
+ map.put(ID_PROPERTIES, this.detachOptions.properties);
+ return new MapConfiguration(map);
+ }
+
+ public enum DetachMode {
+
+ ALL,
+ CUSTOM,
+ NONE;
+ }
+
+ public static class DetachOptions {
+ private DetachMode detachMode;
+ private String[] properties;
+
+ private DetachOptions(final DetachOptions.Builder builder) {
+ this.detachMode = builder.detachMode;
+ this.properties = builder.properties;
+ }
+
+ public static DetachOptions.Builder build() {
+ return new DetachOptions.Builder();
+ }
+
+ public DetachMode getDetachMode() {
+ return this.detachMode;
+ }
+
+ public String[] getProperties() {
+ return this.properties;
+ }
+
+ public static final class Builder {
+
+ private DetachMode detachMode = DetachMode.NONE;
+ private String[] properties;
Review Comment:
null is expected default. Is it better to explicitly assign null?
--
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] vkagamlyk commented on a diff in pull request #1843: [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1004885029
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/detached/DetachedVertex.java:
##########
@@ -74,6 +75,38 @@ protected DetachedVertex(final Vertex vertex, final boolean withProperties) {
}
}
+ protected DetachedVertex(final Vertex vertex,
+ final DetachStrategy.DetachOptions detachOptions) {
+ super(vertex);
+
+ if (detachOptions.getDetachMode() == DetachStrategy.DetachMode.NONE
+ || detachOptions.getDetachMode() == DetachStrategy.DetachMode.CUSTOM && detachOptions.getProperties() == null)
+ return;
+
+ final Iterator<VertexProperty<Object>> propertyIterator = vertex.properties();
+ if (!propertyIterator.hasNext())
+ return;
+
+ this.properties = new HashMap<>();
+
+ if (detachOptions.getDetachMode() == DetachStrategy.DetachMode.ALL) {
Review Comment:
fixed in https://github.com/apache/tinkerpop/pull/1843/commits/8290b2049fb4876fd7ddeafd18a2345222f85597
--
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 #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1017858669
##########
docs/src/reference/the-traversal.asciidoc:
##########
@@ -4916,6 +4916,28 @@ g.V().hasLabel('person'). <1>
<8> `PathRetractionStrategy` will remove paths from the traversers and increase the likelihood of bulking as path data is not required after `select('b')`.
<9> `AdjacentToIncidentStrategy` will turn `out()` into `outE()` to increase data access locality.
+=== DetachStrategy
+
+`DetachStrategy` manages the fields that will be included in the result. The strategy adds a step to the end of the traversal
+and does not affect the previous steps.
+There are 3 options for detachMode: `ALL` for all properties, `NONE` for no properties, `CUSTOM` for a specific list of properties.
+`id` and `label` always included in response.
+Same for Vertexes and Edges.
+
+WARNING: by default Gremlin Server configured to use `ReferenceElementStrategy` which removes all properties.
Review Comment:
> I think ReferenceElementStrategy may be deprecated in 3.7.
There are some small differences between ReferenceElement and DetachedElement that need to be aligned before.
I'm not sure that `ReferenceElement` can be replaced by `DetachedElement` for all use cases. The original point of `ReferenceElement` is to avoid the the memory costs of even having an empty `Map<String, List<Property>> properties` declaration at all for OLAP use cases. `ReferenceElement` is meant to be as light as possible. If it could be proven that `DetachedElement` could have that same memory footprint I suppose the entire `ReferenceElement` infrastructure could be deprecated.
As far as `ReferenceElementStrategy` goes.......I think that can be deprecated as part of this because it is not use in OLAP iirc. It is only used for the case `DetachStrategy` is and is behaviorally identical for practical purposes.
> I consider default may be DetachStrategy configured to skip properties. I think users who want to get properties can remove the strategy from server startup or use it from the client when needed. Most current users don't expect to get properties and it doesn't make sense to load the server and network.
i'd agree. so the only real change users will get is that they will now get a `DetachedElement` rather than a `ReferenceElement` and hopefully they haven't been coding to the implementation and will not notice the difference as they would be working with the interface of `Element`. The upgrade documentation will need to be sure to include this notice.
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1016936659
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/DetachStrategy.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization;
+
+import org.apache.commons.configuration2.Configuration;
+import org.apache.commons.configuration2.MapConfiguration;
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.ScalarMapStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.ProfileSideEffectStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
+import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedFactory;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A strategy that manages the properties that will be in the result.
+ *
+ * @author Valentyn Kahamlyk
+ */
+public final class DetachStrategy extends AbstractTraversalStrategy<TraversalStrategy.FinalizationStrategy> implements TraversalStrategy.FinalizationStrategy {
+
+ private static final DetachStrategy INSTANCE = new DetachStrategy(DetachMode.ALL, null);
+ private DetachMode detachMode = DetachMode.ALL;
+ private List<String> properties;
+
+ private DetachStrategy() {}
+
+ private DetachStrategy(final DetachMode detachMode, final List<String> properties) {
+ this.detachMode = detachMode;
+ this.properties = properties;
+ }
+
+ private DetachStrategy(final Builder builder) {
+ this(builder.detachMode, builder.properties);
+ }
+
+ @Override
+ public void apply(final Traversal.Admin<?, ?> traversal) {
+ if (traversal.getParent() == EmptyStep.instance()) {
+ final Optional<ProfileSideEffectStep> profileStep = TraversalHelper.getFirstStepOfAssignableClass(ProfileSideEffectStep.class, traversal);
+ final int index = profileStep.map(step -> traversal.getSteps().indexOf(step))
+ .orElseGet(() -> traversal.getSteps().size());
+ traversal.addStep(index,
+ new DetachElementStep<>(traversal, new DetachOptions(detachMode, properties)));
+ }
+ }
+
+ public static DetachStrategy create(final Configuration configuration) {
+ return new DetachStrategy(DetachMode.valueOf(configuration.getString(ID_MODE)),
+ new ArrayList<>((Collection<String>) configuration.getProperty(ID_PROPERTIES)));
+ }
+
+ /**
+ * Gets the standard configuration of this strategy that will return all properties.
+ */
+ public static DetachStrategy instance() {
+ return INSTANCE;
+ }
+
+ public static final String ID_MODE = "detachMode";
+ public static final String ID_PROPERTIES = "properties";
+
+ @Override
+ public Configuration getConfiguration() {
+ final Map<String, Object> map = new HashMap<>();
+ map.put(STRATEGY, DetachStrategy.class.getCanonicalName());
+ map.put(ID_MODE, this.detachMode.toString());
+ map.put(ID_PROPERTIES, this.properties);
+ return new MapConfiguration(map);
+ }
+
+ public enum DetachMode {
Review Comment:
There are no other strategies that would add enums. May I just use a string value?
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1018702378
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/DetachStrategy.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization;
+
+import org.apache.commons.configuration2.Configuration;
+import org.apache.commons.configuration2.MapConfiguration;
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.ScalarMapStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.ProfileSideEffectStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
+import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedFactory;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A strategy that manages the properties that will be in the result.
+ *
+ * @author Valentyn Kahamlyk
+ */
+public final class DetachStrategy extends AbstractTraversalStrategy<TraversalStrategy.FinalizationStrategy> implements TraversalStrategy.FinalizationStrategy {
+
+ private static final DetachStrategy INSTANCE = new DetachStrategy(DetachMode.ALL, null);
+ private DetachMode detachMode = DetachMode.ALL;
+ private List<String> properties;
+
+ private DetachStrategy() {}
+
+ private DetachStrategy(final DetachMode detachMode, final List<String> properties) {
Review Comment:
done in https://github.com/apache/tinkerpop/pull/1843/commits/7d82c1b134de7b32e0f4f7f37262fbbcf1124654
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1130917537
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerSerializationIntegrateTest.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.server;
+
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.apache.tinkerpop.gremlin.util.ser.AbstractMessageSerializer;
+import org.apache.tinkerpop.gremlin.util.ser.GraphBinaryMessageSerializerV1;
+import org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV2d0;
+import org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV3d0;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+
+import static org.junit.Assert.assertEquals;
+
+@RunWith(Parameterized.class)
+public class GremlinServerSerializationIntegrateTest extends AbstractGremlinServerIntegrationTest {
+
+ private AbstractMessageSerializer serializer;
+ private Cluster cluster = null;
+ private Client client = null;
+ private GraphTraversalSource g = null;
+
+ public GremlinServerSerializationIntegrateTest(AbstractMessageSerializer serializer) {
+ this.serializer = serializer;
+ }
+
+ @Parameterized.Parameters
+ public static Collection serializers() {
+ return Arrays.asList(new Object[][] {
+ { new GraphBinaryMessageSerializerV1() },
+ { new GraphSONMessageSerializerV3d0() },
+ { new GraphSONMessageSerializerV2d0() }
+ });
+ }
+
+ @Before
+ public void openConnection() {
+ cluster = TestClientFactory.build().serializer(serializer).create();
+ client = cluster.connect();
+ g = AnonymousTraversalSource.traversal().withRemote(
+ DriverRemoteConnection.using(cluster, "gmodern"));
+ }
+
+ @After
+ public void closeConnection() {
+ if (cluster != null) cluster.close();
+ }
+
+ @Test
+ public void shouldDeserializeVertexPropertiesForGremlin() {
+ final Vertex vertex = client.submit("gmodern.V(1)").one().getVertex();
+
+ assertVertexWithProperties(vertex);
+ }
+
+ @Test
+ public void shouldSkipVertexPropertiesForGremlin() {
Review Comment:
nit: "ForGremlin" - prefer "ForScripts"
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1130920218
##########
gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerSerializationIntegrateTest.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.server;
+
+import org.apache.tinkerpop.gremlin.driver.Client;
+import org.apache.tinkerpop.gremlin.driver.Cluster;
+import org.apache.tinkerpop.gremlin.driver.remote.DriverRemoteConnection;
+import org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
+import org.apache.tinkerpop.gremlin.structure.Edge;
+import org.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
+import org.apache.tinkerpop.gremlin.util.ser.AbstractMessageSerializer;
+import org.apache.tinkerpop.gremlin.util.ser.GraphBinaryMessageSerializerV1;
+import org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV2d0;
+import org.apache.tinkerpop.gremlin.util.ser.GraphSONMessageSerializerV3d0;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+
+import static org.junit.Assert.assertEquals;
+
+@RunWith(Parameterized.class)
+public class GremlinServerSerializationIntegrateTest extends AbstractGremlinServerIntegrationTest {
Review Comment:
given the issues with metaproperties/neo4j that i noted elsewhere, I think this test needs to include assertions for metaproperties and multiproperties to ensure all of that is working.
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1109001720
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -34,15 +34,16 @@ public class GremlinScriptChecker {
/**
* An empty result whose properties return as empty.
*/
- public static final Result EMPTY_RESULT = new Result(null, null);
+ public static final Result EMPTY_RESULT = new Result(null, null, null);
/**
* At least one of these tokens should be present somewhere in the Gremlin string for {@link #parse(String)} to
* take any action at all.
*/
+ // todo: why not used `Tokens`?
Review Comment:
There's a bit of a mess with names for this constants.
For example we have constant for [evaluationTimeout](https://github.com/apache/tinkerpop/blob/master/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/Tokens.java#L78) and can use `Token.ARGS_EVAL_TIMEOUT`, not plain string. Same for `requestId`, but not for `scriptEvaluationTimeout`. Also `Tokens.ARGS_SCRIPT_EVAL_TIMEOUT` is not defined and I don't understand why.
I think better to use constants when possible and this can looks like
```
private static final Set<String> tokens = new HashSet<>(Arrays.asList(Tokens.ARGS_EVAL_TIMEOUT, Tokens.ARGS_SCRIPT_EVAL_TIMEOUT,
Tokens.REQUEST_ID, Tokens.ARGS_MATERIALIZE_PROPERTIES));
```
but made in the same style as before.
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1116070627
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -34,15 +34,16 @@ public class GremlinScriptChecker {
/**
* An empty result whose properties return as empty.
*/
- public static final Result EMPTY_RESULT = new Result(null, null);
+ public static final Result EMPTY_RESULT = new Result(null, null, null);
/**
* At least one of these tokens should be present somewhere in the Gremlin string for {@link #parse(String)} to
* take any action at all.
*/
+ // todo: why not used `Tokens`?
Review Comment:
added in https://github.com/apache/tinkerpop/pull/1843/commits/8c36b01b673982875d9d9e04d91f3ddce73860f5
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -162,7 +168,10 @@ public class GremlinScriptChecker {
* </pre>
*/
private static final Pattern patternWithOptions =
- Pattern.compile("\\.with\\(((?:" + timeoutTokens + "),(?<to>\\d*)(:?L|l)?)|((?:" + requestIdTokens + "),[\"'](?<rid>.*?))[\"']\\)");
+ Pattern.compile("\\.with\\((((?:"
Review Comment:
Benchmark Mode Cnt Score Error Units
GremlinScriptCheckerBenchmark.testParseAll thrpt 20 203909.105 ± 8915.473 ops/s
GremlinScriptCheckerBenchmark.testParseMaterializeProperties thrpt 20 484647.497 ± 15267.578 ops/s
GremlinScriptCheckerBenchmark.testParseRequestId thrpt 20 382702.687 ± 6140.784 ops/s
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1112961282
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -34,15 +34,16 @@ public class GremlinScriptChecker {
/**
* An empty result whose properties return as empty.
*/
- public static final Result EMPTY_RESULT = new Result(null, null);
+ public static final Result EMPTY_RESULT = new Result(null, null, null);
/**
* At least one of these tokens should be present somewhere in the Gremlin string for {@link #parse(String)} to
* take any action at all.
*/
+ // todo: why not used `Tokens`?
Review Comment:
We have to check all the forms. if you just did:
```
private static final Set<String> tokens = new HashSet<>(Arrays.asList(Tokens.ARGS_EVAL_TIMEOUT, Tokens.ARGS_SCRIPT_EVAL_TIMEOUT,
Tokens.REQUEST_ID, Tokens.ARGS_MATERIALIZE_PROPERTIES));
```
then if the user sent a string query of: `g.with(Tokens.ARGS_EVAL_TIMEOUT, 100)` the checker wouldn't find it. `Tokens.ARGS_EVAL_TIMEOUT` is basically a valid part of the Gremlin string someone might send.
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1122096235
##########
docs/src/upgrade/release-3.7.x.asciidoc:
##########
@@ -29,6 +29,28 @@ Please see the link:https://github.com/apache/tinkerpop/blob/3.7.0/CHANGELOG.asc
=== Upgrading for Users
+==== Properties on Elements
Review Comment:
This is a major piece of functionality. I think the upgrade documentation needs more content to really impress upon folks just what impact this change has for them. some background on the previous behavior would help, a link to the "history" of why it was that way would be good, some example code, etc.
I can help with polishing up wording, but please add a bit more here for us to review.
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1122319496
##########
docs/src/upgrade/release-3.7.x.asciidoc:
##########
@@ -29,6 +29,28 @@ Please see the link:https://github.com/apache/tinkerpop/blob/3.7.0/CHANGELOG.asc
=== Upgrading for Users
+==== Properties on Elements
Review Comment:
different behavior for different GLV's
Python, Go: deserialization error for GraphBinary, skip properties for GraphSON
.NET: skip properties
Javascript: should work ok
also serialization can't be managed with `materializeProperties` option
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1108935201
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptCheckerTest.java:
##########
@@ -192,12 +194,41 @@ public void shouldIdentifyMultipleRequestIds() {
getRequestId().get());
}
+ @Test
Review Comment:
please include test variations for `Tokens` styled naming when that's added given the previous comments.
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1110222260
##########
gremlin-dotnet/src/Gremlin.Net/Structure/Edge.cs:
##########
@@ -52,6 +55,15 @@ public Edge(object? id, Vertex outV, string label, Vertex inV)
/// </summary>
public Vertex OutV { get; }
+ /// <summary>
+ /// Get property by key
+ /// </summary>
+ /// <returns>property or null when not found</returns>
+ public Property? Property(string key)
Review Comment:
I considered 3 options:
1. minimal change, just extend `Vertex` and `Edge` classes with new typed `Property` method like `public Property? Property(string key)`
2. non-breaking change, add dynamic `Property` method to the base `Element` class like `public dynamic? Property(string key)`, it's less convenient for users
3. breaking change, make `Element' type generic `Element<TPropertyType>` and method like `public TPropertyType? Property(string key)`
first one is safest and at the same time user-friendly, so I chose it
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1110225081
##########
gremlin-dotnet/src/Gremlin.Net/Structure/IO/GraphBinary/Types/EdgeSerializer.cs:
##########
@@ -45,15 +46,17 @@ public EdgeSerializer() : base(DataType.Edge)
{
await writer.WriteAsync(value.Id, stream, cancellationToken).ConfigureAwait(false);
await writer.WriteNonNullableValueAsync(value.Label, stream, cancellationToken).ConfigureAwait(false);
-
+
+ // !!! await writer.WriteValueAsync(value.Label, stream, false, cancellationToken).ConfigureAwait(false);
Review Comment:
I forgot to remove this line
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1113630711
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -228,11 +244,20 @@ public Optional<String> getRequestId() {
return null == requestId ? Optional.empty() : Optional.of(requestId);
}
+ /**
+ * Gets the value of the request identifier supplied using the {@code with()} source step. If there are
Review Comment:
and 2 years ago you preferred to use `code` :) and I did the same
fixed in https://github.com/apache/tinkerpop/pull/1843/commits/143a2bf5b39a26412e501bc3a3f6f564d32d9afd
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1108985437
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js:
##########
@@ -131,17 +131,30 @@ Given(/^using the parameter (.+) defined as "(.+)"$/, function (paramName, strin
});
});
+var removeProperties = function(p) {
+ if (p === undefined) {
+ } else if (p instanceof graphModule.Vertex || p instanceof graphModule.Edge) {
+ p.properties = undefined;
+ } else if (p instanceof Array) {
+ p.forEach(removeProperties)
+ } else if (p instanceof Map) {
+ removeProperties(Array.from(p.keys()))
+ removeProperties(Array.from(p.values()))
+ } else if (p instanceof graphModule.Path) {
+ removeProperties(p.objects)
+ }
+
+ return p
+}
+
When('iterated to list', function () {
- return this.traversal.toList().then(list => this.result = list).catch(err => this.result = err);
+ return this.traversal.toList().then(list => this.result = removeProperties(list)).catch(err => this.result = err);
Review Comment:
Current state:
1. different GLV's use different ways to compare elements, for example Java and .NET consider Elements equal when ID's equal. But in Javascript used memberwise comparison, so Elements equal only when all properties equal.
2. Javascript client already contains property deserialization, and format is different for GraphSON and GraphBinary.
For GraphSON properties is object, for GraphBinary used plain array.
As a result Vertex loaded with GraphSON is not equal to same Vertex got with GraphBinary due to different properties format, so I excluded properties from elements for Javascript Gherkin tests to pass existing tests.
--
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 #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1016670856
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/finalization/DetachStrategy.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * 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.
+ */
+package org.apache.tinkerpop.gremlin.process.traversal.strategy.finalization;
+
+import org.apache.commons.configuration2.Configuration;
+import org.apache.commons.configuration2.MapConfiguration;
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.ScalarMapStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.ProfileSideEffectStep;
+import org.apache.tinkerpop.gremlin.process.traversal.step.util.EmptyStep;
+import org.apache.tinkerpop.gremlin.process.traversal.strategy.AbstractTraversalStrategy;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
+import org.apache.tinkerpop.gremlin.structure.util.detached.DetachedFactory;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * A strategy that manages the properties that will be in the result.
+ *
+ * @author Valentyn Kahamlyk
+ */
+public final class DetachStrategy extends AbstractTraversalStrategy<TraversalStrategy.FinalizationStrategy> implements TraversalStrategy.FinalizationStrategy {
+
+ private static final DetachStrategy INSTANCE = new DetachStrategy(DetachMode.ALL, null);
+ private DetachMode detachMode = DetachMode.ALL;
+ private List<String> properties;
+
+ private DetachStrategy() {}
+
+ private DetachStrategy(final DetachMode detachMode, final List<String> properties) {
Review Comment:
would a `Set` be better than a `List` for the properties? i think it might be better to refer to these things as "keys" as "property" tends to denote a `Property` object typically in TinkerPop parlance.
--
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 #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1016683949
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/detached/DetachedFactory.java:
##########
@@ -115,4 +116,74 @@ public static <D> D detach(final Object object, final boolean withProperties) {
return (D) object;
}
}
+
+ // detached with options
+ public static DetachedVertex detach(final Vertex vertex, final DetachStrategy.DetachOptions detachOptions) {
Review Comment:
i think we'd want to deprecate the `withProperties` overloads for these methods and prefer this new one. thoughts?
--
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 #1843: TINKERPOP-2824 [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1018974853
##########
docs/src/reference/the-traversal.asciidoc:
##########
@@ -4916,6 +4916,28 @@ g.V().hasLabel('person'). <1>
<8> `PathRetractionStrategy` will remove paths from the traversers and increase the likelihood of bulking as path data is not required after `select('b')`.
<9> `AdjacentToIncidentStrategy` will turn `out()` into `outE()` to increase data access locality.
+=== DetachStrategy
+
+`DetachStrategy` manages the fields that will be included in the result. The strategy adds a step to the end of the traversal
+and does not affect the previous steps.
+There are 3 options for detachMode: `ALL` for all properties, `NONE` for no properties, `CUSTOM` for a specific list of properties.
+`id` and `label` always included in response.
+Same for Vertexes and Edges.
+
+WARNING: by default Gremlin Server configured to use `ReferenceElementStrategy` which removes all properties.
Review Comment:
that's interesting. We often refer to getting a `ReferenceElement` as "reference detachment" so that might be a neat way to centralize all detachment functions behind a single strategy. seems like a good idea imo
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1131244071
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/types/VertexPropertySerializer.java:
##########
@@ -66,7 +66,10 @@ protected void writeValue(final VertexProperty value, final Buffer buffer, final
// we don't serialize the parent vertex, let's hold a place for it
context.write(null, buffer);
- final List<?> asList = IteratorUtils.toList(value.properties());
- context.write(asList, buffer);
+ // Neo4jVertexProperty throw UnsupportedOperationException for properties,
+ // so for now VertexProperty will be serialized without inner properties
+ // final List<?> asList = IteratorUtils.toList(value.properties());
+ // context.write(asList, buffer);
+ context.write(null, buffer);
Review Comment:
what is the plan to re-enable 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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1131251402
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/types/VertexPropertySerializer.java:
##########
@@ -66,7 +66,10 @@ protected void writeValue(final VertexProperty value, final Buffer buffer, final
// we don't serialize the parent vertex, let's hold a place for it
context.write(null, buffer);
- final List<?> asList = IteratorUtils.toList(value.properties());
- context.write(asList, buffer);
+ // Neo4jVertexProperty throw UnsupportedOperationException for properties,
+ // so for now VertexProperty will be serialized without inner properties
+ // final List<?> asList = IteratorUtils.toList(value.properties());
+ // context.write(asList, buffer);
+ context.write(null, buffer);
Review Comment:
I can do it like in GraphSON, but then there will be a risk of getting a problem if some provider wants to use `vertexPropertyNode`.
Do you have other suggestions?
--
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] cole-bq commented on a diff in pull request #1843: [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
cole-bq commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1004745492
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/detached/DetachedVertex.java:
##########
@@ -74,6 +75,38 @@ protected DetachedVertex(final Vertex vertex, final boolean withProperties) {
}
}
+ protected DetachedVertex(final Vertex vertex,
+ final DetachStrategy.DetachOptions detachOptions) {
+ super(vertex);
+
+ if (detachOptions.getDetachMode() == DetachStrategy.DetachMode.NONE
+ || detachOptions.getDetachMode() == DetachStrategy.DetachMode.CUSTOM && detachOptions.getProperties() == null)
+ return;
+
+ final Iterator<VertexProperty<Object>> propertyIterator = vertex.properties();
+ if (!propertyIterator.hasNext())
+ return;
+
+ this.properties = new HashMap<>();
+
+ if (detachOptions.getDetachMode() == DetachStrategy.DetachMode.ALL) {
Review Comment:
There is some code duplication between this case and DetachedVertex(vertex, true). Could potentially have the above constructor (DetachedVertex(final Vertex vertex, final boolean withProperties)) make calls to this constructor to avoid duplication.
--
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 #1843: [WIP] Properties on response elements
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#issuecomment-1289535166
# [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1843?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 [#1843](https://codecov.io/gh/apache/tinkerpop/pull/1843?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ff00dfa) into [master](https://codecov.io/gh/apache/tinkerpop/commit/fec78da4711128e9f643a69492dbaaa7817b1f70?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fec78da) will **decrease** coverage by `5.06%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #1843 +/- ##
============================================
- Coverage 69.17% 64.11% -5.07%
============================================
Files 875 23 -852
Lines 41759 3684 -38075
Branches 5623 0 -5623
============================================
- Hits 28888 2362 -26526
+ Misses 10894 1154 -9740
+ Partials 1977 168 -1809
```
| [Impacted Files](https://codecov.io/gh/apache/tinkerpop/pull/1843?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/structure/util/detached/DetachedEdge.java](https://codecov.io/gh/apache/tinkerpop/pull/1843/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-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9zdHJ1Y3R1cmUvdXRpbC9kZXRhY2hlZC9EZXRhY2hlZEVkZ2UuamF2YQ==) | | |
| [...emlin/structure/util/detached/DetachedFactory.java](https://codecov.io/gh/apache/tinkerpop/pull/1843/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-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9zdHJ1Y3R1cmUvdXRpbC9kZXRhY2hlZC9EZXRhY2hlZEZhY3RvcnkuamF2YQ==) | | |
| [...remlin/structure/util/detached/DetachedVertex.java](https://codecov.io/gh/apache/tinkerpop/pull/1843/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-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9zdHJ1Y3R1cmUvdXRpbC9kZXRhY2hlZC9EZXRhY2hlZFZlcnRleC5qYXZh) | | |
| [...tructure/util/detached/DetachedVertexProperty.java](https://codecov.io/gh/apache/tinkerpop/pull/1843/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-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9zdHJ1Y3R1cmUvdXRpbC9kZXRhY2hlZC9EZXRhY2hlZFZlcnRleFByb3BlcnR5LmphdmE=) | | |
| [...erpop/gremlin/groovy/loaders/StrategyLoader.groovy](https://codecov.io/gh/apache/tinkerpop/pull/1843/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-Z3JlbWxpbi1ncm9vdnkvc3JjL21haW4vZ3Jvb3Z5L29yZy9hcGFjaGUvdGlua2VycG9wL2dyZW1saW4vZ3Jvb3Z5L2xvYWRlcnMvU3RyYXRlZ3lMb2FkZXIuZ3Jvb3Z5) | | |
| [...jsr223/ast/AmbiguousMethodASTTransformation.groovy](https://codecov.io/gh/apache/tinkerpop/pull/1843/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-Z3JlbWxpbi1ncm9vdnkvc3JjL21haW4vZ3Jvb3Z5L29yZy9hcGFjaGUvdGlua2VycG9wL2dyZW1saW4vZ3Jvb3Z5L2pzcjIyMy9hc3QvQW1iaWd1b3VzTWV0aG9kQVNUVHJhbnNmb3JtYXRpb24uZ3Jvb3Z5) | | |
| [.../apache/tinkerpop/gremlin/structure/util/Host.java](https://codecov.io/gh/apache/tinkerpop/pull/1843/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-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9zdHJ1Y3R1cmUvdXRpbC9Ib3N0LmphdmE=) | | |
| [...structure/io/binary/types/LocalDateSerializer.java](https://codecov.io/gh/apache/tinkerpop/pull/1843/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-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9zdHJ1Y3R1cmUvaW8vYmluYXJ5L3R5cGVzL0xvY2FsRGF0ZVNlcmlhbGl6ZXIuamF2YQ==) | | |
| [...nguage/grammar/TraversalTerminalMethodVisitor.java](https://codecov.io/gh/apache/tinkerpop/pull/1843/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-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9sYW5ndWFnZS9ncmFtbWFyL1RyYXZlcnNhbFRlcm1pbmFsTWV0aG9kVmlzaXRvci5qYXZh) | | |
| [...kerpop/gremlin/structure/io/graphson/TypeInfo.java](https://codecov.io/gh/apache/tinkerpop/pull/1843/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-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9zdHJ1Y3R1cmUvaW8vZ3JhcGhzb24vVHlwZUluZm8uamF2YQ==) | | |
| ... and [842 more](https://codecov.io/gh/apache/tinkerpop/pull/1843/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] spmallette commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1108926694
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/binary/types/EdgeSerializer.java:
##########
@@ -71,8 +73,12 @@ protected void writeValue(final Edge value, final Buffer buffer, final GraphBina
// we don't serialize the parent Vertex for edges. they are "references", but we leave a place holder
// here as an option for the future as we've waffled this soooooooooo many times now
context.write(null, buffer);
- // we don't serialize properties for graph vertices/edges. they are "references", but we leave a place holder
- // here as an option for the future as we've waffled this soooooooooo many times now
- context.write(null, buffer);
+ if (value.properties() == null) {
+ context.write(null, buffer);
+ }
+ else {
+ List<?> asList = IteratorUtils.toList(value.properties());
Review Comment:
nit: please do a check for variables that can be marked `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] spmallette commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1108919213
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -162,7 +168,10 @@ public class GremlinScriptChecker {
* </pre>
*/
private static final Pattern patternWithOptions =
- Pattern.compile("\\.with\\(((?:" + timeoutTokens + "),(?<to>\\d*)(:?L|l)?)|((?:" + requestIdTokens + "),[\"'](?<rid>.*?))[\"']\\)");
+ Pattern.compile("\\.with\\((((?:"
Review Comment:
every time i touch this, i've double checked to make sure there is no significant performance penalty. this checker is executed a lot so we'd want to know if a slow down was happening here. could you just validate that there is no significant change with a quick benchmark? i don't expect anything given what i recall from the last update here, but i think it's worth continuing to validate assumptions on this one. maybe even make this a permanent benchmark in gremlin-benchmarks so that we dont have to start from scratch each time?
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1109001720
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -34,15 +34,16 @@ public class GremlinScriptChecker {
/**
* An empty result whose properties return as empty.
*/
- public static final Result EMPTY_RESULT = new Result(null, null);
+ public static final Result EMPTY_RESULT = new Result(null, null, null);
/**
* At least one of these tokens should be present somewhere in the Gremlin string for {@link #parse(String)} to
* take any action at all.
*/
+ // todo: why not used `Tokens`?
Review Comment:
There's a bit of a mess with names for this constants.
For example we have constant for [evaluationTimeout](https://github.com/apache/tinkerpop/blob/master/gremlin-util/src/main/java/org/apache/tinkerpop/gremlin/util/Tokens.java#L78) and can use `Token.ARGS_EVAL_TIMEOUT`, not plain string. Same for `requestId`, but not for `scriptEvaluationTimeout`. Also `Tokens.ARGS_SCRIPT_EVAL_TIMEOUT` is not defined and I don't understand why.
I think better to use constants when possible and this can looks like
```
private static final Set<String> tokens = new HashSet<>(Arrays.asList(Tokens.ARGS_EVAL_TIMEOUT, Tokens.ARGS_SCRIPT_EVAL_TIMEOUT,
Tokens.REQUEST_ID, Tokens.ARGS_MATERIALIZE_PROPERTIES));
```
but I made in the same style as before.
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#issuecomment-1450565119
i think you need multiple CHANGELOG entries on this one. you added one feature but a lot of things changed like, major changes to different serializers, changes to where `HaltedTraverserStrategy` was used, `GremlinScriptChecker` changes, etc.
--
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] Cole-Greer commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1123865203
##########
docs/src/upgrade/release-3.7.x.asciidoc:
##########
@@ -29,9 +29,73 @@ Please see the link:https://github.com/apache/tinkerpop/blob/3.7.0/CHANGELOG.asc
=== Upgrading for Users
+==== Properties on Elements
+
+===== Introduction
+
+By default properties on Elements are now returned for OLTP requests. Gremlin Server 3.5 and 3.6 does not return properties by default.
+More hitory details about serialization of properties can be found in the link:https://lists.apache.org/thread/xltcon4zxnwq4fyw2r2126syyrqm8spy[Stephen's post].
+
+===== Behavior for OLAP queries
+
+Queries still won't return Properties on Elements. The main reason for this is performance considerations.
+If you need to get a properties, then this can be explicitly configured with `HaltedTraverserStrategy`
+
+[source,java]
+----
+ g.withComputer().withStrategies(HaltedTraverserFactoryStrategy.detached())
+----
+
+===== Output compare for Gremlin Server 3.5/3.6 and 3.7
+
+Let's take a closer look at Javascript GLV code example in 3.6 and 3.7
+
+[source,javascript]
+----
+ const client = new Client('ws://localhost:8182/gremlin',{traversalSource: 'gmodern'});
+ await client.open();
+ const result = await client.submit('g.V(1)');
+ console.log(JSON.stringify(result.first()));
+ await client.close();
+----
+
+The result will be different depending on the version of Gremlin Server.
+For 3.5/3.6:
+[source,json]
+----
+ {"id":1,"label":"person"}
+----
+
+For 3.7:
+[source,json]
+----
+ {"id":1,"label":"person","properties":{"name":[{"id":0,"label":"name","value":"marko","key":"name"}],"age":[{"id":1,"label":"age","value":29,"key":"age"}]}}
+----
+
+===== Enabling the previous behavior
+
+GLV's 3.5/3.6 will not be able to work correctly with Properties on Elements. If you don't need to get properties then you can do one of the following ways:
+
+* Configure Gremlin Server to not return properties, update Gremlin Server initialization script with `ReferenceElementStrategy`.
+Also this method is better to use with 3.5/3.6 drivers.
+For example
+[source,groovy]
+----
+globals << [g : traversal().withEmbedded(graph).withStrategies(ReferenceElementStrategy)]
+----
+
+* Use config per request `with('materializeProperties', 'tokens')`
+[source,csharp]
+----
+g.With(Tokens.ArgMaterializeProperties, "tokens").V(1).Next()
+----
+
+See: link:https://issues.apache.org/jira/browse/TINKERPOP-2824[TINKERPOP-2824]
+
+
==== Gremlin.NET: Nullable Annotations
-Gremlin.NET now uses link:https://learn.microsoft.com/en-us/dotnet/csharp/nullable-references#nullable-variable-annotations[nullable annotations]
+Gremlin.NET now uses link:https://learn.microsoft.com/en-us/dotnet/sharp/nullable-references#nullable-variable-annotations[nullable annotations]
Review Comment:
I believe this edit was added by mistake. New link results in a 404
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1130208183
##########
gremlin-server/src/test/scripts/generate-all.groovy:
##########
@@ -61,7 +61,7 @@ globals << [hook : [
// add default TraversalSource instances for each graph instance
globals << [gclassic : traversal().withEmbedded(classic).withStrategies(ReferenceElementStrategy)]
Review Comment:
to reduce the number of problems with tests
--
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] vkagamlyk commented on pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#issuecomment-1470560287
Replaced with squashed https://github.com/apache/tinkerpop/pull/1992
--
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] vkagamlyk commented on pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#issuecomment-1435427556
> Great change! I just added a few comments but basically all of them are only nitpicks.
>
> Apart from that, I don't see any documentation for this new functionality. I think this needs to be documented in a lot of places (upgrade docs, changelog, reference docs, I/O docs). We should also definitely document that this changes the default behavior which will now include properties for all returned vertices and edges.
>
> Do you want to do that in a follow-up PR?
I plan to add documentation update to this PR, I first wanted the PR to be more or less formed
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1110226248
##########
gremlin-server/src/test/scripts/generate-all.groovy:
##########
@@ -61,7 +61,7 @@ globals << [hook : [
// add default TraversalSource instances for each graph instance
globals << [gclassic : traversal().withEmbedded(classic).withStrategies(ReferenceElementStrategy)]
-globals << [gmodern : traversal().withEmbedded(modern).withStrategies(ReferenceElementStrategy)]
+globals << [gmodern : traversal().withEmbedded(modern)]
Review Comment:
Yes, I used `gmodern` for tests
--
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 #1843: TINKERPOP-2824 Properties on response elements
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1112968074
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/jsr223/GremlinScriptChecker.java:
##########
@@ -162,7 +168,10 @@ public class GremlinScriptChecker {
* </pre>
*/
private static final Pattern patternWithOptions =
- Pattern.compile("\\.with\\(((?:" + timeoutTokens + "),(?<to>\\d*)(:?L|l)?)|((?:" + requestIdTokens + "),[\"'](?<rid>.*?))[\"']\\)");
+ Pattern.compile("\\.with\\((((?:"
Review Comment:
please update the "explain" in the javadoc: https://regex101.com/
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1113603950
##########
gremlin-go/driver/graphBinary.go:
##########
@@ -1052,8 +1052,8 @@ func vertexReader(data *[]byte, i *int) (interface{}, error) {
return vertexReaderNullByte(data, i, true)
}
-// {fully qualified id}{unqualified label}{[unused null byte]}
-func vertexReaderNullByte(data *[]byte, i *int, unusedByte bool) (interface{}, error) {
+// {fully qualified id}{unqualified label}{fully qualified properties}
+func vertexReaderNullByte(data *[]byte, i *int, readProperties bool) (interface{}, error) {
Review Comment:
sounds good, renamed in https://github.com/apache/tinkerpop/pull/1843/commits/31abd69ac156047a4de748011ea768d8bf9cdae9
--
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] vkagamlyk commented on a diff in pull request #1843: TINKERPOP-2824 Properties on response elements
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #1843:
URL: https://github.com/apache/tinkerpop/pull/1843#discussion_r1113664155
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/test/cucumber/feature-steps.js:
##########
@@ -131,17 +131,30 @@ Given(/^using the parameter (.+) defined as "(.+)"$/, function (paramName, strin
});
});
+var removeProperties = function(p) {
+ if (p === undefined) {
+ } else if (p instanceof graphModule.Vertex || p instanceof graphModule.Edge) {
+ p.properties = undefined;
+ } else if (p instanceof Array) {
+ p.forEach(removeProperties)
+ } else if (p instanceof Map) {
+ removeProperties(Array.from(p.keys()))
+ removeProperties(Array.from(p.values()))
+ } else if (p instanceof graphModule.Path) {
+ removeProperties(p.objects)
+ }
+
+ return p
+}
+
When('iterated to list', function () {
- return this.traversal.toList().then(list => this.result = list).catch(err => this.result = err);
+ return this.traversal.toList().then(list => this.result = removeProperties(list)).catch(err => this.result = err);
Review Comment:
I created ticket: https://issues.apache.org/jira/browse/TINKERPOP-2872
Perhaps it is also worth discussing to get the same properties format for GraphSON and GraphBinary.
--
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