From 8254724bb17fd4cf479ab34c3919d9a862ca4ea5 Mon Sep 17 00:00:00 2001 From: Ryan McKinley Date: Fri, 25 Mar 2016 12:22:44 -0700 Subject: [PATCH 1/3] SOLR-8902: Make sure ReturnFields only returns the requested fields --- solr/CHANGES.txt | 3 +++ .../apache/solr/response/BinaryResponseWriter.java | 11 ++++++++--- .../org/apache/solr/search/SolrReturnFields.java | 12 +----------- .../org/apache/solr/search/ReturnFieldsTest.java | 8 ++++++++ 4 files changed, 20 insertions(+), 14 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index a9731e16f9a..baca0df101f 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -55,6 +55,9 @@ Bug Fixes * SOLR-8857: HdfsUpdateLog does not use configured or new default number of version buckets and is hard coded to 256. (Mark Miller, yonik, Gregory Chanan) +* SOLR-8902: Make sure ReturnFields only returns the requested fields from (fl=) evn when + DocumentTransformers ask for getExtraRequestFields() (ryan) + Optimizations ---------------------- * SOLR-8722: Don't force a full ZkStateReader refresh on every Overseer operation. diff --git a/solr/core/src/java/org/apache/solr/response/BinaryResponseWriter.java b/solr/core/src/java/org/apache/solr/response/BinaryResponseWriter.java index 6878f16ccb0..885bf78a021 100644 --- a/solr/core/src/java/org/apache/solr/response/BinaryResponseWriter.java +++ b/solr/core/src/java/org/apache/solr/response/BinaryResponseWriter.java @@ -72,8 +72,7 @@ public class BinaryResponseWriter implements BinaryQueryResponseWriter { public static class Resolver implements JavaBinCodec.ObjectResolver , JavaBinCodec.WritableDocFields { protected final SolrQueryRequest solrQueryRequest; protected IndexSchema schema; - protected SolrIndexSearcher searcher; // TODO - this is never set? always null? - protected final ReturnFields returnFields; + protected ReturnFields returnFields; public Resolver(SolrQueryRequest req, ReturnFields returnFields) { solrQueryRequest = req; @@ -83,7 +82,13 @@ public class BinaryResponseWriter implements BinaryQueryResponseWriter { @Override public Object resolve(Object o, JavaBinCodec codec) throws IOException { if (o instanceof ResultContext) { - writeResults((ResultContext) o, codec); + ReturnFields orig = returnFields; + ResultContext res = (ResultContext)o; + if(res.getReturnFields()!=null) { + returnFields = res.getReturnFields(); + } + writeResults(res, codec); + returnFields = orig; return null; // null means we completely handled it } if (o instanceof DocList) { diff --git a/solr/core/src/java/org/apache/solr/search/SolrReturnFields.java b/solr/core/src/java/org/apache/solr/search/SolrReturnFields.java index b84f4de5528..b667f0e2e46 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrReturnFields.java +++ b/solr/core/src/java/org/apache/solr/search/SolrReturnFields.java @@ -51,6 +51,7 @@ public class SolrReturnFields extends ReturnFields { private final List globs = new ArrayList<>(1); // The lucene field names to request from the SolrIndexSearcher + // This *may* include fields that will not be in the final response private final Set fields = new HashSet<>(); // Field names that are OK to include in the response. @@ -130,17 +131,6 @@ public class SolrReturnFields extends ReturnFields { augmenters.addTransformer( new RenameFieldTransformer( from, to, copy ) ); } - if( !_wantsAllFields ) { - if( !globs.isEmpty() ) { - // TODO??? need to fill up the fields with matching field names in the index - // and add them to okFieldNames? - // maybe just get all fields? - // this would disable field selection optimization... i think thatis OK - fields.clear(); // this will get all fields, and use wantsField to limit - } - okFieldNames.addAll( fields ); - } - if( augmenters.size() == 1 ) { transformer = augmenters.getTransformer(0); } diff --git a/solr/core/src/test/org/apache/solr/search/ReturnFieldsTest.java b/solr/core/src/test/org/apache/solr/search/ReturnFieldsTest.java index f72aee8385d..459f9596c6a 100644 --- a/solr/core/src/test/org/apache/solr/search/ReturnFieldsTest.java +++ b/solr/core/src/test/org/apache/solr/search/ReturnFieldsTest.java @@ -264,6 +264,14 @@ public class ReturnFieldsTest extends SolrTestCaseJ4 { assertFalse( rf.wantsField( "id" ) ); assertFalse(rf.wantsAllFields()); assertNull(rf.getTransformer()); + + // Don't return 'store_rpt' just because it is required by the transformer + rf = new SolrReturnFields( req("fl", "[geo f=store_rpt]") ); + assertFalse( rf.wantsScore() ); + assertTrue(rf.wantsField("[geo]")); + assertFalse( rf.wantsField( "store_rpt" ) ); + assertFalse(rf.wantsAllFields()); + assertNotNull(rf.getTransformer()); } @Test From f785d2a0341ab5192e963ff5a470fb09b6b2709b Mon Sep 17 00:00:00 2001 From: Mike McCandless Date: Fri, 25 Mar 2016 15:40:16 -0400 Subject: [PATCH 2/3] randomize how BKDWriter splits in RandomCodec so we exercise geo shape APIs with more exotic rectangles --- .../codecs/lucene60/Lucene60PointsWriter.java | 4 +- .../org/apache/lucene/util/bkd/BKDWriter.java | 3 +- .../org/apache/lucene/index/RandomCodec.java | 68 ++++++++++++++++++- 3 files changed, 70 insertions(+), 5 deletions(-) diff --git a/lucene/core/src/java/org/apache/lucene/codecs/lucene60/Lucene60PointsWriter.java b/lucene/core/src/java/org/apache/lucene/codecs/lucene60/Lucene60PointsWriter.java index 7bb1faf6803..9098cfbb01a 100644 --- a/lucene/core/src/java/org/apache/lucene/codecs/lucene60/Lucene60PointsWriter.java +++ b/lucene/core/src/java/org/apache/lucene/codecs/lucene60/Lucene60PointsWriter.java @@ -42,8 +42,8 @@ import org.apache.lucene.util.bkd.BKDWriter; /** Writes dimensional values */ public class Lucene60PointsWriter extends PointsWriter implements Closeable { - final IndexOutput dataOut; - final Map indexFPs = new HashMap<>(); + protected final IndexOutput dataOut; + protected final Map indexFPs = new HashMap<>(); final SegmentWriteState writeState; final int maxPointsInLeafNode; final double maxMBSortInHeap; diff --git a/lucene/core/src/java/org/apache/lucene/util/bkd/BKDWriter.java b/lucene/core/src/java/org/apache/lucene/util/bkd/BKDWriter.java index dd2ec5df7f7..5002e50445b 100644 --- a/lucene/core/src/java/org/apache/lucene/util/bkd/BKDWriter.java +++ b/lucene/core/src/java/org/apache/lucene/util/bkd/BKDWriter.java @@ -1033,8 +1033,7 @@ public class BKDWriter implements Closeable { return true; } - // TODO: make this protected when we want to subclass to play with different splitting criteria - private int split(byte[] minPackedValue, byte[] maxPackedValue) { + protected int split(byte[] minPackedValue, byte[] maxPackedValue) { // Find which dim has the largest span so we can split on it: int splitDim = -1; for(int dim=0;dim 0) { + indexFPs.put(fieldInfo.name, writer.finish(dataOut)); + } + } + } + }; } @Override @@ -152,6 +197,7 @@ public class RandomCodec extends AssertingCodec { maxPointsInLeafNode = TestUtil.nextInt(random, 16, 2048); maxMBSortInHeap = 4.0 + (3*random.nextDouble()); + bkdSplitRandomSeed = random.nextInt(); add(avoidCodecs, TestUtil.getDefaultPostingsFormat(minItemsPerBlock, maxItemsPerBlock), @@ -221,4 +267,24 @@ public class RandomCodec extends AssertingCodec { ", maxPointsInLeafNode=" + maxPointsInLeafNode + ", maxMBSortInHeap=" + maxMBSortInHeap; } + + /** Just like {@link BKDWriter} except it evilly picks random ways to split cells on + * recursion to try to provoke geo APIs that get upset at fun rectangles. */ + private static class RandomlySplittingBKDWriter extends BKDWriter { + + final Random random; + + public RandomlySplittingBKDWriter(int maxDoc, Directory tempDir, String tempFileNamePrefix, int numDims, + int bytesPerDim, int maxPointsInLeafNode, double maxMBSortInHeap, + long totalPointCount, boolean singleValuePerDoc, int randomSeed) throws IOException { + super(maxDoc, tempDir, tempFileNamePrefix, numDims, bytesPerDim, maxPointsInLeafNode, maxMBSortInHeap, totalPointCount, singleValuePerDoc); + this.random = new Random(randomSeed); + } + + @Override + protected int split(byte[] minPackedValue, byte[] maxPackedValue) { + // BKD normally defaults by the widest dimension, to try to make as squarish cells as possible, but we just pick a random one ;) + return random.nextInt(numDims); + } + } } From e26c0b7125903e24e5865b825f0f9e993eb10178 Mon Sep 17 00:00:00 2001 From: Ryan McKinley Date: Fri, 25 Mar 2016 13:49:25 -0700 Subject: [PATCH 3/3] SOLR-8902: fix glob test (put back the fields.clear()) --- .../src/java/org/apache/solr/search/SolrReturnFields.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/solr/core/src/java/org/apache/solr/search/SolrReturnFields.java b/solr/core/src/java/org/apache/solr/search/SolrReturnFields.java index b667f0e2e46..34ef79e674f 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrReturnFields.java +++ b/solr/core/src/java/org/apache/solr/search/SolrReturnFields.java @@ -130,6 +130,13 @@ public class SolrReturnFields extends ReturnFields { } augmenters.addTransformer( new RenameFieldTransformer( from, to, copy ) ); } + if( !_wantsAllFields && !globs.isEmpty() ) { + // TODO??? need to fill up the fields with matching field names in the index + // and add them to okFieldNames? + // maybe just get all fields? + // this would disable field selection optimization... i think thatis OK + fields.clear(); // this will get all fields, and use wantsField to limit + } if( augmenters.size() == 1 ) { transformer = augmenters.getTransformer(0);