From 54cfa922ae37ee4a65901240d9952a9062f0d3b0 Mon Sep 17 00:00:00 2001 From: Yonik Seeley Date: Thu, 24 Apr 2014 18:12:15 +0000 Subject: [PATCH] SOLR-6011: ComplexPhraseQuery hashCode/equals fix for inOrder git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1589812 13f79535-47bb-0310-9956-ffa450edef68 --- lucene/CHANGES.txt | 4 + .../ComplexPhraseQueryParser.java | 9 +- .../complexPhrase/TestComplexPhraseQuery.java | 25 ++++ .../TestComplexPhraseQParserPlugin.java | 132 +++++++++--------- 4 files changed, 102 insertions(+), 68 deletions(-) diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index b64b6b6aaed..59129438409 100644 --- a/lucene/CHANGES.txt +++ b/lucene/CHANGES.txt @@ -319,6 +319,10 @@ Bug fixes * LUCENE-5624: Ensure NativeFSLockFactory does not leak file handles if it is unable to obtain the lock. (Uwe Schindler, Robert Muir) +* SOLR-6011: ComplexPhraseQueryParser produced Query objects that did not correctly + implement hashCode and equals (inOrder was ignored), causing issues for any + system using Query objects as keys. (yonik) + Test Framework * LUCENE-5592: Incorrectly reported uncloseable files. (Dawid Weiss) diff --git a/lucene/queryparser/src/java/org/apache/lucene/queryparser/complexPhrase/ComplexPhraseQueryParser.java b/lucene/queryparser/src/java/org/apache/lucene/queryparser/complexPhrase/ComplexPhraseQueryParser.java index c32f4c249ef..0e76a1969ef 100644 --- a/lucene/queryparser/src/java/org/apache/lucene/queryparser/complexPhrase/ComplexPhraseQueryParser.java +++ b/lucene/queryparser/src/java/org/apache/lucene/queryparser/complexPhrase/ComplexPhraseQueryParser.java @@ -208,11 +208,11 @@ public class ComplexPhraseQueryParser extends QueryParser { */ static class ComplexPhraseQuery extends Query { - String field; + final String field; - String phrasedQueryStringContents; + final String phrasedQueryStringContents; - int slopFactor; + final int slopFactor; private final boolean inOrder; @@ -394,6 +394,7 @@ public class ComplexPhraseQueryParser extends QueryParser { + ((phrasedQueryStringContents == null) ? 0 : phrasedQueryStringContents.hashCode()); result = prime * result + slopFactor; + result = prime * result + (inOrder ? 1 : 0); return result; } @@ -422,7 +423,7 @@ public class ComplexPhraseQueryParser extends QueryParser { return false; if (slopFactor != other.slopFactor) return false; - return true; + return inOrder == other.inOrder; } } } diff --git a/lucene/queryparser/src/test/org/apache/lucene/queryparser/complexPhrase/TestComplexPhraseQuery.java b/lucene/queryparser/src/test/org/apache/lucene/queryparser/complexPhrase/TestComplexPhraseQuery.java index bf7e189dbd7..35351019ef7 100644 --- a/lucene/queryparser/src/test/org/apache/lucene/queryparser/complexPhrase/TestComplexPhraseQuery.java +++ b/lucene/queryparser/src/test/org/apache/lucene/queryparser/complexPhrase/TestComplexPhraseQuery.java @@ -139,6 +139,31 @@ public class TestComplexPhraseQuery extends LuceneTestCase { checkMatches("+role:developer +name:jack*", ""); checkMatches("name:\"john smith\"~2 AND role:designer AND id:3", "3"); } + + public void testHashcodeEquals() throws Exception { + ComplexPhraseQueryParser qp = new ComplexPhraseQueryParser(TEST_VERSION_CURRENT, defaultFieldName, analyzer); + qp.setInOrder(true); + qp.setFuzzyPrefixLength(1); + + String qString = "\"aaa* bbb*\""; + + Query q = qp.parse(qString); + Query q2 = qp.parse(qString); + + assertEquals(q.hashCode(), q2.hashCode()); + assertEquals(q, q2); + + qp.setInOrder(false); // SOLR-6011 + + q2 = qp.parse(qString); + + // although the general contract of hashCode can't guarantee different values, if we only change one thing + // about a single query, it normally should result in a different value (and will with the current + // implementation in ComplexPhraseQuery) + assertTrue(q.hashCode() != q2.hashCode()); + assertTrue(!q.equals(q2)); + assertTrue(!q2.equals(q)); + } @Override public void setUp() throws Exception { diff --git a/solr/core/src/test/org/apache/solr/search/TestComplexPhraseQParserPlugin.java b/solr/core/src/test/org/apache/solr/search/TestComplexPhraseQParserPlugin.java index 2b0066b92be..8e12509e9b9 100644 --- a/solr/core/src/test/org/apache/solr/search/TestComplexPhraseQParserPlugin.java +++ b/solr/core/src/test/org/apache/solr/search/TestComplexPhraseQParserPlugin.java @@ -30,7 +30,7 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase { @BeforeClass public static void beforeClass() throws Exception { - initCore("solrconfig-query-parser-init.xml","schema-complex-phrase.xml"); + initCore("solrconfig.xml","schema15.xml"); } @Override @@ -54,43 +54,43 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase { assertQ(req("q", "{!complexphrase} \"john smith\"") , "//result[@numFound='1']" - , "//doc[./int[@name='id']='1']" + , "//doc[./str[@name='id']='1']" ); assertQ(req("q", "{!complexphrase} \"j* smyth~\"") , "//result[@numFound='2']" - , "//doc[./int[@name='id']='1']" - , "//doc[./int[@name='id']='2']" + , "//doc[./str[@name='id']='1']" + , "//doc[./str[@name='id']='2']" ); assertQ(req("q", "{!complexphrase} \"(jo* -john) smith\"") , "//result[@numFound='1']" - , "//doc[./int[@name='id']='2']" + , "//doc[./str[@name='id']='2']" ); assertQ(req("q", "{!complexphrase} \"jo* smith\"~2") , "//result[@numFound='3']" - , "//doc[./int[@name='id']='1']" - , "//doc[./int[@name='id']='2']" - , "//doc[./int[@name='id']='3']" + , "//doc[./str[@name='id']='1']" + , "//doc[./str[@name='id']='2']" + , "//doc[./str[@name='id']='3']" ); assertQ(req("q", "{!complexphrase} \"jo* [sma TO smz]\"") , "//result[@numFound='2']" - , "//doc[./int[@name='id']='1']" - , "//doc[./int[@name='id']='2']" + , "//doc[./str[@name='id']='1']" + , "//doc[./str[@name='id']='2']" ); assertQ(req("q", "{!complexphrase} \"john\"") , "//result[@numFound='2']" - , "//doc[./int[@name='id']='1']" - , "//doc[./int[@name='id']='3']" + , "//doc[./str[@name='id']='1']" + , "//doc[./str[@name='id']='3']" ); assertQ(req("q", "{!complexphrase} \"(john johathon) smith\"") , "//result[@numFound='2']" - , "//doc[./int[@name='id']='1']" - , "//doc[./int[@name='id']='2']" + , "//doc[./str[@name='id']='1']" + , "//doc[./str[@name='id']='2']" ); } @@ -113,55 +113,55 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase { assertQ("Simple multi-term still works", sumLRF.makeRequest("name:\"john smith\""), - "//doc[./int[@name='id']='1']", + "//doc[./str[@name='id']='1']", "//result[@numFound='1']" ); assertQ(req("q", "{!complexphrase} name:\"john smith\""), - "//doc[./int[@name='id']='1']", + "//doc[./str[@name='id']='1']", "//result[@numFound='1']" ); assertQ("wildcards and fuzzies are OK in phrases", sumLRF.makeRequest("name:\"j* smyth~\""), - "//doc[./int[@name='id']='1']", - "//doc[./int[@name='id']='2']", + "//doc[./str[@name='id']='1']", + "//doc[./str[@name='id']='2']", "//result[@numFound='2']" ); assertQ("boolean logic works", sumLRF.makeRequest("name:\"(jo* -john) smith\""), - "//doc[./int[@name='id']='2']", + "//doc[./str[@name='id']='2']", "//result[@numFound='1']" ); assertQ("position logic works", sumLRF.makeRequest("name:\"jo* smith\"~2"), - "//doc[./int[@name='id']='1']", - "//doc[./int[@name='id']='2']", - "//doc[./int[@name='id']='3']", + "//doc[./str[@name='id']='1']", + "//doc[./str[@name='id']='2']", + "//doc[./str[@name='id']='3']", "//result[@numFound='3']" ); assertQ("range queries supported", sumLRF.makeRequest("name:\"jo* [sma TO smz]\""), - "//doc[./int[@name='id']='1']", - "//doc[./int[@name='id']='2']", + "//doc[./str[@name='id']='1']", + "//doc[./str[@name='id']='2']", "//result[@numFound='2']" ); assertQ("Simple single-term still works", sumLRF.makeRequest("name:\"john\""), - "//doc[./int[@name='id']='1']", - "//doc[./int[@name='id']='3']", + "//doc[./str[@name='id']='1']", + "//doc[./str[@name='id']='3']", "//result[@numFound='2']" ); assertQ("OR inside phrase works", sumLRF.makeRequest("name:\"(john johathon) smith\""), - "//doc[./int[@name='id']='1']", - "//doc[./int[@name='id']='2']", + "//doc[./str[@name='id']='1']", + "//doc[./str[@name='id']='2']", "//result[@numFound='2']" ); @@ -192,9 +192,9 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase { assertQ("range queries supported", sumLRF.makeRequest("name:[sma TO smz]"), - "//doc[./int[@name='id']='1']", - "//doc[./int[@name='id']='2']", - "//doc[./int[@name='id']='3']", + "//doc[./str[@name='id']='1']", + "//doc[./str[@name='id']='2']", + "//doc[./str[@name='id']='3']", "//result[@numFound='3']" ); @@ -246,29 +246,29 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase { assertQ(req("q", "{!complexphrase} name:\"protein digest\" AND text:\"dna rules\"") , "//result[@numFound='1']" - , "//doc[./int[@name='id']='3']" + , "//doc[./str[@name='id']='3']" ); assertQ(req("q", "{!complexphrase} name:\"prot* dige*\" AND text:\"d* r*\"") , "//result[@numFound='1']" - , "//doc[./int[@name='id']='3']" + , "//doc[./str[@name='id']='3']" ); assertQ(req("q", "{!complexphrase inOrder=\"false\"} name:\"dna* rule*\" AND text:\"prot* diges*\"") , "//result[@numFound='1']" - , "//doc[./int[@name='id']='1']" + , "//doc[./str[@name='id']='1']" ); - assertQ(req("q", "{!unorderedcomplexphrase} name:\"protein digest\" AND text:\"dna rules\"~2") + assertQ(req("q", "{!complexphrase inOrder=false} name:\"protein digest\" AND text:\"dna rules\"~2") , "//result[@numFound='2']" - , "//doc[./int[@name='id']='3']" - , "//doc[./int[@name='id']='4']" + , "//doc[./str[@name='id']='3']" + , "//doc[./str[@name='id']='4']" ); - assertQ(req("q", "{!unorderedcomplexphrase inOrder=\"true\"} name:\"protein digest\" AND text:\"dna rules\"") + assertQ(req("q", "{!complexphrase inOrder=\"true\"} name:\"protein digest\" AND text:\"dna rules\"") , "//result[@numFound='1']" - , "//doc[./int[@name='id']='3']" + , "//doc[./str[@name='id']='3']" ); } @@ -290,49 +290,49 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase { */ assertQ(req("q", "{!complexphrase} \"protein digest\"") , "//result[@numFound='1']" - , "//doc[./int[@name='id']='1']" + , "//doc[./str[@name='id']='1']" ); assertQ(req("q", "{!complexphrase} \"pro* di*\"") , "//result[@numFound='1']" - , "//doc[./int[@name='id']='1']" + , "//doc[./str[@name='id']='1']" ); assertQ(req("q", "{!complexphrase} name:\"protein digest\"") , "//result[@numFound='1']" - , "//doc[./int[@name='id']='3']" + , "//doc[./str[@name='id']='3']" ); assertQ(req("q", "{!complexphrase} name:\"pro* di*\"") , "//result[@numFound='1']" - , "//doc[./int[@name='id']='3']" + , "//doc[./str[@name='id']='3']" ); /** * unordered phrase query returns two documents. */ - assertQ(req("q", "{!unorderedcomplexphrase} \"digest protein\"") + assertQ(req("q", "{!complexphrase inOrder=false} \"digest protein\"") , "//result[@numFound='2']" - , "//doc[./int[@name='id']='1']" - , "//doc[./int[@name='id']='2']" + , "//doc[./str[@name='id']='1']" + , "//doc[./str[@name='id']='2']" ); - assertQ(req("q", "{!unorderedcomplexphrase} \"di* pro*\"") + assertQ(req("q", "{!complexphrase inOrder=false} \"di* pro*\"") , "//result[@numFound='2']" - , "//doc[./int[@name='id']='1']" - , "//doc[./int[@name='id']='2']" + , "//doc[./str[@name='id']='1']" + , "//doc[./str[@name='id']='2']" ); - assertQ(req("q", "{!unorderedcomplexphrase} name:\"digest protein\"") + assertQ(req("q", "{!complexphrase inOrder=false} name:\"digest protein\"") , "//result[@numFound='2']" - , "//doc[./int[@name='id']='3']" - , "//doc[./int[@name='id']='4']" + , "//doc[./str[@name='id']='3']" + , "//doc[./str[@name='id']='4']" ); - assertQ(req("q", "{!unorderedcomplexphrase} name:\"di* pro*\"") + assertQ(req("q", "{!complexphrase inOrder=false} name:\"di* pro*\"") , "//result[@numFound='2']" - , "//doc[./int[@name='id']='3']" - , "//doc[./int[@name='id']='4']" + , "//doc[./str[@name='id']='3']" + , "//doc[./str[@name='id']='4']" ); /** @@ -340,8 +340,13 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase { */ assertQ(req("q", "{!complexphrase inOrder=false} \"di* pro*\"") , "//result[@numFound='2']" - , "//doc[./int[@name='id']='1']" - , "//doc[./int[@name='id']='2']" + , "//doc[./str[@name='id']='1']" + , "//doc[./str[@name='id']='2']" + ); + + + assertQ(req("q", "{!complexphrase inOrder=true} \"di* pro*\"") + , "//result[@numFound='1']" ); /** @@ -349,8 +354,8 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase { */ assertQ(req("q", "{!complexphrase inOrder=false df=name} \"di* pro*\"") , "//result[@numFound='2']" - , "//doc[./int[@name='id']='3']" - , "//doc[./int[@name='id']='4']" + , "//doc[./str[@name='id']='3']" + , "//doc[./str[@name='id']='4']" ); } /** @@ -369,14 +374,13 @@ public class TestComplexPhraseQParserPlugin extends AbstractSolrTestCase { assertQ(req("q", "{!complexphrase} \"sulfur-reducing bacteria\"") , "//result[@numFound='2']" - , "//doc[./int[@name='id']='1']" - , "//doc[./int[@name='id']='2']" + , "//doc[./str[@name='id']='1']" + , "//doc[./str[@name='id']='2']" ); + // the analysis for "name" currently does not break on "-" (only whitespace) and thus only matches one doc assertQ(req("q", "{!complexphrase} name:\"sulfur-reducing bacteria\"") - , "//result[@numFound='2']" - , "//doc[./int[@name='id']='3']" - , "//doc[./int[@name='id']='4']" + , "//result[@numFound='1']" ); } }