Skip to content

Issue #71: Various Gremlin query patterns throw NPE when query.batch=true#1000

Merged
twilmes merged 1 commit intoJanusGraph:0.2from
twilmes:issue_71
Apr 26, 2018
Merged

Issue #71: Various Gremlin query patterns throw NPE when query.batch=true#1000
twilmes merged 1 commit intoJanusGraph:0.2from
twilmes:issue_71

Conversation

@twilmes
Copy link
Contributor

@twilmes twilmes commented Apr 9, 2018

Storage adapter tests were added to exercise all backends in query.batch mode.
The JanusGraphLocalQueryOptimizerStrategy was updated to disable multiquery on Janus specific steps that are nested within RepeatStep, MatchStep, and BranchSteps.

These steps cause difficulties with the barrier step nature of the batched query multiquery enabled steps. We should still be able to get at least some level of batched backend retrieval working with these but it will be a more involved effort. The intent of this PR is to un-break the query.batch option for users and to allow them to benefit from the decreased latencies in other common query patterns and to lay down a foundation for testing out further multiquery optimizations.


Thank you for contributing to JanusGraph!

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there an issue associated with this PR? Is it referenced in the commit message?
  • Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
  • Has your PR been rebased against the latest commit within the target branch (typically master)?
  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you written and/or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
  • If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?
  • If this PR is a documentation-only change, have you added a [skip ci]
    tag to the first line of your commit message to avoid spending CPU cycles in
    Travis CI when no code, tests, or build configuration are modified?

Note:

Please ensure that once the PR is submitted, you check Travis CI for build issues and submit an update to your PR as soon as possible.

@janusgraph-bot janusgraph-bot added the cla: yes This PR is compliant with the CLA label Apr 9, 2018
Copy link
Member

@pluradj pluradj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ted. A few small items.

import java.io.IOException;

/**
* @author Matthias Broecheler (me@matthiasb.com)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cut & paste error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, good catch

public class InMemoryMultiQueryGraphProvider extends AbstractJanusGraphProvider {
@Override
public ModifiableConfiguration getJanusGraphConfiguration(String graphName, Class<?> test, String testMethodName) {
return StorageSetup.getInMemoryConfiguration();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing .set(GraphDatabaseConfiguration.USE_MULTIQUERY, true)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'oh!

TraversalHelper.replaceStep(localStep, vstep, traversal);

if (useMultiQuery) {
if (useMultiQuery) {// && !(isChildOf(vstep, MULTIQUERY_INCOMPATIBLE_STEPS))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this code commented out?


clopen(option(USE_MULTIQUERY), true);
gts = graph.traversal();
List<Vertex> results = gts.V(sv[0]).emit().repeat(__.barrier().out("knows")).toList();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verify the results

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was leftover from some debugging and shouldn't have been included. I'll get rid of it as this combo of steps is covered by the TP test suite.

@pluradj pluradj added this to the Release v0.2.1 milestone Apr 10, 2018
Copy link
Member

@pluradj pluradj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks @twilmes

@pluradj
Copy link
Member

pluradj commented Apr 26, 2018

@twilmes please squash before merging, thanks

…ery.batch=true

Storage adapter tests were added to exercise all backends in query.batch mode.
The JanusGraphLocalQueryOptimizerStrategy was updated to disable multiquery on Janus specific steps
that are nested within RepeatStep, MatchStep, and BranchSteps.
@twilmes
Copy link
Contributor Author

twilmes commented Apr 26, 2018

@pluradj I got them squashed. I'll merge after tests complete. I lost track of where we landed on keeping master in sync. I seem to remember merging 0.2 into master previously. Is that still the process or are we cherry picking commits?

@twilmes twilmes merged commit bcd569f into JanusGraph:0.2 Apr 26, 2018
bwatson-rti-org pushed a commit to bwatson-rti-org/janusgraph that referenced this pull request Mar 9, 2019
Issue JanusGraph#71: Various Gremlin query patterns throw NPE when query.batch=true
micpod pushed a commit to micpod/janusgraph that referenced this pull request Nov 5, 2019
Issue JanusGraph#71: Various Gremlin query patterns throw NPE when query.batch=true
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This PR is compliant with the CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants