| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,14 +22,15 @@ | |
| import org.apache.tinkerpop.gremlin.process.traversal.step.Profiling; | ||
| import org.apache.tinkerpop.gremlin.process.traversal.step.map.PropertiesStep; | ||
| import org.apache.tinkerpop.gremlin.process.traversal.step.util.HasContainer; | ||
| import org.apache.tinkerpop.gremlin.process.traversal.util.MutableMetrics; | ||
| import org.apache.tinkerpop.gremlin.process.traversal.util.TraversalInterruptedException; | ||
| import org.apache.tinkerpop.gremlin.structure.Element; | ||
| import org.apache.tinkerpop.gremlin.structure.Property; | ||
| import org.apache.tinkerpop.gremlin.structure.Vertex; | ||
| import org.apache.tinkerpop.gremlin.structure.util.StringFactory; | ||
| import org.apache.tinkerpop.gremlin.structure.util.wrapped.WrappedVertex; | ||
| import org.janusgraph.core.BaseVertexQuery; | ||
| import org.janusgraph.core.JanusGraphException; | ||
| import org.janusgraph.core.JanusGraphMultiVertexQuery; | ||
| import org.janusgraph.core.JanusGraphProperty; | ||
| import org.janusgraph.core.JanusGraphVertex; | ||
|
|
@@ -43,34 +44,48 @@ | |
| import org.janusgraph.graphdb.tinkerpop.profile.TP3ProfileWrapper; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.HashSet; | ||
| import java.util.Iterator; | ||
| import java.util.LinkedList; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| /** | ||
| * @author Matthias Broecheler (me@matthiasb.com) | ||
| */ | ||
| public class JanusGraphPropertiesStep<E> extends PropertiesStep<E> implements HasStepFolder<Element, E>, Profiling, MultiQueriable<Element,E> { | ||
|
|
||
| private boolean useMultiQuery = false; | ||
| private Set<Vertex> verticesToPrefetch = new HashSet<>(); | ||
| private Map<JanusGraphVertex, Iterable<? extends JanusGraphProperty>> multiQueryResults = null; | ||
| private QueryProfiler queryProfiler = QueryProfiler.NO_OP; | ||
|
|
||
| public JanusGraphPropertiesStep(PropertiesStep<E> originalStep) { | ||
| super(originalStep.getTraversal(), originalStep.getReturnType(), originalStep.getPropertyKeys()); | ||
| originalStep.getLabels().forEach(this::addLabel); | ||
|
|
||
| if (originalStep instanceof JanusGraphPropertiesStep) { | ||
rngcntr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| JanusGraphPropertiesStep originalJanusGraphPropertiesStep = (JanusGraphPropertiesStep) originalStep; | ||
| this.useMultiQuery = originalJanusGraphPropertiesStep.useMultiQuery; | ||
| this.hasContainers = originalJanusGraphPropertiesStep.hasContainers; | ||
| this.limit = originalJanusGraphPropertiesStep.limit; | ||
| } else { | ||
| this.hasContainers = new ArrayList<>(); | ||
| this.limit = Query.NO_LIMIT; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void setUseMultiQuery(boolean useMultiQuery) { | ||
| this.useMultiQuery = useMultiQuery; | ||
| } | ||
|
|
||
| @Override | ||
| public void registerFutureVertexForPrefetching(Vertex futureVertex) { | ||
| verticesToPrefetch.add(futureVertex); | ||
| } | ||
|
|
||
| private <Q extends BaseVertexQuery> Q makeQuery(Q query) { | ||
| final String[] keys = getPropertyKeys(); | ||
| query.keys(keys); | ||
|
|
@@ -91,55 +106,30 @@ private Iterator<E> convertIterator(Iterable<? extends JanusGraphProperty> itera | |
| return (Iterator<E>) Iterators.transform(iterable.iterator(), Property::value); | ||
| } | ||
|
|
||
| /** | ||
| * This initialisation method is called when an attempt to retrieve a vertex from the cached multiQuery results | ||
| * doesn't find an entry. | ||
| */ | ||
| private void prefetchNextBatch() { | ||
| final JanusGraphMultiVertexQuery multiQuery = JanusGraphTraversalUtil.getTx(getTraversal()).multiQuery(); | ||
| multiQuery.addAllVertices(verticesToPrefetch); | ||
| verticesToPrefetch.clear(); | ||
| makeQuery(multiQuery); | ||
|
|
||
| try { | ||
| multiQueryResults = multiQuery.properties(); | ||
| } catch (JanusGraphException janusGraphException) { | ||
|
There was a problem hiding this comment. Why is this needed? The signature of JanusGraphMultiVertexQuery::properties does not say it throws exceptions. There was a problem hiding this comment. I explicitly added that There was a problem hiding this comment. Can you share which test fails? In that case why the old code does not suffer this problem? if (Thread.interrupted()) throw new TraversalInterruptedException()otherwise it does not seem to be semantically correct There was a problem hiding this comment. This is the test that fails if no It creates a I think what happened before this PR is that the There was a problem hiding this comment. Got it. Now it looks good to me. I am just confused why CodeCov complains line 123 is not covered. There was a problem hiding this comment. Somehow Codecov is still reporting that line 123 is not covered by tests. If you set a debug point on line 123 and run the test locally, do you see it get executed? There was a problem hiding this comment. Yes, I did exactly that when I was debugging the exception handling and trying to figure out which Exception has to be caught/thrown. I don't see why Codecov complains here, since the breakpoint definitely triggers. There was a problem hiding this comment. It's probably not a codecov problem. I downloaded the jacoco report from this GitHub Actions run and loaded into Intellij. It shows that JanusGraphPropertiesStep has 0% coverage. There was a problem hiding this comment. I used this one for debugging. It executes a number of TinkerPop tests, including the one you mentioned. Executing this test class leads to the breakpoint being triggered. There was a problem hiding this comment. Thanks, indeed. I also verified that test indeed was executed in CI. No idea why jacoco does not include |
||
| if (janusGraphException.isCausedBy(InterruptedException.class)) { | ||
| throw new TraversalInterruptedException(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| protected Iterator<E> flatMap(final Traverser.Admin<Element> traverser) { | ||
| if (useMultiQuery && traverser.get() instanceof Vertex) { | ||
rngcntr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (multiQueryResults == null || !multiQueryResults.containsKey(traverser.get())) { | ||
| prefetchNextBatch(); | ||
| } | ||
| return convertIterator(multiQueryResults.get(traverser.get())); | ||
| } else if (traverser.get() instanceof JanusGraphVertex || traverser.get() instanceof WrappedVertex) { | ||
|
|
@@ -172,19 +162,6 @@ protected Iterator<E> flatMap(final Traverser.Admin<Element> traverser) { | |
| } | ||
| } | ||
|
|
||
| /* | ||
| ===== HOLDER ===== | ||
| */ | ||
|
|
@@ -211,7 +188,7 @@ public void orderBy(String key, Order order) { | |
|
|
||
| @Override | ||
| public void localOrderBy(List<HasContainer> hasContainers, String key, Order order) { | ||
| throw new UnsupportedOperationException("LocalOrderBy is not supported for properties step."); | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
