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.

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

[fix?] Lucene index – make query-field only depend on the context sequence #1262

Merged
merged 3 commits into from Mar 4, 2017

Conversation

Projects
None yet
4 participants
@olvidalo
Contributor

olvidalo commented Feb 16, 2017

I've noticed that ft:query-field is not evaluated only once for the context sequence, but once for each item in the context sequence. For example, given an index named "testField" on the qname test with a collection with test1.xml and test2.xml both containing the following data:

<test>
  <p>Rüsselsheim</p>
  <p>Russelsheim</p>
  <p>Māori</p>
  <p>Maori</p>
</test>

the query /test[ft:query-field("testField", "Rüsselsheim", <options/>)] will execute ft:query-field two times instead of only once. While this still leads to the correct results, the performance is not as good as it could be for larger context sequences. I've included an xquery test demonstrating this.

This code fixes the issue by making the function only depend on the context sequence. Please be aware that I don't think that I have completely understood the dependency stuff yet, but I am pretty certain that query-field will only ever depend on the context sequence, never on the context item. If this assumption is correct, the changes should be safe to merge. At least all lucene index tests still pass (at least those that are passing on the current develop – there are some failing xquery tests in there, maybe someone should have a look at those, too..).

@adamretter adamretter requested review from wolfgangmm and adamretter Feb 21, 2017

@adamretter

Looks good

}
return result;
}
public int getDependencies() {

This comment has been minimized.

@adamretter

adamretter Mar 4, 2017

Member

Should have an@Override annotation

@@ -90,6 +90,9 @@ public QueryField(XQueryContext context, FunctionSignature signature) {
* @see org.exist.xquery.PathExpr#analyze(org.exist.xquery.Expression)
*/
public void analyze(AnalyzeContextInfo contextInfo) throws XPathException {

This comment has been minimized.

@adamretter

adamretter Mar 4, 2017

Member

The Override annotation was previously missing here. Could you please add an @Override annotation?

@adamretter

This comment has been minimized.

Member

adamretter commented Mar 4, 2017

@wolfgangmm I think this looks good, if you could comment then we could get it merged before tomorrow maybe

@wolfgangmm

Code looks good, so I'm ok with merging it.

@adamretter

This comment has been minimized.

Member

adamretter commented Mar 4, 2017

@wolgangmm thank you :-)

@adamretter adamretter merged commit 06e9207 into eXist-db:develop Mar 4, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@olvidalo

This comment has been minimized.

Contributor

olvidalo commented Mar 4, 2017

Thanks! :)

@joewiz

This comment has been minimized.

Member

joewiz commented Mar 5, 2017

Looks like a potentially big performance improvement. Thanks for spotting this @olvidalo and for the fix & tests.

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