You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "xiazcy (via GitHub)" <gi...@apache.org> on 2023/06/16 07:30:50 UTC
[GitHub] [tinkerpop] xiazcy opened a new pull request, #2099: [WIP] TINKERPOP-2672 - string manipulation function concat()
xiazcy opened a new pull request, #2099:
URL: https://github.com/apache/tinkerpop/pull/2099
Implementation for string `concat()` with reference to on the [remove-closures proposal](https://github.com/apache/tinkerpop/blob/master/docs/src/dev/future/proposal-3-remove-closures.asciidoc#concat), and not considering the scope for list `concat()`
Currently a work-in-progress, as parameter checks, corner cases consideration, and additional tests are still needed, as well as potential changes to implementation.
Also pending rebase to new grammar changes in https://github.com/apache/tinkerpop/pull/2094.
--
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 #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1262596321
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/Concat.feature:
##########
@@ -0,0 +1,213 @@
+# 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.
+
+@StepClassMap @StepConcat
+Feature: Step - concat()
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | a |
+ | b |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_XcX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | ac |
+ | bc |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xc_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c", "d")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acd |
+ | bcd |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xinject_c_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat(__.inject("c", "d"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acda |
Review Comment:
The semantics of `inject()` are sometimes hard to think about and I think that's what makes this example look strange. Consider:
```
gremlin> g.inject("a","b").map(inject("c","d"))
==>a
==>b
gremlin> g.inject("a","b").map(inject("c","d").fold())
==>[a,c,d]
==>[b]
gremlin> g.inject("a","b").project("a").by(inject('c','d').fold())
==>[a:[a,c,d]]
==>[a:[b]]
gremlin> g.inject("a","b").union(inject("c","d"))
==>a
==>c
==>d
==>b
gremlin> g.inject("a","b").inject("c","d")
==>c
==>d
==>a
==>b
```
I can't seem to find context where `inject()` resets, so given that, perhaps the expectation for the `concat()` is maybe:
```
| result |
| ac |
| bd |
```
???
--
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 #2099: String manipulation function concat()
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1261777611
##########
docs/src/reference/the-traversal.asciidoc:
##########
@@ -1049,6 +1049,44 @@ g.V().coin(1.0)
link:++https://tinkerpop.apache.org/javadocs/x.y.z/core/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.html#coin-double-++[`coin(double)`]
+[[concat-step]]
+=== Concat Step
+
+The `concat()`-step (*map*) concatenates one or more String values together to the incoming String traverser.
+If the incoming traverser is a non-String value then an `IllegalArgumentException` will be thrown.
+
+[gremlin-groovy,modern]
+----
+g.addV(constant("prefix_").concat(__.V(1).label())).property(id, 10) <1>
+g.V(10).label()
+g.V().hasLabel('person').values('name').as('a').
+ constant('Mr.').concat(__.select('a')) <2>
+g.V().hasLabel('software').as('a').values('name').
+ concat(' uses ').
+ concat(select('a').values('lang')) <3>
+g.V(1).outE().as("a").constant("").
+ concat(V(1).values("name")).
+ concat(" ").
+ concat(select("a").label()).
+ concat(" ").
+ concat(select("a").inV().values("name")) <4>
+g.inject("hello", "hi").concat(__.V().values('name')) <5>
+g.inject("This").concat(" ").concat("is a ", "gremlin.") <5>
Review Comment:
```suggestion
g.inject("This").concat(" ").concat("is a ", "gremlin.") <6>
```
--
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] xiazcy commented on a diff in pull request #2099: String manipulation function concat()
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1262853967
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/Concat.feature:
##########
@@ -0,0 +1,213 @@
+# 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.
+
+@StepClassMap @StepConcat
+Feature: Step - concat()
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | a |
+ | b |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_XcX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | ac |
+ | bc |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xc_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c", "d")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acd |
+ | bcd |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xinject_c_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat(__.inject("c", "d"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acda |
+ | bcdb |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXListXa_bXcX_concat_XdX
+ Given the empty graph
+ And using the parameter xx1 defined as "l[a,b]"
+ And the traversal of
+ """
+ g.inject(xx1,"c").concat("d")
+ """
+ When iterated to list
+ Then the traversal will raise an error with message containing text of "String concat() can only take string as argument"
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXaX_concat_Xinject_List_b_cX
+ Given the empty graph
+ And using the parameter xx1 defined as "l[b,c]"
+ And the traversal of
+ """
+ g.inject("a").concat(__.inject(xx1))
+ """
+ When iterated to list
+ Then the traversal will raise an error
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXnullX_concat_XinjectX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject(null).concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | null |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXnull_aX_concat_Xnull_bX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject(null, "a").concat(null, "b")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | b |
+ | ab |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXhello_hiX_concat_XV_valuesXnameXX
Review Comment:
> i think my main problem with this is that i can't think of any steps that work that way. no steps allow you to expand a traversal's results as multiple arguments. i'm wary of introducing that, even if it sorta makes sense and is convenient because then you have to know which step does an expansion and which just takes the first value as a single argument.
Ah yes, that is true as well. I will revert to taking the first value.
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/Concat.feature:
##########
@@ -0,0 +1,213 @@
+# 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.
+
+@StepClassMap @StepConcat
+Feature: Step - concat()
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | a |
+ | b |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_XcX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | ac |
+ | bc |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xc_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c", "d")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acd |
+ | bcd |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xinject_c_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat(__.inject("c", "d"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acda |
+ | bcdb |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXListXa_bXcX_concat_XdX
+ Given the empty graph
+ And using the parameter xx1 defined as "l[a,b]"
+ And the traversal of
+ """
+ g.inject(xx1,"c").concat("d")
+ """
+ When iterated to list
+ Then the traversal will raise an error with message containing text of "String concat() can only take string as argument"
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXaX_concat_Xinject_List_b_cX
+ Given the empty graph
+ And using the parameter xx1 defined as "l[b,c]"
+ And the traversal of
+ """
+ g.inject("a").concat(__.inject(xx1))
+ """
+ When iterated to list
+ Then the traversal will raise an error
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXnullX_concat_XinjectX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject(null).concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | null |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXnull_aX_concat_Xnull_bX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject(null, "a").concat(null, "b")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | b |
+ | ab |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXhello_hiX_concat_XV_valuesXnameXX
Review Comment:
> i think my main problem with this is that i can't think of any steps that work that way. no steps allow you to expand a traversal's results as multiple arguments. i'm wary of introducing that, even if it sorta makes sense and is convenient because then you have to know which step does an expansion and which just takes the first value as a single argument.
Ah yes, that is true as well. I will revert to taking the first 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] Cole-Greer commented on a diff in pull request #2099: String manipulation function concat()
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1261629829
##########
docs/src/dev/provider/gremlin-semantics.asciidoc:
##########
@@ -609,6 +609,29 @@ link:https://github.com/apache/tinkerpop/tree/x.y.z/gremlin-core/src/main/java/o
link:https://github.com/apache/tinkerpop/tree/x.y.z/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/service/ServiceRegistry.java[ServiceRegistry],
link:https://tinkerpop.apache.org/docs/x.y.z/reference/#call-step[reference]
+[[concat-step]]
+=== concat()
+
+*Description:* Concatenates the incoming String traverser with the input String arguments, and return the joined String.
+If the incoming traverser is a non-String value then an `IllegalArgumentException` will be thrown.
+
+*Syntax:* `concat()` | `concat(String...)` | `concat(Traversal)`
+
+[width="100%",options="header"]
+|=========================================================
+|Start Step |Mid Step |Modulated |Domain |Range
+|N |Y |N |`Map` |`String`
Review Comment:
I believe this should be:
|N |Y |N |`String` |`String`
--
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] xiazcy commented on a diff in pull request #2099: String manipulation function concat()
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1265744569
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/Concat.feature:
##########
@@ -0,0 +1,213 @@
+# 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.
+
+@StepClassMap @StepConcat
+Feature: Step - concat()
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | a |
+ | b |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_XcX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | ac |
+ | bc |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xc_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c", "d")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acd |
+ | bcd |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xinject_c_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat(__.inject("c", "d"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acda |
Review Comment:
Updated to
```
| result |
| ac |
| bd |
```
(see latest comment).
--
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] xiazcy commented on a diff in pull request #2099: [WIP] TINKERPOP-2672 - string manipulation function concat()
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1231984996
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/Operator.java:
##########
@@ -204,5 +204,5 @@ else if (a instanceof Collection && a instanceof Collection)
public Object apply(final Object a, final Object b) {
return (long) a + (long) b;
}
- }
+ },
Review Comment:
```suggestion
}
```
--
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 #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1256195124
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ConcatStep.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.step.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+
+import java.util.Collections;
+import java.util.Set;
+
+public class ConcatStep<S> extends ScalarMapStep<S, String> implements TraversalParent {
Review Comment:
how do you feel about `final` for this class?
--
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 #2099: String manipulation function concat()
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1261919919
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/util/BytecodeHelper.java:
##########
@@ -196,6 +196,7 @@ public final class BytecodeHelper {
put(GraphTraversal.Symbols.max, Arrays.asList(MaxGlobalStep.class, MaxLocalStep.class));
put(GraphTraversal.Symbols.min, Arrays.asList(MinGlobalStep.class, MinGlobalStep.class));
put(GraphTraversal.Symbols.mean, Arrays.asList(MeanGlobalStep.class, MeanLocalStep.class));
+ put(GraphTraversal.Symbols.concat, Collections.singletonList(ConstantStep.class));
Review Comment:
Typo: ConcatStep
--
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 pull request #2099: String manipulation function concat()
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#issuecomment-1638917727
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a diff in pull request #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1256197647
##########
docs/src/dev/provider/gremlin-semantics.asciidoc:
##########
@@ -609,6 +609,29 @@ link:https://github.com/apache/tinkerpop/tree/x.y.z/gremlin-core/src/main/java/o
link:https://github.com/apache/tinkerpop/tree/x.y.z/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/service/ServiceRegistry.java[ServiceRegistry],
link:https://tinkerpop.apache.org/docs/x.y.z/reference/#call-step[reference]
+[[concat-step]]
Review Comment:
i think this section should address nulls and how they are treated by the step if encountered.
--
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 #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1256197647
##########
docs/src/dev/provider/gremlin-semantics.asciidoc:
##########
@@ -609,6 +609,29 @@ link:https://github.com/apache/tinkerpop/tree/x.y.z/gremlin-core/src/main/java/o
link:https://github.com/apache/tinkerpop/tree/x.y.z/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/service/ServiceRegistry.java[ServiceRegistry],
link:https://tinkerpop.apache.org/docs/x.y.z/reference/#call-step[reference]
+[[concat-step]]
Review Comment:
i think this section should address nulls.
--
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 #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1262603779
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/Concat.feature:
##########
@@ -0,0 +1,213 @@
+# 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.
+
+@StepClassMap @StepConcat
+Feature: Step - concat()
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | a |
+ | b |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_XcX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | ac |
+ | bc |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xc_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c", "d")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acd |
+ | bcd |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xinject_c_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat(__.inject("c", "d"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acda |
+ | bcdb |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXListXa_bXcX_concat_XdX
+ Given the empty graph
+ And using the parameter xx1 defined as "l[a,b]"
+ And the traversal of
+ """
+ g.inject(xx1,"c").concat("d")
+ """
+ When iterated to list
+ Then the traversal will raise an error with message containing text of "String concat() can only take string as argument"
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXaX_concat_Xinject_List_b_cX
+ Given the empty graph
+ And using the parameter xx1 defined as "l[b,c]"
+ And the traversal of
+ """
+ g.inject("a").concat(__.inject(xx1))
+ """
+ When iterated to list
+ Then the traversal will raise an error
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXnullX_concat_XinjectX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject(null).concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | null |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXnull_aX_concat_Xnull_bX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject(null, "a").concat(null, "b")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | b |
+ | ab |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXhello_hiX_concat_XV_valuesXnameXX
Review Comment:
as i mentioned elsewhere, this one feels off to me. i wouldn't expect the traversal given to `concat()` to be fully iterated so:
```
| result |
| hellomarko |
| himarko |
```
--
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 #2099: String manipulation function concat()
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#issuecomment-1638499751
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] Cole-Greer commented on a diff in pull request #2099: String manipulation function concat()
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1261632362
##########
docs/src/dev/provider/gremlin-semantics.asciidoc:
##########
@@ -609,6 +609,29 @@ link:https://github.com/apache/tinkerpop/tree/x.y.z/gremlin-core/src/main/java/o
link:https://github.com/apache/tinkerpop/tree/x.y.z/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/service/ServiceRegistry.java[ServiceRegistry],
link:https://tinkerpop.apache.org/docs/x.y.z/reference/#call-step[reference]
+[[concat-step]]
+=== concat()
+
+*Description:* Concatenates the incoming String traverser with the input String arguments, and return the joined String.
+If the incoming traverser is a non-String value then an `IllegalArgumentException` will be thrown.
+
+*Syntax:* `concat()` | `concat(String...)` | `concat(Traversal)`
+
+[width="100%",options="header"]
+|=========================================================
+|Start Step |Mid Step |Modulated |Domain |Range
+|N |Y |N |`Map` |`String`
+|=========================================================
+
+*Arguments:*
+
+* `String...` - If one or more String values are provided, they will be concatenated with the
+incoming traverser. If no argument is provided, the String value from the incoming traverser is returned.
+* `Traversal` - The `Traversal` value must resolve to a `String`.
+
+See: link:https://github.com/apache/tinkerpop/tree/x.y.z/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ConcatStep.java[source],
+link:https://tinkerpop.apache.org/docs/x.y.z/reference/#concat-step[reference]
+
Review Comment:
Would be good to include a brief *Exceptions* section here similar to the Call step docs. Just a one liner regarding IllegalArgumentException would be good (also any exceptions related to nulls if applicable).
--
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 #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1262841765
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/Concat.feature:
##########
@@ -0,0 +1,213 @@
+# 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.
+
+@StepClassMap @StepConcat
+Feature: Step - concat()
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | a |
+ | b |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_XcX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | ac |
+ | bc |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xc_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c", "d")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acd |
+ | bcd |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xinject_c_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat(__.inject("c", "d"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acda |
+ | bcdb |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXListXa_bXcX_concat_XdX
+ Given the empty graph
+ And using the parameter xx1 defined as "l[a,b]"
+ And the traversal of
+ """
+ g.inject(xx1,"c").concat("d")
+ """
+ When iterated to list
+ Then the traversal will raise an error with message containing text of "String concat() can only take string as argument"
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXaX_concat_Xinject_List_b_cX
+ Given the empty graph
+ And using the parameter xx1 defined as "l[b,c]"
+ And the traversal of
+ """
+ g.inject("a").concat(__.inject(xx1))
+ """
+ When iterated to list
+ Then the traversal will raise an error
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXnullX_concat_XinjectX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject(null).concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | null |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXnull_aX_concat_Xnull_bX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject(null, "a").concat(null, "b")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | b |
+ | ab |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXhello_hiX_concat_XV_valuesXnameXX
Review Comment:
> The reasoning for getting it fully iterated is treating the traversal results like the string inputs.
i think my main problem with this is that i can't think of any steps that work that way. no steps allow you to expand a traversal's results as multiple arguments. i'm wary of introducing that, even if it sorta makes sense and is convenient because then you have to know which step does an expansion and which just takes the first value as a single argument.
--
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] xiazcy commented on a diff in pull request #2099: String manipulation function concat()
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1262812966
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/Concat.feature:
##########
@@ -0,0 +1,213 @@
+# 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.
+
+@StepClassMap @StepConcat
+Feature: Step - concat()
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | a |
+ | b |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_XcX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | ac |
+ | bc |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xc_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c", "d")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acd |
+ | bcd |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xinject_c_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat(__.inject("c", "d"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acda |
Review Comment:
My understanding of the spec is that concat appends the strings in the arguments together and then with the incoming traverser, with each traverser mapping to the results inside the arguments.
I'm using TraversalUtil.applyAll to get the result of the child traversal, and In this case, "a" is appended in front of the "c" and "d" injected. Then for inject, it appends the "a" from the parent to the end. Then the map operation will move to the next incoming traverser "b" then does the same thing:
```
g.inject("a", "b").concat(__.inject("c", "d"))
==>acdb
==>bcdb
```
We thought about getting "ac" and "bd", but weren't sure of how useful it would be, as we'd expect things inside the concat() to be concatenated together. As well given a map operation, if we injected 3 items, then the 3rd one wouldn't be returned since there are only two incoming traversers.
--
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 #2099: [WIP] TINKERPOP-2672 - string manipulation function concat()
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#issuecomment-1594276404
## [Codecov](https://app.codecov.io/gh/apache/tinkerpop/pull/2099?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#2099](https://app.codecov.io/gh/apache/tinkerpop/pull/2099?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8be3cda) into [master](https://app.codecov.io/gh/apache/tinkerpop/commit/f9d784771057dfa299ec0b43d0cadde5e2d95226?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (f9d7847) will **decrease** coverage by `0.04%`.
> The diff coverage is `80.53%`.
```diff
@@ Coverage Diff @@
## master #2099 +/- ##
============================================
- Coverage 74.85% 74.81% -0.04%
- Complexity 11997 12027 +30
============================================
Files 1025 1028 +3
Lines 62034 62147 +113
Branches 6849 6872 +23
============================================
+ Hits 46438 46498 +60
- Misses 13046 13102 +56
+ Partials 2550 2547 -3
```
| [Impacted Files](https://app.codecov.io/gh/apache/tinkerpop/pull/2099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...in/language/grammar/DefaultGremlinBaseVisitor.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9sYW5ndWFnZS9ncmFtbWFyL0RlZmF1bHRHcmVtbGluQmFzZVZpc2l0b3IuamF2YQ==) | `0.27% <0.00%> (-0.01%)` | :arrow_down: |
| [.../tinkerpop/gremlin/process/traversal/Operator.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9PcGVyYXRvci5qYXZh) | `90.24% <ø> (ø)` | |
| [gremlin-go/driver/anonymousTraversal.go](https://app.codecov.io/gh/apache/tinkerpop/pull/2099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1nby9kcml2ZXIvYW5vbnltb3VzVHJhdmVyc2FsLmdv) | `44.97% <0.00%> (-0.40%)` | :arrow_down: |
| [...inkerpop/gremlin/process/ProcessStandardSuite.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi10ZXN0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL1Byb2Nlc3NTdGFuZGFyZFN1aXRlLmphdmE=) | `66.66% <ø> (ø)` | |
| [...kerpop/gremlin/process/traversal/dsl/graph/\_\_.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9kc2wvZ3JhcGgvX18uamF2YQ==) | `77.84% <50.00%> (-0.65%)` | :arrow_down: |
| [...emlin/language/grammar/TraversalMethodVisitor.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9sYW5ndWFnZS9ncmFtbWFyL1RyYXZlcnNhbE1ldGhvZFZpc2l0b3IuamF2YQ==) | `91.20% <57.14%> (-0.80%)` | :arrow_down: |
| [...n/process/traversal/step/map/ConcatGlobalStep.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9zdGVwL21hcC9Db25jYXRHbG9iYWxTdGVwLmphdmE=) | `83.87% <83.87%> (ø)` | |
| [...in/process/traversal/step/map/ConcatLocalStep.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9zdGVwL21hcC9Db25jYXRMb2NhbFN0ZXAuamF2YQ==) | `84.37% <84.37%> (ø)` | |
| [...in/process/traversal/dsl/graph/GraphTraversal.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC9kc2wvZ3JhcGgvR3JhcGhUcmF2ZXJzYWwuamF2YQ==) | `92.32% <100.00%> (-0.55%)` | :arrow_down: |
| [...process/traversal/translator/DotNetTranslator.java](https://app.codecov.io/gh/apache/tinkerpop/pull/2099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JlbWxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS90aW5rZXJwb3AvZ3JlbWxpbi9wcm9jZXNzL3RyYXZlcnNhbC90cmFuc2xhdG9yL0RvdE5ldFRyYW5zbGF0b3IuamF2YQ==) | `75.58% <100.00%> (+0.09%)` | :arrow_up: |
| ... and [3 more](https://app.codecov.io/gh/apache/tinkerpop/pull/2099?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [4 files with indirect coverage changes](https://app.codecov.io/gh/apache/tinkerpop/pull/2099/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=apache)
--
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] xiazcy commented on a diff in pull request #2099: String manipulation function concat()
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1261772926
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ConcatStepTest.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.step.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
+import org.apache.tinkerpop.gremlin.process.traversal.step.StepTest;
+import org.junit.Test;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+
+public class ConcatStepTest extends StepTest {
+
+ @Override
+ protected List<Traversal> getTraversals() {
+ return Collections.singletonList(__.concat("test"));
+ }
+
+ @Test
+ public void testReturnTypes() {
+ assertEquals("abc", __.__("a").concat(__.__("b", "c")).next());
+ assertEquals("abcd", __.__("a").concat("b", "c", "d").next());
+ assertEquals("abcd", __.__("a").concat(__.__(Arrays.asList("b", "c", "d")).unfold()).next());
+ assertEquals("", __.__("").concat("").next());
+
+ assertArrayEquals(new String[]{"a", "b", "c", "d"},
+ __.__("a", "b", "c", "d").concat().toList().toArray());
+ assertArrayEquals(new String[]{"ade", "bde", "cde"},
+ __.__("a", "b", "c").concat("d", "e").toList().toArray());
+ assertArrayEquals(new String[]{"abc", "bbc"},
+ __.__("a", "b").concat(__.__("b", "c")).toList().toArray());
+
+ assertArrayEquals(new String[]{"Mr.a", "Mr.b", "Mr.c", "Mr.d"},
+ __.__("a", "b", "c", "d").as("letters").constant("Mr.").concat(__.select("letters")).toList().toArray());
+
+ String nullStr = null;
Review Comment:
Added additional cases.
--
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] xiazcy commented on a diff in pull request #2099: String manipulation function concat()
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1262812966
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/Concat.feature:
##########
@@ -0,0 +1,213 @@
+# 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.
+
+@StepClassMap @StepConcat
+Feature: Step - concat()
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | a |
+ | b |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_XcX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | ac |
+ | bc |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xc_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c", "d")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acd |
+ | bcd |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xinject_c_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat(__.inject("c", "d"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acda |
Review Comment:
My understanding of the spec is that concat appends the strings in the arguments together and then with the incoming traverser, with each traverser mapping to the results inside the arguments.
I'm using TraversalUtil.applyAll to get the result of the child traversal, and In this case, "a" is appended in front of the "c" and "d" injected. Then for inject itself, it appends the "a" from the parent to the end. Then the map operation will move to the next incoming traverser "b" then does the same thing:
```
g.inject("a", "b").concat(__.inject("c", "d"))
==>acdb
==>bcdb
```
We thought about getting "ac" and "bd", but weren't sure of how useful it would be, as we'd expect things inside the concat() to be concatenated together. As well given a map operation, if we injected 3 items, then the 3rd one wouldn't be returned since there are only two incoming traversers.
--
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] xiazcy commented on a diff in pull request #2099: String manipulation function concat()
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1262812966
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/Concat.feature:
##########
@@ -0,0 +1,213 @@
+# 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.
+
+@StepClassMap @StepConcat
+Feature: Step - concat()
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | a |
+ | b |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_XcX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | ac |
+ | bc |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xc_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c", "d")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acd |
+ | bcd |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xinject_c_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat(__.inject("c", "d"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acda |
Review Comment:
My understanding of the spec is that concat appends the strings in the arguments together and then with the incoming traverser, with each traverser mapping to the results inside the arguments.
I'm using TraversalUtil.applyAll to get the result of the child traversal, and In this case, "a" is appended in front of the "c" and "d" injected. Then for inject itself, it appends the "a" from the parent to the end. Then the map operation will move to the next incoming traverser "b" then does the same thing:
```
g.inject("a", "b").concat(__.inject("c", "d"))
==>acdb
==>bcdb
```
We thought about getting `ac` and `bd`, but weren't sure of how useful it would be, as we'd expect things inside the concat() to be concatenated together. As well given a map operation, if we injected 3 items, then the 3rd one wouldn't be returned since there are only two incoming traversers.
--
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] xiazcy commented on a diff in pull request #2099: String manipulation function concat()
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1262812966
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/Concat.feature:
##########
@@ -0,0 +1,213 @@
+# 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.
+
+@StepClassMap @StepConcat
+Feature: Step - concat()
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | a |
+ | b |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_XcX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | ac |
+ | bc |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xc_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c", "d")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acd |
+ | bcd |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xinject_c_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat(__.inject("c", "d"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acda |
Review Comment:
My understanding of the spec is that concat appends the strings in the arguments together and then with the incoming traverser, with each traverser mapping to the results inside the arguments.
I'm using TraversalUtil.applyAll to get the result of the child traversal, and In this case, "a" is appended in front of the "c" and "d" injected. Then for inject, it appends the "a" from the parent to the end. Then the map operation will move to the next incoming traverser "b" then does the same thing:
```
g.inject("a", "b").concat(__.inject("c", "d"))
==>acdb
==>bcdb
```
We thought about getting "ac" and "bd", but I wasn't sure if it'd be a scalarMap, since we'll have to keep track of matching traverser from the child traversal to the next incoming traverser for map.
--
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 #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1256193441
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ConcatStep.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.step.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+
+import java.util.Collections;
+import java.util.Set;
+
+public class ConcatStep<S> extends ScalarMapStep<S, String> implements TraversalParent {
Review Comment:
some basic javadoc for the class would be nice
--
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 #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1256185057
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/DefaultGremlinBaseVisitor.java:
##########
@@ -911,6 +911,20 @@ protected void notImplemented(final ParseTree ctx) {
* {@inheritDoc}
*/
@Override public T visitTraversalMethod_call_string_map_traversal(final GremlinParser.TraversalMethod_call_string_map_traversalContext ctx) { notImplemented(ctx); return null; }
+ /**
+ * {@inheritDoc}
+ */
+ @Override
Review Comment:
please single line these like the others?
--
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 #2099: String manipulation function concat()
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1261644151
##########
docs/src/reference/the-traversal.asciidoc:
##########
@@ -1049,6 +1049,41 @@ g.V().coin(1.0)
link:++https://tinkerpop.apache.org/javadocs/x.y.z/core/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.html#coin-double-++[`coin(double)`]
+[[concat-step]]
+=== Concat Step
+
+The `concat()`-step (*map*) concatenates one or more String values together to the incoming String traverser.
+If the incoming traverser is a non-String value then an `IllegalArgumentException` will be thrown.
+
+[gremlin-groovy,modern]
+----
+g.addV(constant("prefix_").concat(__.V(1).label())).property(id, 10) <1>
+g.V(10).label()
+g.V().hasLabel('person').values('name').as('a').
+ constant('Mr.').concat(__.select('a')) <2>
+g.V().hasLabel('software').as('a').values('name').
+ concat(' uses ').
+ concat(select('a').values('lang')) <3>
+g.V(1).outE().as("a").constant("").
+ concat(V(1).values("name")).
+ concat(" ").
+ concat(select("a").label()).
+ concat(" ").
+ concat(select("a").inV().values("name")) <4>
+g.inject("This").concat(" ").concat("is a ", "gremlin.") <5>
+----
+
+<1> Add a new vertex with id 10 which should be labeled like an existing vertex but with some prefix attached
+<2> Attach the prefix "Mr." to all the names using the `constant()`-step
+<3> Generate a string of software names and the language they use
+<4> Generate a string description for each of marko's outgoing edges
+<5> A generic use of `concat()` to join strings together
+
+*Additional References*
+
+link:++https://tinkerpop.apache.org/javadocs/x.y.z/core/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.html#concat-String++[`concat(String...)`]
+link:++https://tinkerpop.apache.org/javadocs/x.y.z/core/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.html#concat-Traversal++[`concat(Traversal)`]
Review Comment:
Consider adding link to semantics docs.
link:++https://tinkerpop.apache.org/docs/x.y.z/dev/provider/#concat-step++[`Semantics`]
--
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] xiazcy commented on a diff in pull request #2099: String manipulation function concat()
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1261774592
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/ProcessStandardSuite.java:
##########
@@ -144,6 +145,7 @@ public class ProcessStandardSuite extends AbstractGremlinSuite {
AddEdgeTest.Traversals.class,
AddVertexTest.Traversals.class,
CoalesceTest.Traversals.class,
+ ConcatTest.Traversals.class,
Review Comment:
I would agree for string functions there is no need to keep the duality. I will remove it 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] xiazcy merged pull request #2099: String manipulation function concat()
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy merged PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099
--
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 #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1256209925
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/graph-traversal.js:
##########
@@ -543,6 +543,16 @@ class GraphTraversal extends Traversal {
return this;
}
+ /**
Review Comment:
i don't believe this is the only change required for javascript. you need the `statics` method definition as well
--
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 #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1266934156
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ConcatStep.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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.step.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent;
+import org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.InjectStep;
+import org.apache.tinkerpop.gremlin.process.traversal.traverser.TraverserRequirement;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+
+import java.util.Collections;
+import java.util.Set;
+
+/**
+ * Reference implementation for String concatenation step, a mid-traversal step which concatenates one or more
+ * String values together to the incoming String traverser. If the incoming traverser is a non-String value then an
+ * `IllegalArgumentException` will be thrown.
Review Comment:
nit: `{@code IllegalArgumentException}`
--
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] xiazcy commented on pull request #2099: String manipulation function concat()
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#issuecomment-1640995237
Thanks for everyone's review and input! If there are no additional comments, I'll merge this end of day today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] vkagamlyk commented on a diff in pull request #2099: String manipulation function concat()
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1261786554
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -501,6 +501,28 @@ private GraphTraversal(ICollection<ITraversalStrategy> traversalStrategies, Byte
return Wrap<TStart, TEnd>(this);
}
+ /// <summary>
+ /// Adds the concat step to this <see cref="GraphTraversal{SType, EType}" />.
+ /// </summary>
+ public GraphTraversal<TStart, TEnd> Concat<TNewEnd> (ITraversal concatTraversal)
+ {
+ Bytecode.AddStep("concat", concatTraversal);
+ return Wrap<TStart, TEnd>(this);
+ }
+
+ /// <summary>
+ /// Adds the concat step to this <see cref="GraphTraversal{SType, EType}" />.
+ /// </summary>
+ public GraphTraversal<TStart, TEnd> Concat<TNewEnd> (params string?[] concatStrings)
+ {
+ // need null check?
Review Comment:
yes
--
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 #2099: String manipulation function concat()
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1261786554
##########
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/GraphTraversal.cs:
##########
@@ -501,6 +501,28 @@ private GraphTraversal(ICollection<ITraversalStrategy> traversalStrategies, Byte
return Wrap<TStart, TEnd>(this);
}
+ /// <summary>
+ /// Adds the concat step to this <see cref="GraphTraversal{SType, EType}" />.
+ /// </summary>
+ public GraphTraversal<TStart, TEnd> Concat<TNewEnd> (ITraversal concatTraversal)
+ {
+ Bytecode.AddStep("concat", concatTraversal);
+ return Wrap<TStart, TEnd>(this);
+ }
+
+ /// <summary>
+ /// Adds the concat step to this <see cref="GraphTraversal{SType, EType}" />.
+ /// </summary>
+ public GraphTraversal<TStart, TEnd> Concat<TNewEnd> (params string?[] concatStrings)
+ {
+ // need null check?
Review Comment:
yes
--
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 #2099: String manipulation function concat()
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1261802158
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/Concat.feature:
##########
@@ -0,0 +1,213 @@
+# 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.
+
+@StepClassMap @StepConcat
+Feature: Step - concat()
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | a |
+ | b |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_XcX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | ac |
+ | bc |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xc_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c", "d")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acd |
+ | bcd |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xinject_c_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat(__.inject("c", "d"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acda |
Review Comment:
@spmallette what is your opinion on this result?
--
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] xiazcy commented on a diff in pull request #2099: String manipulation function concat()
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1262822027
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/Concat.feature:
##########
@@ -0,0 +1,213 @@
+# 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.
+
+@StepClassMap @StepConcat
+Feature: Step - concat()
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | a |
+ | b |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_XcX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | ac |
+ | bc |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xc_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c", "d")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acd |
+ | bcd |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xinject_c_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat(__.inject("c", "d"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acda |
+ | bcdb |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXListXa_bXcX_concat_XdX
+ Given the empty graph
+ And using the parameter xx1 defined as "l[a,b]"
+ And the traversal of
+ """
+ g.inject(xx1,"c").concat("d")
+ """
+ When iterated to list
+ Then the traversal will raise an error with message containing text of "String concat() can only take string as argument"
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXaX_concat_Xinject_List_b_cX
+ Given the empty graph
+ And using the parameter xx1 defined as "l[b,c]"
+ And the traversal of
+ """
+ g.inject("a").concat(__.inject(xx1))
+ """
+ When iterated to list
+ Then the traversal will raise an error
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXnullX_concat_XinjectX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject(null).concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | null |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXnull_aX_concat_Xnull_bX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject(null, "a").concat(null, "b")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | b |
+ | ab |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXhello_hiX_concat_XV_valuesXnameXX
Review Comment:
So this was a weird one, I was going back and forth between fulling iterated or not. The reasoning for getting it fully iterated is treating the traversal results like the string inputs. Since we concatenate all string inputs inside concat, I'd think we should iterate the whole traversal and append them?
Yea, I thought about `concat(Traversal...)`, but ultimately decided to follow the spec. It would provide more flexibility, I can see to implement 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 a diff in pull request #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1256185057
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/grammar/DefaultGremlinBaseVisitor.java:
##########
@@ -911,6 +911,20 @@ protected void notImplemented(final ParseTree ctx) {
* {@inheritDoc}
*/
@Override public T visitTraversalMethod_call_string_map_traversal(final GremlinParser.TraversalMethod_call_string_map_traversalContext ctx) { notImplemented(ctx); return null; }
+ /**
+ * {@inheritDoc}
+ */
+ @Override
Review Comment:
please single line these like the others? also note `final` designations.
--
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 #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1261835381
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/Concat.feature:
##########
@@ -0,0 +1,213 @@
+# 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.
+
+@StepClassMap @StepConcat
+Feature: Step - concat()
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | a |
+ | b |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_XcX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | ac |
+ | bc |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xc_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c", "d")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acd |
+ | bcd |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xinject_c_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat(__.inject("c", "d"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acda |
Review Comment:
not what i would have expected. i think i would have expected:
```
| result |
| ac |
| bc|
```
[looking at other tests](https://github.com/apache/tinkerpop/pull/2099/files/86238327a61b7246aa182b86d7b65c135513929f#diff-3cc4e477a58382ce059167e0cede86838765da192b3b734705e68c5d1364eaceR121), it appears the behavior is to consume the whole traversal rather than `next()`. what was the thinking there? that's sorta different that what child traversals do in Gremlin.
i guess that still doesn't explain the added "a" and "b" at the end of each result. i'm trying to understand why it does 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] spmallette commented on a diff in pull request #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1262603779
##########
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/Concat.feature:
##########
@@ -0,0 +1,213 @@
+# 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.
+
+@StepClassMap @StepConcat
+Feature: Step - concat()
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | a |
+ | b |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_XcX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | ac |
+ | bc |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xc_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat("c", "d")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acd |
+ | bcd |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXa_bX_concat_Xinject_c_dX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject("a", "b").concat(__.inject("c", "d"))
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | acda |
+ | bcdb |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXListXa_bXcX_concat_XdX
+ Given the empty graph
+ And using the parameter xx1 defined as "l[a,b]"
+ And the traversal of
+ """
+ g.inject(xx1,"c").concat("d")
+ """
+ When iterated to list
+ Then the traversal will raise an error with message containing text of "String concat() can only take string as argument"
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXaX_concat_Xinject_List_b_cX
+ Given the empty graph
+ And using the parameter xx1 defined as "l[b,c]"
+ And the traversal of
+ """
+ g.inject("a").concat(__.inject(xx1))
+ """
+ When iterated to list
+ Then the traversal will raise an error
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXnullX_concat_XinjectX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject(null).concat()
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | null |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXnull_aX_concat_Xnull_bX
+ Given the empty graph
+ And the traversal of
+ """
+ g.inject(null, "a").concat(null, "b")
+ """
+ When iterated to list
+ Then the result should be unordered
+ | result |
+ | b |
+ | ab |
+
+ @GraphComputerVerificationInjectionNotSupported
+ Scenario: g_injectXhello_hiX_concat_XV_valuesXnameXX
Review Comment:
as i mentioned elsewhere, this one feels off to me. i wouldn't expect the traversal given to `concat()` to be fully iterated so:
```
| result |
| hellomarko |
| himarko |
```
also, should `concat(Traversal)` be `concat(Traversal...)` to match the `String` form? this way you could `concat(select('a'), select('b'), select('c'))` which would match its `String` overload?
--
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 #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1256219455
##########
gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/ProcessStandardSuite.java:
##########
@@ -144,6 +145,7 @@ public class ProcessStandardSuite extends AbstractGremlinSuite {
AddEdgeTest.Traversals.class,
AddVertexTest.Traversals.class,
CoalesceTest.Traversals.class,
+ ConcatTest.Traversals.class,
Review Comment:
i think TinkerPop needs to start pressing on providers to get off of the old java process tests. its time consuming to maintain both these and the gherkin tests as duplicates. not sure if it's good to start this path where all the new string functions that follow will need java and gherkin tests. 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 #2099: String manipulation function concat()
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#discussion_r1256201346
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ConcatStepTest.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.step.map;
+
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
+import org.apache.tinkerpop.gremlin.process.traversal.step.StepTest;
+import org.junit.Test;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+
+public class ConcatStepTest extends StepTest {
+
+ @Override
+ protected List<Traversal> getTraversals() {
+ return Collections.singletonList(__.concat("test"));
+ }
+
+ @Test
+ public void testReturnTypes() {
+ assertEquals("abc", __.__("a").concat(__.__("b", "c")).next());
+ assertEquals("abcd", __.__("a").concat("b", "c", "d").next());
+ assertEquals("abcd", __.__("a").concat(__.__(Arrays.asList("b", "c", "d")).unfold()).next());
+ assertEquals("", __.__("").concat("").next());
+
+ assertArrayEquals(new String[]{"a", "b", "c", "d"},
+ __.__("a", "b", "c", "d").concat().toList().toArray());
+ assertArrayEquals(new String[]{"ade", "bde", "cde"},
+ __.__("a", "b", "c").concat("d", "e").toList().toArray());
+ assertArrayEquals(new String[]{"abc", "bbc"},
+ __.__("a", "b").concat(__.__("b", "c")).toList().toArray());
+
+ assertArrayEquals(new String[]{"Mr.a", "Mr.b", "Mr.c", "Mr.d"},
+ __.__("a", "b", "c", "d").as("letters").constant("Mr.").concat(__.select("letters")).toList().toArray());
+
+ String nullStr = null;
Review Comment:
is this sufficient testing for null? what if one value is null and the rest of the stream is strings for example?
--
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] xiazcy commented on pull request #2099: String manipulation function concat()
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#issuecomment-1636280210
I believe I've addressed all existing comments so far.
Concat with inject is now updated to the expected behaviour:
```
gremlin> g.inject('a', 'b', 'c').concat(__.inject('1', '2'))
==>a1
==>b2
==>c
gremlin> g.inject('a', 'b').concat(__.inject('1', '2', '3'))
==>a1
==>b2
```
Similarly for child traversals, reverted to iterating only the first result:
```
gremlin> g.inject('hello', 'hi').concat(__.V().values('name'))
==>hellomarko
==>himarko
```
> also, should concat(Traversal) be concat(Traversal...) to match the String form? this way you could concat(select('a'), select('b'), select('c')) which would match its String overload?
After some thought around this, I think maybe we should not implement `concat(Traversal…)` at this point, to lessen the complexity of `concat()`.
It’s also currently achievable by chaining concat():
```
gremlin> g.V().as('a').out().as('b').out().as('c').constant("").concat(select("a").label()).concat(select("b").label()).concat(select("c").label())
==>personpersonsoftware
==>personpersonsoftware
```
However, I do want to explore this avenue a bit, so if it leads somewhere I will update here or on the dev list.
--
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 pull request #2099: String manipulation function concat()
Posted by "kenhuuu (via GitHub)" <gi...@apache.org>.
kenhuuu commented on PR #2099:
URL: https://github.com/apache/tinkerpop/pull/2099#issuecomment-1638697593
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org