Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Try to retreive multiple indices for multiple labels #97

Merged
merged 2 commits into from
Oct 10, 2016
Merged

Try to retreive multiple indices for multiple labels #97

merged 2 commits into from
Oct 10, 2016

Conversation

fppt
Copy link
Contributor

@fppt fppt commented Oct 6, 2016

No description provided.

@fppt fppt mentioned this pull request Oct 6, 2016
Closed
@fppt
Copy link
Contributor Author

fppt commented Oct 6, 2016 •
edited
Loading

My own test vertexUniqueIndexLookupWithMultipleLabels is still failing. When multiple labels are defined it fails to use the index. It can find the indices for each label but can't use them. I am not entirely sure why so any hints would be very much appreciated.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 84.257% when pulling 28a8a66 on fppt:try-to-improve-indexing into 1571324 on mpollmeier:master.

@fppt
Copy link
Contributor Author

fppt commented Oct 6, 2016

The error was a poorly defined test on my part. I think this is working now.

mpollmeier
mpollmeier reviewed Oct 8, 2016
View reviewed changes
driver/src/test/java/org/apache/tinkerpop/gremlin/orientdb/OrientGraphIndexTest.java
Set<OrientIndexQuery> indicies = findUsedIndex(traversal);
Assert.assertEquals(3, indicies.size());

assertTrue(valueFound(indicies, v1, value1));
Copy link
Collaborator

Choose a reason for hiding this comment

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

we would we get better test assertion errors if you use assertEquals.. you could e.g. rename valueFound to assertValueFound and use assertEquals in there.

just a thought..

mpollmeier
mpollmeier approved these changes Oct 8, 2016
View reviewed changes
Copy link
Collaborator

@mpollmeier mpollmeier left a comment

Choose a reason for hiding this comment

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

other than that minor comment, LGTM
let me know what you think and I'll merge either way

@mpollmeier mpollmeier merged commit 1d604d5 into orientechnologies:master Oct 10, 2016
@fppt
Copy link
Contributor Author

fppt commented Oct 17, 2016

Thanks for merging Michael. Sorry for not replying sooner, I have been away over the past week. I agree with your comment. It would make the test more informative. In my next PR I will make that minor change.

@fppt
Copy link
Contributor Author

fppt commented Oct 17, 2016

On a different note. When do you think you will make the next release for this plugin ?

@mpollmeier
Copy link
Collaborator

just released 3.2.1.2 :)

@fppt
Copy link
Contributor Author

fppt commented Oct 19, 2016

Awesome ! Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpollmeier mpollmeier mpollmeier approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fppt @coveralls @mpollmeier