This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 82a78fcf744ad4dc971ab7817ffd233fc00206fb Merge: b355b6d002 b2fb2b2465 Author: Stephen Mallette <[email protected]> AuthorDate: Fri Jul 7 13:37:56 2023 -0400 Merge branch '3.5-dev' into 3.6-dev CHANGELOG.asciidoc | 5 +++-- .../process/traversal/lambda/ValueTraversal.java | 7 ++++++- .../traversal/step/map/PropertyMapStep.java | 8 ++++---- .../optimization/ProductiveByStrategy.java | 24 +++++++++++++++++----- .../structure/io/graphson/GraphSONModule.java | 1 + .../decoration/SubgraphStrategyProcessTest.java | 23 +++++++++++++++++++++ 6 files changed, 56 insertions(+), 12 deletions(-) diff --cc CHANGELOG.asciidoc index 58ec3f2452,07f4ecb222..1a0eb72529 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@@ -359,12 -23,13 +359,13 @@@ image::https://raw.githubusercontent.co [[release-3-5-7]] === TinkerPop 3.5.7 (Release Date: NOT OFFICIALLY RELEASED YET) -* Fixed a memory leak in the Gremlin.Net driver that only occurred if a CancellationToken was provided. +* Fixed a memory leak in the Gremlin.Net driver that only occurred if a `CancellationToken` was provided. * Fixed gremlin-python `Client` problem where calling `submit()` after` `close()` would hang the system. * Added `gremlin.spark.dontDeleteNonEmptyOutput` to stop deleting the output folder if it is not empty in `spark-gremlin`. + * Fixed a bug in `SubgraphStrategy` where the vertex property filter produced errors if a `Vertex` was missing the key provided to `by()` as a token. * Upgraded `gremlin-javascript` and `gremlint` to Node 16.20.0. * Upgraded `gremlin-go` to Go 1.20. - * Improved the python `Translator` class. + * Improved the python `Translator` class with better handling for `P`, `None` and subclasses of `str`. * Added `gremlin-java8.bat` file as a workaround to allow loading the console using Java 8 on Windows. * Fixed a bug in `gremlin-server` where timeout tasks were not cancelled and could cause very large memory usage when timeout is large. * Removed `jcabi-manifests` dependency from `gremlin-core`, `gremlin-driver`, and `gremlin-server`. diff --cc gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java index f089636572,16d21935e7..812f13b4d2 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertyMapStep.java @@@ -180,83 -211,7 +184,79 @@@ public class PropertyMapStep<K,E> exten return this.tokens; } - private boolean includeToken(final int token) { + protected boolean includeToken(final int token) { return 0 != (this.tokens & token); } + + protected void addIncludedOptions(Element element, Map<Object, Object> map){ + if (this.returnType == PropertyType.VALUE) { + if (includeToken(WithOptions.ids)) map.put(T.id, getElementId(element)); + if (element instanceof VertexProperty) { + + if (includeToken(WithOptions.keys)) map.put(T.key, getVertexPropertyKey((VertexProperty<?>) element)); + if (includeToken(WithOptions.values)) map.put(T.value, getVertexPropertyValue((VertexProperty<?>) element)); + } else { + if (includeToken(WithOptions.labels)) map.put(T.label, getElementLabel(element)); + } + } + } + + protected Object getElementId(Element element){ + return element.id(); + } + + protected String getElementLabel(Element element){ + return element.label(); + } + + protected String getVertexPropertyKey(VertexProperty<?> vertexProperty){ + return vertexProperty.key(); + } + + protected Object getVertexPropertyValue(VertexProperty<?> vertexProperty){ + return vertexProperty.value(); + } + + protected void addElementProperties(final Traverser.Admin<Element> traverser, Map<Object, Object> map){ + final Element element = traverser.get(); + final boolean isVertex = element instanceof Vertex; + + final Iterator<? extends Property> properties = null == this.propertyTraversal ? + element.properties(this.propertyKeys) : + TraversalUtil.applyAll(traverser, this.propertyTraversal); + + while (properties.hasNext()) { + final Property<?> property = properties.next(); + final Object value = this.returnType == PropertyType.VALUE ? property.value() : property; + if (isVertex) { + map.compute(property.key(), (k, v) -> { + final List<Object> values = v != null ? (List<Object>) v : new ArrayList<>(); + values.add(value); + return values; + }); + } else { + map.put(property.key(), value); + } + } + } + + protected void applyTraversalRingToMap(Map<Object, Object> map){ + if (!traversalRing.isEmpty()) { + // will cop a ConcurrentModification if a key is dropped so need this little copy here + final List<Object> keys = new ArrayList<>(map.keySet()); + for (final Object key : keys) { + map.compute(key, (k, v) -> { + final TraversalProduct product = TraversalUtil.produce(v, (Traversal.Admin) this.traversalRing.next()); + + // compute() should take the null and remove the key + return product.isProductive() ? product.get() : null; + }); + } + this.traversalRing.reset(); + } + } + - public Traversal.Admin<Element, ? extends Property> getPropertyTraversal() { - return propertyTraversal; - } - + public TraversalRing<K, E> getTraversalRing() { + return traversalRing; + } } diff --cc gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/ProductiveByStrategy.java index ba7b92d3c3,29c7bee647..ccdb820bce --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/ProductiveByStrategy.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/optimization/ProductiveByStrategy.java @@@ -21,14 -21,14 +21,12 @@@ package org.apache.tinkerpop.gremlin.pr import org.apache.commons.configuration2.Configuration; import org.apache.commons.configuration2.MapConfiguration; --import org.apache.tinkerpop.gremlin.process.traversal.Step; import org.apache.tinkerpop.gremlin.process.traversal.Traversal; import org.apache.tinkerpop.gremlin.process.traversal.TraversalStrategy; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.DefaultGraphTraversal; import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal; import org.apache.tinkerpop.gremlin.process.traversal.lambda.ValueTraversal; import org.apache.tinkerpop.gremlin.process.traversal.step.ByModulating; --import org.apache.tinkerpop.gremlin.process.traversal.step.Grouping; import org.apache.tinkerpop.gremlin.process.traversal.step.TraversalParent; import org.apache.tinkerpop.gremlin.process.traversal.step.map.CoalesceStep; import org.apache.tinkerpop.gremlin.process.traversal.step.map.ConstantStep; @@@ -91,26 -92,51 +90,41 @@@ public class ProductiveByStrategy exten TraversalHelper.getStepsOfAssignableClass(ByModulating.class, traversal).stream(). filter(bm -> bm instanceof TraversalParent).forEach(bm -> { final TraversalParent parentStep = (TraversalParent) bm; - parentStep.getLocalChildren().forEach(child -> { - if (child instanceof ValueTraversal && !containsValidByPass((ValueTraversal) child) && - final boolean parentStepIsGrouping = parentStep instanceof Grouping; + for (Traversal.Admin child : parentStep.getLocalChildren()) { + // just outright skip the PropertyMapStep.propertyTraversal - it is only set by SubgraphStrategy and + // for this case it doesn't make sense to force that traversal to be productive. if it is forced then + // PropertyMap actually gets a null Property object if the traversal is not productive and that + // causes an NPE. It doesn't make sense to catch the NPE there really as the code wants an empty + // Iterator<Property> rather than a productive null. Actually, the propertyTraversal isn't even a - // by() provided Traversal so it really doesn't make sense for this strategy to touch it. Another ++ // by() provided Traversal, so it really doesn't make sense for this strategy to touch it. Another + // sort of example where strategies shouldn't have to know so much about one another. also, another + // scenario where ProductiveByStrategy isn't so good. the default behavior without this strategy + // works so much more nicely + if (parentStep instanceof PropertyMapStep) { + final Traversal.Admin propertyTraversal = ((PropertyMapStep) parentStep).getPropertyTraversal(); + if (propertyTraversal != null && propertyTraversal.equals(child)) + continue; + } + - // Grouping requires a bit of special handling because of TINKERPOP-2627 which is a bug that has - // some unexpected behavior for coalesce() as the value traversal. Grouping also has so inconsistencies - // in how null vs filter behavior works and that behavior needs to stay to not break in 3.5.x - if (!parentStepIsGrouping) { - if (child instanceof ValueTraversal && hasKeyNotKnownAsProductive((ValueTraversal) child)) { - wrapValueTraversalInCoalesce(parentStep, child); - } else if (!(child.getEndStep() instanceof ReducingBarrierStep)) { - // ending reducing barrier will always return something so seems safe to not bother wrapping - // that up in coalesce(). - final Traversal.Admin extractedChildTraversal = new DefaultGraphTraversal<>(); - TraversalHelper.removeToTraversal(child.getStartStep(), EmptyStep.instance(), extractedChildTraversal); - child.addStep(new CoalesceStep(child, extractedChildTraversal, nullTraversal)); ++ if (child instanceof ValueTraversal && !containsValidByPass((ValueTraversal) child) && + hasKeyNotKnownAsProductive((ValueTraversal) child)) { + wrapValueTraversalInCoalesce(parentStep, child); + } else if (!(child.getEndStep() instanceof ReducingBarrierStep)) { + // ending reducing barrier will always return something so seems safe to not bother wrapping + // that up in coalesce(). + final Traversal.Admin extractedChildTraversal = new DefaultGraphTraversal<>(); + TraversalHelper.removeToTraversal(child.getStartStep(), EmptyStep.instance(), extractedChildTraversal); + child.addStep(new CoalesceStep(child, extractedChildTraversal, nullTraversal)); - // replace so that internally the parent step gets to re-initialize the child as it may need to. - try { - parentStep.replaceLocalChild(child, child); - } catch (IllegalStateException ignored) { - // ignore situations where the parent traversal doesn't support replacement. in those cases - // we simply retain whatever the original behavior was even if it is inconsistent - } - } - } else { - if (child instanceof ValueTraversal && ((Grouping) parentStep).getKeyTraversal() == child && - hasKeyNotKnownAsProductive((ValueTraversal) child) && !containsValidByPass((ValueTraversal) child)) { - wrapValueTraversalInCoalesce(parentStep, child); + // replace so that internally the parent step gets to re-initialize the child as it may need to. + try { + parentStep.replaceLocalChild(child, child); + } catch (IllegalStateException ignored) { + // ignore situations where the parent traversal doesn't support replacement. in those cases + // we simply retain whatever the original behavior was even if it is inconsistent } } - }); + } }); } diff --cc gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java index 7bb4fa9cda,a378170931..ef26a080d2 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/strategy/decoration/SubgraphStrategyProcessTest.java @@@ -508,6 -514,24 +514,23 @@@ public class SubgraphStrategyProcessTes checkResults(Arrays.asList(3, 3, 3, 4, 4, 5, 5, 5), sg.V().outE("uses").values("skill")); checkResults(Arrays.asList(3, 3, 3, 4, 4, 5, 5, 5), sg.V().as("a").properties().select("a").dedup().outE().values("skill")); checkResults(Arrays.asList(3, 3, 3, 4, 4, 5, 5, 5), sg.V().as("a").properties().select("a").dedup().outE().properties("skill").as("b").identity().select("b").by(__.value())); + // + + // testing situations where the key specified is not present for the vertices - TINKERPOP-2920 use of + // constant(true) here is sorta weird because a Traversal<?,VertexProperty> is expected, but not really + sg = g.withStrategies(SubgraphStrategy.create(new MapConfiguration(new HashMap<String, Object>() {{ + put(SubgraphStrategy.VERTEX_PROPERTIES, constant(true)); + }}))); + final List<Map<String, Object>> l = sg.V().project("example").by("example").toList(); + l.forEach(map -> { - assertEquals(1, map.size()); - assertNull(map.get("example")); ++ assertEquals(0, map.size()); + }); + checkResults(Arrays.asList("aachen","baltimore","bremen","brussels","centreville","dulles","kaiserslautern", + "oakland","purcellville","san diego","santa cruz","santa fe","seattle","spremberg"), + sg.withoutStrategies(ProductiveByStrategy.class).V().valueMap("location").select(Column.values).unfold().unfold()); + checkResults(Arrays.asList("aachen","baltimore","bremen","brussels","centreville","dulles","kaiserslautern", + "oakland","purcellville","san diego","santa cruz","santa fe","seattle","spremberg"), + sg.V().valueMap("location").select(Column.values).unfold().unfold()); } @Test
