[
https://issues.apache.org/jira/browse/TINKERPOP-2334?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17779183#comment-17779183
]
ASF GitHub Bot commented on TINKERPOP-2334:
-------------------------------------------
Cole-Greer commented on code in PR #2307:
URL: https://github.com/apache/tinkerpop/pull/2307#discussion_r1370523821
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/FormatStep.java:
##########
@@ -0,0 +1,195 @@
+/*
+ * 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.commons.lang3.StringUtils;
+import org.apache.tinkerpop.gremlin.process.traversal.Pop;
+import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
+import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
+import org.apache.tinkerpop.gremlin.process.traversal.step.PathProcessor;
+import org.apache.tinkerpop.gremlin.process.traversal.step.Scoping;
+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.traverser.util.EmptyTraverser;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalProduct;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalRing;
+import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalUtil;
+import org.apache.tinkerpop.gremlin.structure.Element;
+import org.apache.tinkerpop.gremlin.structure.Property;
+import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
+
+import java.util.HashMap;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Reference implementation for String format step, a mid-traversal step which
will handle result formatting
+ * to string values. If the incoming traverser is a non-String value then an
{@code IllegalArgumentException}
+ * will be thrown.
+ *
+ * @author Valentyn Kahamlyk
+ */
+public final class FormatStep<S> extends MapStep<S, String> implements
TraversalParent, Scoping, PathProcessor {
+
+ private String format;
+ private Set<String> variables;
+ private TraversalRing<S, String> traversalRing = new TraversalRing<>();
+ private Set<String> keepLabels;
+
+ public FormatStep(final Traversal.Admin traversal, final String format) {
+ super(traversal);
+ this.format = format;
+ this.variables = getVariables();
+ }
+
+ @Override
+ protected Traverser.Admin<String> processNextStart() {
+ final Map<String, Object> values = new HashMap<>();
+
+ final Traverser.Admin traverser = this.starts.next();
+
+ boolean productive = true;
+ for (final String var : variables) {
+ final Object current = traverser.get();
+ // try to get property value
+ if (current instanceof Element) {
+ final Property prop = ((Element) current).property(var);
+ if (prop != null && prop.isPresent()) {
+ values.put(var, prop.value());
+ continue;
+ }
+ }
+
+ final TraversalProduct product =
+ TraversalUtil.produce((S)
this.getNullableScopeValue(Pop.last, var, traverser),
this.traversalRing.next());
+
+ if (!product.isProductive() || product.get() == null) {
+ productive = false;
+ break;
+ }
+
+ values.put(var, product.get());
+ }
+ this.traversalRing.reset();
+
+ return productive ?
+
PathProcessor.processTraverserPathLabels(traverser.split(evaluate(values),
this), this.keepLabels) :
+ EmptyTraverser.instance();
+ }
+
+ @Override
+ public String toString() {
+ return StringFactory.stepString(this, this.format, this.traversalRing);
+ }
+
+ @Override
+ public int hashCode() {
+ int result = super.hashCode();
+ return Objects.hash(result, format, traversalRing);
+ }
+
+ @Override
+ public List<Traversal.Admin<S, String>> getLocalChildren() {
+ return this.traversalRing.getTraversals();
+ }
+
+ @Override
+ public void reset() {
+ super.reset();
+ this.traversalRing.reset();
+ }
+
+ @Override
+ public FormatStep<S> clone() {
+ final FormatStep<S> clone = (FormatStep<S>) super.clone();
+ clone.format = this.format;
+ clone.variables = this.variables;
+ clone.traversalRing = this.traversalRing;
+ return clone;
+ }
+
+ @Override
+ public void setTraversal(final Traversal.Admin<?, ?> parentTraversal) {
+ super.setTraversal(parentTraversal);
+ this.traversalRing.getTraversals().forEach(this::integrateChild);
+ }
+
+ @Override
+ public Set<TraverserRequirement> getRequirements() {
+ return this.getSelfAndChildRequirements(TraverserRequirement.OBJECT,
TraverserRequirement.SIDE_EFFECTS);
+ }
+
+ @Override
+ public Set<String> getScopeKeys() {
+ return variables;
+ }
+
+ @Override
+ public void setKeepLabels(final Set<String> labels) {
+ this.keepLabels = labels;
+ }
+
+ @Override
+ public Set<String> getKeepLabels() {
+ return this.keepLabels;
+ }
+
+ // private methods
+
+ private static final Pattern VARIABLE_PATTERN =
Pattern.compile("%\\{(.*?)\\}");
+
+ Set<String> getVariables() {
+ final Matcher matcher = VARIABLE_PATTERN.matcher(format);
+ final Set<String> variables = new LinkedHashSet<>();
+ while (matcher.find()) {
+ final String varName = matcher.group(1);
+ if (!StringUtils.isEmpty(varName)) {
+ variables.add(matcher.group(1));
+ }
+ }
+ return variables;
+ }
+
+ private String evaluate(final Map<String, Object> values) {
+ int lastIndex = 0;
+ final StringBuilder output = new StringBuilder();
+ final Matcher matcher = VARIABLE_PATTERN.matcher(format);
+ matcher.reset();
+
+ while (matcher.find()) {
+ final String varName = matcher.group(1);
+ if (StringUtils.isEmpty(varName)) continue;
Review Comment:
I think an empty string should be a valid varName based on this:
```
gremlin> g.inject("test").as("").V(1).select("")
==>test
```
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/FormatStepTest.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import static org.apache.tinkerpop.gremlin.util.CollectionUtil.asMap;
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class FormatStepTest extends StepTest {
+
+ @Override
+ protected List<Traversal> getTraversals() {
+ return Collections.singletonList(
+ __.format("hello")
+ );
+ }
+
+ private List<String> getVariables(final String format) {
+ final FormatStep formatStep = new
FormatStep(__.inject("test").asAdmin(), format);
+ return new ArrayList<>(formatStep.getScopeKeys());
+ }
+
+ @Test
+ public void shouldGetVariablesFromTemplate() {
+ assertEquals(Collections.emptyList(), getVariables("Hello world"));
+ assertEquals(Collections.emptyList(), getVariables("Hello %{world"));
+ assertEquals(Collections.emptyList(), getVariables("Hello %{}"));
+ assertEquals(Collections.emptyList(), getVariables("Hello {world}"));
+ assertEquals(Collections.emptyList(), getVariables("Hello % {world}"));
+ assertEquals(Collections.singletonList(" "), getVariables("Hello %{
}"));
+ assertEquals(Collections.singletonList("world"), getVariables("Hello
%{world}"));
+ assertEquals(Arrays.asList("Hello", "world"), getVariables("%{Hello}
%{world}"));
+ assertEquals(Arrays.asList("Hello", "world"), getVariables("%{Hello}
%{Hello} %{world}"));
+ assertEquals(Arrays.asList("Hello", "hello", "world"),
getVariables("%{Hello} %{hello} %{world}"));
+ }
+
+ @Test
+ public void shouldWorkWithoutVariables() {
+ assertEquals("Hello world", __.__("test").format("Hello
world").next());
+ }
+
+ @Test
+ public void shouldWorkWithVertexInput() {
+ final VertexProperty mockProperty = mock(VertexProperty.class);
+ when(mockProperty.key()).thenReturn("name");
+ when(mockProperty.value()).thenReturn("Stephen");
+ when(mockProperty.isPresent()).thenReturn(true);
+
+ final Vertex mockVertex = mock(Vertex.class);
+ when(mockVertex.property("name")).thenReturn(mockProperty);
+
+ assertEquals("Hello Stephen", __.__(mockVertex).format("Hello
%{name}").next());
+ }
+
+ @Test
+ public void shouldWorkWithMultipleVertexInput() {
+ final VertexProperty mockProperty1 = mock(VertexProperty.class);
+ when(mockProperty1.key()).thenReturn("name");
+ when(mockProperty1.value()).thenReturn("Stephen");
+ when(mockProperty1.isPresent()).thenReturn(true);
+
+ final Vertex mockVertex1 = mock(Vertex.class);
+ when(mockVertex1.property("name")).thenReturn(mockProperty1);
+
+ final VertexProperty mockProperty2 = mock(VertexProperty.class);
+ when(mockProperty2.key()).thenReturn("name");
+ when(mockProperty2.value()).thenReturn("Marko");
+ when(mockProperty2.isPresent()).thenReturn(true);
+
+ final Vertex mockVertex2 = mock(Vertex.class);
+ when(mockVertex2.property("name")).thenReturn(mockProperty2);
+
+ assertEquals(Arrays.asList("Hello Stephen", "Hello Marko"),
+ __.__(mockVertex1, mockVertex2).format("Hello
%{name}").toList());
+ }
+ @Test
+ public void shouldWorkWithMap() {
+ assertEquals("Hello 2", __.__(asMap("name", 2)).format("Hello
%{name}").next());
+ assertEquals("Hello Stephen", __.__(asMap("name",
"Stephen")).format("Hello %{name}").next());
+ }
+
+ @Test
+ public void shouldWorkWithScopeVariables() {
+ assertEquals("Hello Stephen",
__.__("Stephen").as("name").format("Hello %{name}").next());
+ }
+
+ @Test
+ public void shouldHandleSameVariableTwice() {
+ assertEquals("Hello, Hello Stephen",
+ __.__("Hello").as("action").format("%{action}, %{action}
Stephen").next());
+ }
+
+ @Test
+ public void shouldWorkWithMixedInput() {
+ final VertexProperty mockProperty1 = mock(VertexProperty.class);
+ when(mockProperty1.key()).thenReturn("p1");
+ when(mockProperty1.value()).thenReturn("val1");
+ when(mockProperty1.isPresent()).thenReturn(true);
+
+ final VertexProperty mockProperty2 = mock(VertexProperty.class);
+ when(mockProperty2.key()).thenReturn("p2");
+ when(mockProperty2.value()).thenReturn("val2");
+ when(mockProperty2.isPresent()).thenReturn(true);
+
+ final Vertex mockVertex = mock(Vertex.class);
+ when(mockVertex.property("p1")).thenReturn(mockProperty1);
+ when(mockVertex.property("p2")).thenReturn(mockProperty2);
+
+ assertEquals("val1 val2 valA valB",
+ __.inject("valA").as("varA").
+ constant("valB").as("varB").
+ constant(mockVertex).format("%{p1} %{p2} %{varA}
%{varB}").next());
+ }
Review Comment:
I'm a bit concerned about namespace collisions between property keys and
scope variables. Could you add a test which has a name collision? Based on the
semantics docs, I would expect the following:
```
gremlin> g.inject("stephen").as("name").V(1).format("Hello %{name}")
==>Hello marko
```
##########
gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/FormatStepTest.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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.apache.tinkerpop.gremlin.structure.Vertex;
+import org.apache.tinkerpop.gremlin.structure.VertexProperty;
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import static org.apache.tinkerpop.gremlin.util.CollectionUtil.asMap;
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class FormatStepTest extends StepTest {
+
+ @Override
+ protected List<Traversal> getTraversals() {
+ return Collections.singletonList(
+ __.format("hello")
+ );
+ }
+
+ private List<String> getVariables(final String format) {
+ final FormatStep formatStep = new
FormatStep(__.inject("test").asAdmin(), format);
+ return new ArrayList<>(formatStep.getScopeKeys());
+ }
+
+ @Test
+ public void shouldGetVariablesFromTemplate() {
+ assertEquals(Collections.emptyList(), getVariables("Hello world"));
+ assertEquals(Collections.emptyList(), getVariables("Hello %{world"));
+ assertEquals(Collections.emptyList(), getVariables("Hello %{}"));
+ assertEquals(Collections.emptyList(), getVariables("Hello {world}"));
+ assertEquals(Collections.emptyList(), getVariables("Hello % {world}"));
+ assertEquals(Collections.singletonList(" "), getVariables("Hello %{
}"));
+ assertEquals(Collections.singletonList("world"), getVariables("Hello
%{world}"));
+ assertEquals(Arrays.asList("Hello", "world"), getVariables("%{Hello}
%{world}"));
+ assertEquals(Arrays.asList("Hello", "world"), getVariables("%{Hello}
%{Hello} %{world}"));
+ assertEquals(Arrays.asList("Hello", "hello", "world"),
getVariables("%{Hello} %{hello} %{world}"));
+ }
+
+ @Test
+ public void shouldWorkWithoutVariables() {
+ assertEquals("Hello world", __.__("test").format("Hello
world").next());
+ }
+
+ @Test
+ public void shouldWorkWithVertexInput() {
+ final VertexProperty mockProperty = mock(VertexProperty.class);
+ when(mockProperty.key()).thenReturn("name");
+ when(mockProperty.value()).thenReturn("Stephen");
+ when(mockProperty.isPresent()).thenReturn(true);
+
+ final Vertex mockVertex = mock(Vertex.class);
+ when(mockVertex.property("name")).thenReturn(mockProperty);
+
+ assertEquals("Hello Stephen", __.__(mockVertex).format("Hello
%{name}").next());
+ }
+
+ @Test
+ public void shouldWorkWithMultipleVertexInput() {
+ final VertexProperty mockProperty1 = mock(VertexProperty.class);
+ when(mockProperty1.key()).thenReturn("name");
+ when(mockProperty1.value()).thenReturn("Stephen");
+ when(mockProperty1.isPresent()).thenReturn(true);
+
+ final Vertex mockVertex1 = mock(Vertex.class);
+ when(mockVertex1.property("name")).thenReturn(mockProperty1);
+
+ final VertexProperty mockProperty2 = mock(VertexProperty.class);
+ when(mockProperty2.key()).thenReturn("name");
+ when(mockProperty2.value()).thenReturn("Marko");
+ when(mockProperty2.isPresent()).thenReturn(true);
+
+ final Vertex mockVertex2 = mock(Vertex.class);
+ when(mockVertex2.property("name")).thenReturn(mockProperty2);
+
+ assertEquals(Arrays.asList("Hello Stephen", "Hello Marko"),
+ __.__(mockVertex1, mockVertex2).format("Hello
%{name}").toList());
+ }
+ @Test
+ public void shouldWorkWithMap() {
+ assertEquals("Hello 2", __.__(asMap("name", 2)).format("Hello
%{name}").next());
+ assertEquals("Hello Stephen", __.__(asMap("name",
"Stephen")).format("Hello %{name}").next());
+ }
+
+ @Test
+ public void shouldWorkWithScopeVariables() {
+ assertEquals("Hello Stephen",
__.__("Stephen").as("name").format("Hello %{name}").next());
+ }
+
+ @Test
+ public void shouldHandleSameVariableTwice() {
+ assertEquals("Hello, Hello Stephen",
+ __.__("Hello").as("action").format("%{action}, %{action}
Stephen").next());
+ }
+
Review Comment:
Could you add a test for undefined variables being filtered out?
```
gremlin> g.V(1).format("%{name}'s birthday is %{birthday}!")
//No results
```
##########
docs/src/dev/provider/gremlin-semantics.asciidoc:
##########
@@ -892,6 +892,36 @@ None
None
+[[format-step]]
+=== format()
+
+*Description:* a mid-traversal step which will handle result formatting to
string values.
+
+*Syntax:* `format(String)`
+
+[width="100%",options="header"]
+|=========================================================
+|Start Step |Mid Step |Modulated |Domain |Range
+|N |Y |N |`any` |`String`
+|=========================================================
+
+*Arguments:*
+
+Format string. Variables can be represented using `%{variable_name}` notation.
+The variable values are used in the order that the first one will be found:
Element properties, than Scope values.
+If value for some variable was not found than result for input is skipped.
Review Comment:
I prefer `filtered` over `skipped` here. To me, skipped is ambiguous if it
means the whole string result is skipped, or if that single variable is left
blank in the result.
```suggestion
The variable values are used in the order that the first one will be found:
Element properties, then Scope values.
If value for some variable was not found, then the result is filtered out.
```
> Add format() step
> -----------------
>
> Key: TINKERPOP-2334
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2334
> Project: TinkerPop
> Issue Type: Improvement
> Components: process
> Affects Versions: 3.4.4
> Reporter: Stephen Mallette
> Assignee: Valentyn Kahamlyk
> Priority: Major
>
> Provide for a {{format()}} step which will handle result formatting to string
> values. This change will help resolve the need for string concatenation
> functions while providing a lot of flexibility to how results can be formed:
> {code}
> gremlin> g.V().hasLabel('person').format("%{n} is %{a} years old.").by('n',
> 'name').by('a', 'age')
> ==>marko is 29 years old.
> ==>vadas is 27 years old.
> ==>josh is 32 years old.
> ==>peter is 35 years old.
> gremlin> g.V().hasLabel('person').format("%{name} is %{age} years old.")
> ==>marko is 29 years old.
> ==>vadas is 27 years old.
> ==>josh is 32 years old.
> ==>peter is 35 years old.
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
