From 4f9e67c63ce5130795df647ef5e86ae970601cb6 Mon Sep 17 00:00:00 2001 From: Caleb Rackliffe Date: Wed, 29 Jun 2016 00:01:23 -0700 Subject: [PATCH 1/5] SOLR-8858 SolrIndexSearcher#doc() Completely Ignores Field Filters Unless Lazy Field Loading is Enabled --- .../solr/handler/component/QueryComponent.java | 8 ++++++-- .../apache/solr/search/SolrIndexSearcher.java | 14 +++++++++----- .../transform/TestSubQueryTransformer.java | 17 ++++++++--------- 3 files changed, 23 insertions(+), 16 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java index 931d36284648..b3bcc91c7b61 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java @@ -910,8 +910,12 @@ protected void createMainQuery(ResponseBuilder rb) { additionalAdded = addFL(additionalFL, "score", additionalAdded); } } else { - // reset so that only unique key is requested in shard requests - sreq.params.set(CommonParams.FL, rb.req.getSchema().getUniqueKeyField().getName()); + if (rb.req.getSearcher().enableLazyFieldLoading) { + // reset so that only unique key is requested in shard requests + sreq.params.set(CommonParams.FL, rb.req.getSchema().getUniqueKeyField().getName()); + } else { + sreq.params.set(CommonParams.FL, "*"); + } if (shardQueryIncludeScore) { additionalAdded = addFL(additionalFL, "score", additionalAdded); } diff --git a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java index 213f75835402..fab80aa078c6 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java +++ b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java @@ -766,12 +766,16 @@ public Document doc(int i, Set fields) throws IOException { } final DirectoryReader reader = getIndexReader(); - if (!enableLazyFieldLoading || fields == null) { - d = reader.document(i); + if (fields != null) { + if (enableLazyFieldLoading) { + final SetNonLazyFieldSelector visitor = new SetNonLazyFieldSelector(fields, reader, i); + reader.document(i, visitor); + d = visitor.doc; + } else { + d = reader.document(i, fields); + } } else { - final SetNonLazyFieldSelector visitor = new SetNonLazyFieldSelector(fields, reader, i); - reader.document(i, visitor); - d = visitor.doc; + d = reader.document(i); } if (documentCache != null) { diff --git a/solr/core/src/test/org/apache/solr/response/transform/TestSubQueryTransformer.java b/solr/core/src/test/org/apache/solr/response/transform/TestSubQueryTransformer.java index bd9ff393dcb9..e9af1cff7042 100644 --- a/solr/core/src/test/org/apache/solr/response/transform/TestSubQueryTransformer.java +++ b/solr/core/src/test/org/apache/solr/response/transform/TestSubQueryTransformer.java @@ -127,7 +127,7 @@ public void testJohnOrNancySingleField() throws Exception { //System.out.println("p "+peopleMultiplier+" d "+deptMultiplier); assertQ("subq1.fl is limited to single field", req("q","name_s:(john nancy)", "indent","true", - "fl","name_s_dv,depts:[subquery]", + "fl","dept_ss_dv,name_s_dv,depts:[subquery]", "rows","" + (2 * peopleMultiplier), "depts.q","{!term f=dept_id_s v=$row.dept_ss_dv}", "depts.fl","text_t", @@ -150,8 +150,8 @@ public void testJohnOrNancySingleField() throws Exception { } final String[] johnAndNancyParams = new String[]{"q","name_s:(john nancy)", "indent","true", - "fl","name_s_dv,depts:[subquery]", - "fl","depts_i:[subquery]", + "fl","dept_ss_dv,name_s_dv,depts:[subquery]", + "fl","dept_i_dv,depts_i:[subquery]", "rows","" + (2 * peopleMultiplier), "depts.q","{!term f=dept_id_s v=$row.dept_ss_dv}", "depts.fl","text_t", @@ -225,7 +225,7 @@ public void testRowsStartForSubqueryAndScores() throws Exception { } String[] john = new String[]{"q","name_s:john", "indent","true", - "fl","name_s_dv,depts:[subquery]", + "fl","dept_ss_dv,name_s_dv,depts:[subquery]", "rows","" + (2 * peopleMultiplier), "depts.q","+{!term f=dept_id_s v=$row.dept_ss_dv}^=0 _val_:id_i", "depts.fl","id", @@ -277,10 +277,10 @@ public void testThreeLevel() throws Exception { assertQ("dave works at both dept with other folks", // System.out.println(h.query( req(new String[]{"q","name_s:dave", "indent","true", - "fl","name_s_dv,subq1:[subquery]", + "fl","dept_ss_dv,name_s_dv,subq1:[subquery]", "rows","" + peopleMultiplier, "subq1.q","{!terms f=dept_id_s v=$row.dept_ss_dv}", - "subq1.fl","text_t,dept_id_s_dv,neighbours:[subquery]", + "subq1.fl","dept_id_i_dv,text_t,dept_id_s_dv,neighbours:[subquery]", "subq1.indent","true", "subq1.rows",""+(deptMultiplier*2), "subq1.neighbours.q",//flipping via numbers @@ -459,9 +459,8 @@ public void testMultiValue() throws Exception { assertQ("dave works at both, whether we set a default separator or both", req(new String[]{"q","name_s:dave", "indent","true", - "fl",(random().nextBoolean() ? "name_s_dv" : "*")+ //"dept_ss_dv, - ",subq1:[subquery " - +((random1.nextBoolean() ? "" : "separator=,"))+"]", + "fl", (random().nextBoolean() ? "name_s_dv,dept_ss_dv" : "*") + + ",subq1:[subquery " +((random1.nextBoolean() ? "" : "separator=,"))+"]", "rows","" + peopleMultiplier, "subq1.q","{!terms f=dept_id_s v=$row.dept_ss_dv "+((random1.nextBoolean() ? "" : "separator=,"))+"}", "subq1.fl","text_t", From 650f8d790ffed99839772f1ec2a0a255d9221b5c Mon Sep 17 00:00:00 2001 From: Caleb Rackliffe Date: Fri, 1 Jul 2016 22:41:24 -0700 Subject: [PATCH 2/5] SOLR-8858 opted to change the caching behavior in SolrIndexSearcher#doc() to skip caching non-full documents instead of hacking the field list in QueryComponent --- .../org/apache/solr/handler/component/QueryComponent.java | 8 ++------ .../java/org/apache/solr/search/SolrIndexSearcher.java | 5 ++++- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java index b3bcc91c7b61..931d36284648 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java @@ -910,12 +910,8 @@ protected void createMainQuery(ResponseBuilder rb) { additionalAdded = addFL(additionalFL, "score", additionalAdded); } } else { - if (rb.req.getSearcher().enableLazyFieldLoading) { - // reset so that only unique key is requested in shard requests - sreq.params.set(CommonParams.FL, rb.req.getSchema().getUniqueKeyField().getName()); - } else { - sreq.params.set(CommonParams.FL, "*"); - } + // reset so that only unique key is requested in shard requests + sreq.params.set(CommonParams.FL, rb.req.getSchema().getUniqueKeyField().getName()); if (shardQueryIncludeScore) { additionalAdded = addFL(additionalFL, "score", additionalAdded); } diff --git a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java index fab80aa078c6..188237d15a9d 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java +++ b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java @@ -766,6 +766,8 @@ public Document doc(int i, Set fields) throws IOException { } final DirectoryReader reader = getIndexReader(); + boolean cachable = true; + if (fields != null) { if (enableLazyFieldLoading) { final SetNonLazyFieldSelector visitor = new SetNonLazyFieldSelector(fields, reader, i); @@ -773,12 +775,13 @@ public Document doc(int i, Set fields) throws IOException { d = visitor.doc; } else { d = reader.document(i, fields); + cachable = false; } } else { d = reader.document(i); } - if (documentCache != null) { + if (documentCache != null && cachable) { documentCache.put(i, d); } From 3166bbda7b57e817162e679c40a0693f326ae2ea Mon Sep 17 00:00:00 2001 From: Caleb Rackliffe Date: Mon, 4 Jul 2016 22:52:58 -0700 Subject: [PATCH 3/5] SOLR-8858 The caching behavior in SolrIndexSearcher#doc() is now functionally equivalent to its pre-patch state, i.e. a complete document is cached even if a fields list is specified. --- .../org/apache/solr/search/SolrIndexSearcher.java | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java index 188237d15a9d..145720bfc69b 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java +++ b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java @@ -766,7 +766,7 @@ public Document doc(int i, Set fields) throws IOException { } final DirectoryReader reader = getIndexReader(); - boolean cachable = true; + boolean containsAllFields = true; if (fields != null) { if (enableLazyFieldLoading) { @@ -775,14 +775,20 @@ public Document doc(int i, Set fields) throws IOException { d = visitor.doc; } else { d = reader.document(i, fields); - cachable = false; + containsAllFields = false; } } else { d = reader.document(i); } - if (documentCache != null && cachable) { - documentCache.put(i, d); + if (documentCache != null) { + // Only cache the already retrieved document if it is complete... + if (containsAllFields) { + documentCache.put(i, d); + } else { + // ...and retrieve a complete document for caching otherwise. + documentCache.put(i, reader.document(i)); + } } return d; From 218cfb3913ff0aff833c418384c62871f7cbf9e3 Mon Sep 17 00:00:00 2001 From: Caleb Rackliffe Date: Wed, 6 Jul 2016 14:25:44 -0700 Subject: [PATCH 4/5] SOLR-8858 If the document cache and lazy field loading are disabled, pass only requested fields to the visitor --- .../apache/solr/search/SolrIndexSearcher.java | 25 ++++++------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java index 145720bfc69b..074bad9048fa 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java +++ b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java @@ -766,29 +766,20 @@ public Document doc(int i, Set fields) throws IOException { } final DirectoryReader reader = getIndexReader(); - boolean containsAllFields = true; - - if (fields != null) { - if (enableLazyFieldLoading) { - final SetNonLazyFieldSelector visitor = new SetNonLazyFieldSelector(fields, reader, i); - reader.document(i, visitor); - d = visitor.doc; - } else { + if (!enableLazyFieldLoading) { + if (fields != null && documentCache == null) { d = reader.document(i, fields); - containsAllFields = false; + } else { + d = reader.document(i); } } else { - d = reader.document(i); + final SetNonLazyFieldSelector visitor = new SetNonLazyFieldSelector(fields, reader, i); + reader.document(i, visitor); + d = visitor.doc; } if (documentCache != null) { - // Only cache the already retrieved document if it is complete... - if (containsAllFields) { - documentCache.put(i, d); - } else { - // ...and retrieve a complete document for caching otherwise. - documentCache.put(i, reader.document(i)); - } + documentCache.put(i, d); } return d; From 9da8e6f9f313409043223d1b6220bd6c637cba27 Mon Sep 17 00:00:00 2001 From: Caleb Rackliffe Date: Wed, 6 Jul 2016 16:07:27 -0700 Subject: [PATCH 5/5] SOLR-8858 If the document cache and lazy field loading are disabled, pass only requested fields to the visitor --- .../org/apache/solr/search/SolrIndexSearcher.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java index 074bad9048fa..7d6a2d3300d3 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java +++ b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java @@ -766,16 +766,18 @@ public Document doc(int i, Set fields) throws IOException { } final DirectoryReader reader = getIndexReader(); - if (!enableLazyFieldLoading) { - if (fields != null && documentCache == null) { + if (fields != null) { + if (enableLazyFieldLoading) { + final SetNonLazyFieldSelector visitor = new SetNonLazyFieldSelector(fields, reader, i); + reader.document(i, visitor); + d = visitor.doc; + } else if (documentCache == null) { d = reader.document(i, fields); } else { d = reader.document(i); } } else { - final SetNonLazyFieldSelector visitor = new SetNonLazyFieldSelector(fields, reader, i); - reader.document(i, visitor); - d = visitor.doc; + d = reader.document(i); } if (documentCache != null) {