From 4123b3bf26156227174ef3c417b36309c2beeb9a Mon Sep 17 00:00:00 2001 From: Chris Hostetter Date: Mon, 18 Jul 2016 10:21:08 -0700 Subject: [PATCH] SOLR-9285: Fixed AIOOBE when using ValueSourceAugmenter in single node RTG --- solr/CHANGES.txt | 2 + .../component/RealTimeGetComponent.java | 79 ++- .../response/transform/DocTransformer.java | 26 +- .../response/transform/DocTransformers.java | 12 + .../transform/ValueSourceAugmenter.java | 13 +- .../cloud/TestCloudPseudoReturnFields.java | 5 +- .../solr/cloud/TestRandomFlRTGCloud.java | 625 ++++++++++++++++++ .../solr/search/TestPseudoReturnFields.java | 57 +- 8 files changed, 764 insertions(+), 55 deletions(-) create mode 100644 solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 4864925f5c4..bff2909c87e 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -155,6 +155,8 @@ Bug Fixes * SOLR-7280: In cloud-mode sort the cores smartly before loading & limit threads to improve cluster stability (noble, Erick Erickson, shalin) +* SOLR-9285: Fixed AIOOBE when using ValueSourceAugmenter in single node RTG (hossman) + Optimizations ---------------------- diff --git a/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java b/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java index 78cebd35f1f..9865a1129da 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java @@ -23,6 +23,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; @@ -58,13 +59,13 @@ import org.apache.solr.common.util.NamedList; import org.apache.solr.common.util.StrUtils; import org.apache.solr.core.SolrCore; import org.apache.solr.request.SolrQueryRequest; -import org.apache.solr.response.BasicResultContext; import org.apache.solr.response.ResultContext; import org.apache.solr.response.SolrQueryResponse; import org.apache.solr.response.transform.DocTransformer; import org.apache.solr.schema.FieldType; import org.apache.solr.schema.IndexSchema; import org.apache.solr.schema.SchemaField; +import org.apache.solr.search.DocList; import org.apache.solr.search.QParser; import org.apache.solr.search.ReturnFields; import org.apache.solr.search.SolrIndexSearcher; @@ -192,12 +193,18 @@ public class RealTimeGetComponent extends SearchComponent UpdateLog ulog = core.getUpdateHandler().getUpdateLog(); RefCounted searcherHolder = null; + + // this is initialized & set on the context *after* any searcher (re-)opening + ResultContext resultContext = null; + final DocTransformer transformer = rsp.getReturnFields().getTransformer(); + + // true in any situation where we have to use a realtime searcher rather then returning docs + // directly from the UpdateLog + final boolean mustUseRealtimeSearcher = + // if we have filters, we need to check those against the indexed form of the doc + (rb.getFilters() != null) + || ((null != transformer) && transformer.needsSolrIndexSearcher()); - DocTransformer transformer = rsp.getReturnFields().getTransformer(); - if (transformer != null) { - ResultContext context = new BasicResultContext(null, rsp.getReturnFields(), null, null, req); - transformer.setContext(context); - } try { SolrIndexSearcher searcher = null; @@ -214,13 +221,13 @@ public class RealTimeGetComponent extends SearchComponent switch (oper) { case UpdateLog.ADD: - if (rb.getFilters() != null) { - // we have filters, so we need to check those against the indexed form of the doc + if (mustUseRealtimeSearcher) { if (searcherHolder != null) { - // close handles to current searchers + // close handles to current searchers & result context searcher = null; searcherHolder.decref(); searcherHolder = null; + resultContext = null; } ulog.openRealtimeSearcher(); // force open a new realtime searcher o = null; // pretend we never found this record and fall through to use the searcher @@ -228,7 +235,7 @@ public class RealTimeGetComponent extends SearchComponent } SolrDocument doc = toSolrDoc((SolrInputDocument)entry.get(entry.size()-1), core.getLatestSchema()); - if(transformer!=null) { + if (transformer!=null) { transformer.transform(doc, -1, 0); // unknown docID } docList.add(doc); @@ -246,6 +253,7 @@ public class RealTimeGetComponent extends SearchComponent if (searcher == null) { searcherHolder = core.getRealtimeSearcher(); searcher = searcherHolder.get(); + // don't bother with ResultContext yet, we won't need it if doc doesn't match filters } int docid = -1; @@ -267,12 +275,17 @@ public class RealTimeGetComponent extends SearchComponent } } - if (docid < 0) continue; + Document luceneDocument = searcher.doc(docid, rsp.getReturnFields().getLuceneFieldNames()); SolrDocument doc = toSolrDoc(luceneDocument, core.getLatestSchema()); searcher.decorateDocValueFields(doc, docid, searcher.getNonStoredDVs(true)); - if( transformer != null ) { + if ( null != transformer) { + if (null == resultContext) { + // either first pass, or we've re-opened searcher - either way now we setContext + resultContext = new RTGResultContext(rsp.getReturnFields(), searcher, req); + transformer.setContext(resultContext); + } transformer.transform(doc, docid, 0); } docList.add(doc); @@ -754,4 +767,46 @@ public class RealTimeGetComponent extends SearchComponent // TODO do we need to sort versions using PeerSync.absComparator? return new ArrayList<>(versionsToRet); } + + /** + * A lite weight ResultContext for use with RTG requests that can point at Realtime Searchers + */ + private static final class RTGResultContext extends ResultContext { + final ReturnFields returnFields; + final SolrIndexSearcher searcher; + final SolrQueryRequest req; + public RTGResultContext(ReturnFields returnFields, SolrIndexSearcher searcher, SolrQueryRequest req) { + this.returnFields = returnFields; + this.searcher = searcher; + this.req = req; + } + + /** @returns null */ + public DocList getDocList() { + return null; + } + + public ReturnFields getReturnFields() { + return this.returnFields; + } + + public SolrIndexSearcher getSearcher() { + return this.searcher; + } + + /** @returns null */ + public Query getQuery() { + return null; + } + + public SolrQueryRequest getRequest() { + return this.req; + } + + /** @returns null */ + public Iterator getProcessedDocuments() { + return null; + } + } + } diff --git a/solr/core/src/java/org/apache/solr/response/transform/DocTransformer.java b/solr/core/src/java/org/apache/solr/response/transform/DocTransformer.java index 11e3a9b69e4..6111804ddbb 100644 --- a/solr/core/src/java/org/apache/solr/response/transform/DocTransformer.java +++ b/solr/core/src/java/org/apache/solr/response/transform/DocTransformer.java @@ -42,8 +42,10 @@ public abstract class DocTransformer { public abstract String getName(); /** - * This is called before transform and sets + * This is called before {@link #transform} to provide context for any subsequent transformations. + * * @param context The {@link ResultContext} stores information about how the documents were produced. + * @see #needsSolrIndexSearcher */ public void setContext( ResultContext context ) { this.context = context; @@ -51,13 +53,31 @@ public abstract class DocTransformer { } /** - * This is where implementations do the actual work + * Indicates if this transformer requires access to the underlying index to perform it's functions. * + * In some situations (notably RealTimeGet) this method may be called before {@link #setContext} + * to determine if the transformer must be given a "full" ResultContext and accurate docIds + * that can be looked up using {@link ResultContext#getSearcher}, or if optimizations can be taken + * advantage of such that {@link ResultContext#getSearcher} may return null, and docIds passed to + * {@link #transform} may be negative. + * + * The default implementation always returns false. + * + * @see ResultContext#getSearcher + * @see #transform + */ + public boolean needsSolrIndexSearcher() { return false; } + + /** + * This is where implementations do the actual work. + * If implementations require a valid docId and index access, the {@link #needsSolrIndexSearcher} + * method must return true * * @param doc The document to alter - * @param docid The Lucene internal doc id + * @param docid The Lucene internal doc id, or -1 in cases where the doc did not come from the index * @param score the score for this document * @throws IOException If there is a low-level I/O error. + * @see #needsSolrIndexSearcher */ public abstract void transform(SolrDocument doc, int docid, float score) throws IOException; diff --git a/solr/core/src/java/org/apache/solr/response/transform/DocTransformers.java b/solr/core/src/java/org/apache/solr/response/transform/DocTransformers.java index e0b3a3c6eec..7bb53ff2182 100644 --- a/solr/core/src/java/org/apache/solr/response/transform/DocTransformers.java +++ b/solr/core/src/java/org/apache/solr/response/transform/DocTransformers.java @@ -76,4 +76,16 @@ public class DocTransformers extends DocTransformer a.transform( doc, docid, score); } } + + /** Returns true if and only if at least 1 child transformer returns true */ + @Override + public boolean needsSolrIndexSearcher() { + for( DocTransformer kid : children ) { + if (kid.needsSolrIndexSearcher()) { + return true; + } + } + return false; + } + } diff --git a/solr/core/src/java/org/apache/solr/response/transform/ValueSourceAugmenter.java b/solr/core/src/java/org/apache/solr/response/transform/ValueSourceAugmenter.java index 9ca0f2bfb9f..9edf826e2c0 100644 --- a/solr/core/src/java/org/apache/solr/response/transform/ValueSourceAugmenter.java +++ b/solr/core/src/java/org/apache/solr/response/transform/ValueSourceAugmenter.java @@ -21,7 +21,6 @@ import java.util.List; import java.util.Map; import org.apache.lucene.index.LeafReaderContext; -import org.apache.lucene.index.IndexReader; import org.apache.lucene.index.ReaderUtil; import org.apache.lucene.queries.function.FunctionValues; import org.apache.lucene.queries.function.ValueSource; @@ -64,11 +63,9 @@ public class ValueSourceAugmenter extends DocTransformer public void setContext( ResultContext context ) { super.setContext(context); try { - IndexReader reader = qparser.getReq().getSearcher().getIndexReader(); - readerContexts = reader.leaves(); + searcher = context.getSearcher(); + readerContexts = searcher.getIndexReader().leaves(); docValuesArr = new FunctionValues[readerContexts.size()]; - - searcher = qparser.getReq().getSearcher(); fcontext = ValueSource.newContext(searcher); this.valueSource.createWeight(fcontext, searcher); } catch (IOException e) { @@ -76,13 +73,11 @@ public class ValueSourceAugmenter extends DocTransformer } } - Map fcontext; SolrIndexSearcher searcher; List readerContexts; FunctionValues docValuesArr[]; - @Override public void transform(SolrDocument doc, int docid, float score) { // This is only good for random-access functions @@ -103,6 +98,10 @@ public class ValueSourceAugmenter extends DocTransformer throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "exception at docid " + docid + " for valuesource " + valueSource, e); } } + + /** Always returns true */ + @Override + public boolean needsSolrIndexSearcher() { return true; } protected void setValue(SolrDocument doc, Object val) { if(val!=null) { diff --git a/solr/core/src/test/org/apache/solr/cloud/TestCloudPseudoReturnFields.java b/solr/core/src/test/org/apache/solr/cloud/TestCloudPseudoReturnFields.java index bf56821a614..8553697d6c2 100644 --- a/solr/core/src/test/org/apache/solr/cloud/TestCloudPseudoReturnFields.java +++ b/solr/core/src/test/org/apache/solr/cloud/TestCloudPseudoReturnFields.java @@ -53,7 +53,10 @@ import org.apache.commons.lang.StringUtils; import org.junit.AfterClass; import org.junit.BeforeClass; -/** @see TestPseudoReturnFields */ +/** + * @see TestPseudoReturnFields + * @see TestRandomFlRTGCloud + */ public class TestCloudPseudoReturnFields extends SolrCloudTestCase { private static final String DEBUG_LABEL = MethodHandles.lookup().lookupClass().getName(); diff --git a/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java b/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java new file mode 100644 index 00000000000..8cf1129dfcb --- /dev/null +++ b/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java @@ -0,0 +1,625 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.cloud; + +import java.lang.invoke.MethodHandles; + +import java.io.IOException; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.Random; + +import org.apache.solr.client.solrj.SolrClient; +import org.apache.solr.client.solrj.SolrServerException; +import org.apache.solr.client.solrj.embedded.JettySolrRunner; +import org.apache.solr.client.solrj.impl.HttpSolrClient; +import org.apache.solr.client.solrj.impl.CloudSolrClient; +import org.apache.solr.client.solrj.response.QueryResponse; + +import org.apache.solr.cloud.SolrCloudTestCase; + +import org.apache.solr.common.SolrDocument; +import org.apache.solr.common.SolrDocumentList; +import org.apache.solr.common.SolrInputDocument; +import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.solr.common.params.SolrParams; +import org.apache.solr.common.util.NamedList; + +import org.apache.solr.util.RandomizeSSL; +import org.apache.lucene.util.TestUtil; + +import org.apache.commons.io.FilenameUtils; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.junit.AfterClass; +import org.junit.BeforeClass; + +/** @see TestCloudPseudoReturnFields */ +@RandomizeSSL(clientAuth=0.0,reason="client auth uses too much RAM") +public class TestRandomFlRTGCloud extends SolrCloudTestCase { + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + private static final String DEBUG_LABEL = MethodHandles.lookup().lookupClass().getName(); + private static final String COLLECTION_NAME = DEBUG_LABEL + "_collection"; + + /** A basic client for operations at the cloud level, default collection will be set */ + private static CloudSolrClient CLOUD_CLIENT; + /** One client per node */ + private static ArrayList CLIENTS = new ArrayList<>(5); + + /** Always included in fl so we can vet what doc we're looking at */ + private static final FlValidator ID_VALIDATOR = new SimpleFieldValueValidator("id"); + + /** + * Types of things we will randomly ask for in fl param, and validate in response docs. + * + * This list starts out with the things we know concretely should work for any type of request, + * {@link #createMiniSolrCloudCluster} will add too it with additional validators that are expected + * to work dependingon hte random cluster creation + * + * @see #addRandomFlValidators + */ + private static final List FL_VALIDATORS = new ArrayList<> + // TODO: SOLR-9314: once all the known bugs are fixed, and this list can be constant + // regardless of single/multi node, change this to Collections.unmodifiableList + // (and adjust jdocs accordingly) + (Arrays.asList( + // TODO: SOLR-9314: add more of these for other various transformers + // + // TODO: add a [docid] validator (blocked by SOLR-9288 & SOLR-9289) + // + new GlobValidator("*"), + new GlobValidator("*_i"), + new GlobValidator("*_s"), + new GlobValidator("a*"), + new SimpleFieldValueValidator("aaa_i"), + new SimpleFieldValueValidator("ccc_s"), + new NotIncludedValidator("bogus_unused_field_ss"), + new NotIncludedValidator("bogus_alias","bogus_alias:other_bogus_field_i"), + new NotIncludedValidator("explain_alias","explain_alias:[explain]"), + new NotIncludedValidator("score"))); + + @BeforeClass + private static void createMiniSolrCloudCluster() throws Exception { + + // Due to known bugs with some transformers in either multi vs single node, we want + // to test both possible cases explicitly and modify the List of FL_VALIDATORS we use accordingly: + // - 50% runs use single node/shard a FL_VALIDATORS with all validators known to work on single node + // - 50% runs use multi node/shard with FL_VALIDATORS only containing stuff that works in cloud + final boolean singleCoreMode = random().nextBoolean(); + if (singleCoreMode) { + // these don't work in distrib cloud mode due to SOLR-9286 + FL_VALIDATORS.addAll(Arrays.asList + (new FunctionValidator("aaa_i"), // fq field + new FunctionValidator("aaa_i", "func_aaa_alias"), + new RenameFieldValueValidator("id", "my_id_alias"), + new RenameFieldValueValidator("bbb_i", "my_int_field_alias"), + new RenameFieldValueValidator("ddd_s", "my_str_field_alias"))); + } else { + // No-Op + // No known transformers that only work in distrib cloud but fail in singleCoreMode + + } + // TODO: SOLR-9314: programatically compare FL_VALIDATORS with all known transformers. + // (ala QueryEqualityTest) can't be done until we eliminate the need for "singleCodeMode" + // conditional logic (might still want 'singleCoreMode' on the MiniSolrCloudCluster side, + // but shouldn't have conditional FlValidators + + // (asuming multi core multi replicas shouldn't matter (assuming multi node) ... + final int repFactor = singleCoreMode ? 1 : (usually() ? 1 : 2); + // ... but we definitely want to ensure forwarded requests to other shards work ... + final int numShards = singleCoreMode ? 1 : 2; + // ... including some forwarded requests from nodes not hosting a shard + final int numNodes = 1 + (singleCoreMode ? 0 : (numShards * repFactor)); + + final String configName = DEBUG_LABEL + "_config-set"; + final Path configDir = Paths.get(TEST_HOME(), "collection1", "conf"); + + configureCluster(numNodes).addConfig(configName, configDir).configure(); + + Map collectionProperties = new HashMap<>(); + collectionProperties.put("config", "solrconfig-tlog.xml"); + collectionProperties.put("schema", "schema-psuedo-fields.xml"); + + assertNotNull(cluster.createCollection(COLLECTION_NAME, numShards, repFactor, + configName, null, null, collectionProperties)); + + CLOUD_CLIENT = cluster.getSolrClient(); + CLOUD_CLIENT.setDefaultCollection(COLLECTION_NAME); + + waitForRecoveriesToFinish(CLOUD_CLIENT); + + for (JettySolrRunner jetty : cluster.getJettySolrRunners()) { + CLIENTS.add(getHttpSolrClient(jetty.getBaseUrl() + "/" + COLLECTION_NAME + "/")); + } + } + + @AfterClass + private static void afterClass() throws Exception { + CLOUD_CLIENT.close(); CLOUD_CLIENT = null; + for (HttpSolrClient client : CLIENTS) { + client.close(); + } + CLIENTS = null; + } + + public void testRandomizedUpdatesAndRTGs() throws Exception { + + final int maxNumDocs = atLeast(100); + final int numSeedDocs = random().nextInt(maxNumDocs / 10); // at most ~10% of the max possible docs + final int numIters = atLeast(maxNumDocs * 10); + final SolrInputDocument[] knownDocs = new SolrInputDocument[maxNumDocs]; + + log.info("Starting {} iters by seeding {} of {} max docs", + numIters, numSeedDocs, maxNumDocs); + + int itersSinceLastCommit = 0; + for (int i = 0; i < numIters; i++) { + itersSinceLastCommit = maybeCommit(random(), itersSinceLastCommit, numIters); + + if (i < numSeedDocs) { + // first N iters all we worry about is seeding + knownDocs[i] = addRandomDocument(i); + } else { + assertOneIter(knownDocs); + } + } + } + + /** + * Randomly chooses to do a commit, where the probability of doing so increases the longer it's been since + * a commit was done. + * + * @returns 0 if a commit was done, else itersSinceLastCommit + 1 + */ + private static int maybeCommit(final Random rand, final int itersSinceLastCommit, final int numIters) throws IOException, SolrServerException { + final float threshold = itersSinceLastCommit / numIters; + if (rand.nextFloat() < threshold) { + log.info("COMMIT"); + assertEquals(0, getRandClient(rand).commit().getStatus()); + return 0; + } + return itersSinceLastCommit + 1; + } + + private void assertOneIter(final SolrInputDocument[] knownDocs) throws IOException, SolrServerException { + // we want to occasionally test more then one doc per RTG + final int numDocsThisIter = TestUtil.nextInt(random(), 1, atLeast(2)); + int numDocsThisIterThatExist = 0; + + // pick some random docIds for this iteration and ... + final int[] docIds = new int[numDocsThisIter]; + for (int i = 0; i < numDocsThisIter; i++) { + docIds[i] = random().nextInt(knownDocs.length); + if (null != knownDocs[docIds[i]]) { + // ...check how many already exist + numDocsThisIterThatExist++; + } + } + + // we want our RTG requests to occasionally include missing/deleted docs, + // but that's not the primary focus of the test, so weight the odds accordingly + if (random().nextInt(numDocsThisIter + 2) <= numDocsThisIterThatExist) { + + if (0 < TestUtil.nextInt(random(), 0, 13)) { + log.info("RTG: numDocsThisIter={} numDocsThisIterThatExist={}, docIds={}", + numDocsThisIter, numDocsThisIterThatExist, docIds); + assertRTG(knownDocs, docIds); + } else { + // sporadically delete some docs instead of doing an RTG + log.info("DEL: numDocsThisIter={} numDocsThisIterThatExist={}, docIds={}", + numDocsThisIter, numDocsThisIterThatExist, docIds); + assertDelete(knownDocs, docIds); + } + } else { + log.info("UPD: numDocsThisIter={} numDocsThisIterThatExist={}, docIds={}", + numDocsThisIter, numDocsThisIterThatExist, docIds); + assertUpdate(knownDocs, docIds); + } + } + + /** + * Does some random indexing of the specified docIds and adds them to knownDocs + */ + private void assertUpdate(final SolrInputDocument[] knownDocs, final int[] docIds) throws IOException, SolrServerException { + + for (final int docId : docIds) { + // TODO: this method should also do some atomic update operations (ie: "inc" and "set") + // (but make sure to eval the updates locally as well before modifying knownDocs) + knownDocs[docId] = addRandomDocument(docId); + } + } + + /** + * Deletes the docIds specified and asserts the results are valid, updateing knownDocs accordingly + */ + private void assertDelete(final SolrInputDocument[] knownDocs, final int[] docIds) throws IOException, SolrServerException { + List ids = new ArrayList<>(docIds.length); + for (final int docId : docIds) { + ids.add("" + docId); + knownDocs[docId] = null; + } + assertEquals("Failed delete: " + docIds, 0, getRandClient(random()).deleteById(ids).getStatus()); + } + + /** + * Adds one randomly generated document with the specified docId, asserting success, and returns + * the document added + */ + private SolrInputDocument addRandomDocument(final int docId) throws IOException, SolrServerException { + final SolrClient client = getRandClient(random()); + + final SolrInputDocument doc = sdoc("id", "" + docId, + "aaa_i", random().nextInt(), + "bbb_i", random().nextInt(), + // + "ccc_s", TestUtil.randomSimpleString(random()), + "ddd_s", TestUtil.randomSimpleString(random()), + // + "axx_i", random().nextInt(), + "ayy_i", random().nextInt(), + "azz_s", TestUtil.randomSimpleString(random())); + + log.info("ADD: {} = {}", docId, doc); + assertEquals(0, client.add(doc).getStatus()); + return doc; + } + + + /** + * Does one or more RTG request for the specified docIds with a randomized fl & fq params, asserting + * that the returned document (if any) makes sense given the expected SolrInputDocuments + */ + private void assertRTG(final SolrInputDocument[] knownDocs, final int[] docIds) throws IOException, SolrServerException { + final SolrClient client = getRandClient(random()); + // NOTE: not using SolrClient.getById or getByIds because we want to force choice of "id" vs "ids" params + final ModifiableSolrParams params = params("qt","/get"); + + // TODO: fq testing blocked by SOLR-9308 + // + // // random fq -- nothing fancy, secondary concern for our test + final Integer FQ_MAX = null; // TODO: replace this... + // final Integer FQ_MAX = usually() ? null : random().nextInt(); // ... with this + // if (null != FQ_MAX) { + // params.add("fq", "aaa_i:[* TO " + FQ_MAX + "]"); + // } + // TODO: END + + final Set validators = new HashSet<>(); + validators.add(ID_VALIDATOR); // always include id so we can be confident which doc we're looking at + addRandomFlValidators(random(), validators); + FlValidator.addFlParams(validators, params); + + final List idsToRequest = new ArrayList<>(docIds.length); + final List docsToExpect = new ArrayList<>(docIds.length); + for (int docId : docIds) { + // every docId will be included in the request + idsToRequest.add("" + docId); + + // only docs that should actually exist and match our (optional) filter will be expected in response + if (null != knownDocs[docId]) { + Integer filterVal = (Integer) knownDocs[docId].getFieldValue("aaa_i"); + if (null == FQ_MAX || ((null != filterVal) && filterVal.intValue() <= FQ_MAX.intValue())) { + docsToExpect.add(knownDocs[docId]); + } + } + } + + // even w/only 1 docId requested, the response format can vary depending on how we request it + final boolean askForList = random().nextBoolean() || (1 != idsToRequest.size()); + if (askForList) { + if (1 == idsToRequest.size()) { + // have to be careful not to try to use "multi" 'id' params with only 1 docId + // with a single docId, the only way to ask for a list is with the "ids" param + params.add("ids", idsToRequest.get(0)); + } else { + if (random().nextBoolean()) { + // each id in it's own param + for (String id : idsToRequest) { + params.add("id",id); + } + } else { + // add one or more comma seperated ids params + params.add(buildCommaSepParams(random(), "ids", idsToRequest)); + } + } + } else { + assert 1 == idsToRequest.size(); + params.add("id",idsToRequest.get(0)); + } + + final QueryResponse rsp = client.query(params); + assertNotNull(params.toString(), rsp); + + final SolrDocumentList docs = getDocsFromRTGResponse(askForList, rsp); + assertNotNull(params + " => " + rsp, docs); + + assertEquals("num docs mismatch: " + params + " => " + docsToExpect + " vs " + docs, + docsToExpect.size(), docs.size()); + + // NOTE: RTG makes no garuntees about the order docs will be returned in when multi requested + for (SolrDocument actual : docs) { + try { + int actualId = Integer.parseInt(actual.getFirstValue("id").toString()); + final SolrInputDocument expected = knownDocs[actualId]; + assertNotNull("expected null doc but RTG returned: " + actual, expected); + + Set expectedFieldNames = new HashSet<>(); + for (FlValidator v : validators) { + expectedFieldNames.addAll(v.assertRTGResults(validators, expected, actual)); + } + // ensure only expected field names are in the actual document + Set actualFieldNames = new HashSet<>(actual.getFieldNames()); + assertEquals("More actual fields then expected", expectedFieldNames, actualFieldNames); + } catch (AssertionError ae) { + throw new AssertionError(params + " => " + actual + ": " + ae.getMessage(), ae); + } + } + } + + /** + * trivial helper method to deal with diff response structure between using a single 'id' param vs + * 2 or more 'id' params (or 1 or more 'ids' params). + * + * NOTE: expectList is currently ignored due to SOLR-9309 -- instead best efforst are made to + * return a synthetic list based on whatever can be found in the response. + * + * @return List from response, or a synthetic one created from single response doc if + * expectList was false; May be empty; May be null if response included null list. + */ + private static SolrDocumentList getDocsFromRTGResponse(final boolean expectList, final QueryResponse rsp) { + // TODO: blocked by SOLR-9309 (once this can be fixed, update jdocs) + if (null != rsp.getResults()) { // TODO: replace this.. + // if (expectList) { // TODO: ...with this tighter check. + return rsp.getResults(); + } + + // else: expect single doc, make our own list... + + final SolrDocumentList result = new SolrDocumentList(); + NamedList raw = rsp.getResponse(); + Object doc = raw.get("doc"); + if (null != doc) { + result.add((SolrDocument) doc); + result.setNumFound(1); + } + return result; + } + + /** + * returns a random SolrClient -- either a CloudSolrClient, or an HttpSolrClient pointed + * at a node in our cluster + */ + public static SolrClient getRandClient(Random rand) { + int numClients = CLIENTS.size(); + int idx = TestUtil.nextInt(rand, 0, numClients); + return (idx == numClients) ? CLOUD_CLIENT : CLIENTS.get(idx); + } + + public static void waitForRecoveriesToFinish(CloudSolrClient client) throws Exception { + assert null != client.getDefaultCollection(); + AbstractDistribZkTestBase.waitForRecoveriesToFinish(client.getDefaultCollection(), + client.getZkStateReader(), + true, true, 330); + } + + /** + * abstraction for diff types of things that can be added to an 'fl' param that can validate + * the results are correct compared to an expected SolrInputDocument + */ + private interface FlValidator { + + /** Given a list of FlValidators, adds one or more fl params that corrispond to the entire set */ + public static void addFlParams(final Collection validators, final ModifiableSolrParams params) { + final List fls = new ArrayList<>(validators.size()); + for (FlValidator v : validators) { + fls.add(v.getFlParam()); + } + params.add(buildCommaSepParams(random(), "fl", fls)); + } + + /** + * Must return a non null String that can be used in an fl param -- either by itself, + * or with other items separated by commas + */ + public String getFlParam(); + + /** + * Given the expected document and the actual document returned from an RTG, this method + * should assert that relative to what {@link #getFlParam} returns, the actual document contained + * what it should relative to the expected document. + * + * @param validators all validators in use for this request, including the current one + * @param expected a document containing the expected fields & values that should be in the index + * @param actual A document that was returned by an RTG request + * @return A set of "field names" in the actual document that this validator expected. + */ + public Collection assertRTGResults(final Collection validators, + final SolrInputDocument expected, + final SolrDocument actual); + } + + private abstract static class FieldValueValidator implements FlValidator { + protected final String expectedFieldName; + protected final String actualFieldName; + public FieldValueValidator(final String expectedFieldName, final String actualFieldName) { + this.expectedFieldName = expectedFieldName; + this.actualFieldName = actualFieldName; + } + public abstract String getFlParam(); + public Collection assertRTGResults(final Collection validators, + final SolrInputDocument expected, + final SolrDocument actual) { + assertEquals(expectedFieldName + " vs " + actualFieldName, + expected.getFieldValue(expectedFieldName), actual.getFirstValue(actualFieldName)); + return Collections.singleton(actualFieldName); + } + } + + private static class SimpleFieldValueValidator extends FieldValueValidator { + public SimpleFieldValueValidator(final String fieldName) { + super(fieldName, fieldName); + } + public String getFlParam() { return expectedFieldName; } + } + + private static class RenameFieldValueValidator extends FieldValueValidator { + /** @see GlobValidator */ + public String getRealFieldName() { return expectedFieldName; } + public RenameFieldValueValidator(final String origFieldName, final String alias) { + super(origFieldName, alias); + } + public String getFlParam() { return actualFieldName + ":" + expectedFieldName; } + } + + /** Trivial validator of a ValueSourceAugmenter */ + private static class FunctionValidator implements FlValidator { + private static String func(String fieldName) { + return "add(1.3,sub("+fieldName+","+fieldName+"))"; + } + protected final String fl; + protected final String resultKey; + protected final String fieldName; + public FunctionValidator(final String fieldName) { + this(func(fieldName), fieldName, func(fieldName)); + } + public FunctionValidator(final String fieldName, final String resultKey) { + this(resultKey + ":" + func(fieldName), fieldName, resultKey); + } + private FunctionValidator(final String fl, final String fieldName, final String resultKey) { + this.fl = fl; + this.resultKey = resultKey; + this.fieldName = fieldName; + } + public String getFlParam() { return fl; } + public Collection assertRTGResults(final Collection validators, + final SolrInputDocument expected, + final SolrDocument actual) { + final Object origVal = expected.getFieldValue(fieldName); + assertTrue("this validator only works on numeric fields: " + origVal, origVal instanceof Number); + + assertEquals(fl, 1.3F, actual.getFirstValue(resultKey)); + return Collections.singleton(resultKey); + } + } + + /** + * Glob based validator. + * This class checks that every field in the expected doc exists in the actual doc with the expected + * value -- with special exceptions for fields that are "renamed" with an alias. + * + * By design, fields that are aliased are "moved" unless the original field name was explicitly included + * in the fl, globs don't count. + * + * @see RenameFieldValueValidator + */ + private static class GlobValidator implements FlValidator { + private final String glob; + public GlobValidator(final String glob) { + this.glob = glob; + } + private final Set matchingFieldsCache = new HashSet<>(); + + public String getFlParam() { return glob; } + + private boolean matchesGlob(final String fieldName) { + if ( FilenameUtils.wildcardMatch(fieldName, glob) ) { + matchingFieldsCache.add(fieldName); // Don't calculate it again + return true; + } + return false; + } + + public Collection assertRTGResults(final Collection validators, + final SolrInputDocument expected, + final SolrDocument actual) { + + final Set renamed = new HashSet<>(validators.size()); + for (FlValidator v : validators) { + if (v instanceof RenameFieldValueValidator) { + renamed.add(((RenameFieldValueValidator)v).getRealFieldName()); + } + } + + // every real field name matching the glob that is not renamed should be in the results + Set result = new HashSet<>(expected.getFieldNames().size()); + for (String f : expected.getFieldNames()) { + if ( matchesGlob(f) && (! renamed.contains(f) ) ) { + result.add(f); + assertEquals(glob + " => " + f, expected.getFieldValue(f), actual.getFirstValue(f)); + } + } + return result; + } + } + + /** + * for things like "score" and "[explain]" where we explicitly expect what we ask for in the fl + * to not be returned when using RTG. + */ + private static class NotIncludedValidator implements FlValidator { + private final String fieldName; + private final String fl; + public NotIncludedValidator(final String fl) { + this(fl, fl); + } + public NotIncludedValidator(final String fieldName, final String fl) { + this.fieldName = fieldName; + this.fl = fl; + } + public String getFlParam() { return fl; } + public Collection assertRTGResults(final Collection validators, + final SolrInputDocument expected, + final SolrDocument actual) { + assertEquals(fl, null, actual.getFirstValue(fieldName)); + return Collections.emptySet(); + } + } + + /** helper method for adding a random number (may be 0) of items from {@link #FL_VALIDATORS} */ + private static void addRandomFlValidators(final Random r, final Set validators) { + List copyToShuffle = new ArrayList<>(FL_VALIDATORS); + Collections.shuffle(copyToShuffle, r); + final int numToReturn = r.nextInt(copyToShuffle.size()); + validators.addAll(copyToShuffle.subList(0, numToReturn + 1)); + } + + /** + * Given an ordered list of values to include in a (key) param, randomly groups them (ie: comma seperated) + * into actual param key=values which are returned as a new SolrParams instance + */ + private static SolrParams buildCommaSepParams(final Random rand, final String key, Collection values) { + ModifiableSolrParams result = new ModifiableSolrParams(); + List copy = new ArrayList<>(values); + while (! copy.isEmpty()) { + List slice = copy.subList(0, random().nextInt(1 + copy.size())); + result.add(key,String.join(",",slice)); + slice.clear(); + } + return result; + } +} diff --git a/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java b/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java index 8b85ba0097e..68f0773dec5 100644 --- a/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java +++ b/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java @@ -95,7 +95,6 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { ); } - @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9285") public void testMultiValuedRTG() throws Exception { // single value int using alias that matches multivalued dynamic field - via RTG @@ -247,7 +246,6 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { ); } - @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9285") public void testFunctionsRTG() throws Exception { // if we use RTG (committed or otherwise) functions should behave the same for (String id : Arrays.asList("42","99")) { @@ -286,7 +284,6 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { ); } - @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9285") public void testFunctionsAndExplicitRTG() throws Exception { // shouldn't matter if we use RTG (committed or otherwise) for (String id : Arrays.asList("42","99")) { @@ -346,10 +343,7 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { } - @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9285") public void testFunctionsAndScoreRTG() throws Exception { - // NOTE: once this test is fixed to pass, testAugmentersRTG should also be updated to test a abs(val_i) - // if we use RTG (committed or otherwise) score should be ignored for (String id : Arrays.asList("42","99")) { for (SolrParams p : Arrays.asList(params("fl","score","fl","log(val_i)","fl","abs(val_i)"), @@ -360,7 +354,7 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { req(p, "qt","/get","id",id, "wt","xml") ,"count(//doc)=1" ,"//doc/double[@name='log(val_i)']" - ,"//doc/float[@name='abs(val_i)']" + ,"//doc/float[@name='abs(val_i)'][.='1.0']" ,"//doc[count(*)=2]" ); } @@ -561,20 +555,21 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { // behavior shouldn't matter if we are committed or uncommitted for (String id : Arrays.asList("42","99")) { // NOTE: once testDocIdAugmenterRTG can pass, [docid] should be tested here as well. - // NOTE: once testFunctionsAndScoreRTG can pass, abs(val_i) should be tested here as well - for (SolrParams p : Arrays.asList(params("fl","[shard],[explain],x_alias:[value v=10 t=int]"), - params("fl","[shard]","fl","[explain],x_alias:[value v=10 t=int]"), - params("fl","[shard]","fl","[explain]","fl","x_alias:[value v=10 t=int]"))) { + for (SolrParams p : Arrays.asList + (params("fl","[shard],[explain],x_alias:[value v=10 t=int],abs(val_i)"), + params("fl","[shard],abs(val_i)","fl","[explain],x_alias:[value v=10 t=int]"), + params("fl","[shard]","fl","[explain],x_alias:[value v=10 t=int]","fl","abs(val_i)"), + params("fl","[shard]","fl","[explain]","fl","x_alias:[value v=10 t=int]","fl","abs(val_i)"))) { assertQ(id + ": " + p, req(p, "qt","/get","id",id, "wt","xml") ,"count(//doc)=1" // ,"//doc/int[@name='[docid]']" // TODO - // ,"//doc/gloat[@name='abs(val_i)']" // TODO + ,"//doc/float[@name='abs(val_i)'][.='1.0']" ,"//doc/str[@name='[shard]'][.='[not a shard request]']" // RTG: [explain] should be missing (ignored) ,"//doc/int[@name='x_alias'][.=10]" - ,"//doc[count(*)=2]" + ,"//doc[count(*)=3]" ); } } @@ -601,20 +596,20 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { // behavior shouldn't matter if we are committed or uncommitted for (String id : Arrays.asList("42","99")) { // NOTE: once testDocIdAugmenterRTG can pass, [docid] should be tested here as well. - // NOTE: once testFunctionsAndScoreRTG can pass, abs(val_i) should be tested here as well - for (SolrParams p : Arrays.asList(params("fl","id,[explain],x_alias:[value v=10 t=int]"), - params("fl","id","fl","[explain],x_alias:[value v=10 t=int]"), - params("fl","id","fl","[explain]","fl","x_alias:[value v=10 t=int]"))) { + for (SolrParams p : Arrays.asList + (params("fl","id,[explain],x_alias:[value v=10 t=int],abs(val_i)"), + params("fl","id,abs(val_i)","fl","[explain],x_alias:[value v=10 t=int]"), + params("fl","id","fl","[explain]","fl","x_alias:[value v=10 t=int]","fl","abs(val_i)"))) { assertQ(id + ": " + p, req(p, "qt","/get","id",id, "wt","xml") ,"count(//doc)=1" ,"//doc/str[@name='id']" // ,"//doc/int[@name='[docid]']" // TODO - // ,"//doc/gloat[@name='abs(val_i)']" // TODO + ,"//doc/float[@name='abs(val_i)'][.='1.0']" // RTG: [explain] should be missing (ignored) ,"//doc/int[@name='x_alias'][.=10]" - ,"//doc[count(*)=2]" + ,"//doc[count(*)=3]" ); } } @@ -652,29 +647,28 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { // if we use RTG (committed or otherwise) score should be ignored for (String id : Arrays.asList("42","99")) { // NOTE: once testDocIdAugmenterRTG can pass, [docid] should be tested here as well. - // NOTE: once testFunctionsAndScoreRTG can pass, abs(val_i) should be tested here as well assertQ(id, req("qt","/get","id",id, "wt","xml", - "fl","x_alias:[value v=10 t=int],score") + "fl","x_alias:[value v=10 t=int],score,abs(val_i)") // ,"//doc/int[@name='[docid]']" // TODO - // ,"//doc/gloat[@name='abs(val_i)']" // TODO + ,"//doc/float[@name='abs(val_i)'][.='1.0']" ,"//doc/int[@name='x_alias'][.=10]" - ,"//doc[count(*)=1]" + ,"//doc[count(*)=2]" ); - for (SolrParams p : Arrays.asList(params("fl","x_alias:[value v=10 t=int],[explain],score"), - params("fl","x_alias:[value v=10 t=int],[explain]","fl","score"), - params("fl","x_alias:[value v=10 t=int]","fl","[explain]","fl","score"))) { + for (SolrParams p : Arrays.asList(params("fl","x_alias:[value v=10 t=int],[explain],score,abs(val_i)"), + params("fl","x_alias:[value v=10 t=int],[explain]","fl","score,abs(val_i)"), + params("fl","x_alias:[value v=10 t=int]","fl","[explain]","fl","score","fl","abs(val_i)"))) { assertQ(p.toString(), req(p, "qt","/get","id",id, "wt","xml") // ,"//doc/int[@name='[docid]']" // TODO - // ,"//doc/gloat[@name='abs(val_i)']" // TODO + ,"//doc/float[@name='abs(val_i)'][.='1.0']" ,"//doc/int[@name='x_alias'][.=10]" // RTG: [explain] and score should be missing (ignored) - ,"//doc[count(*)=1]" + ,"//doc[count(*)=2]" ); } } @@ -720,8 +714,7 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { // NOTE: 'ssto' is the missing one final List fl = Arrays.asList // NOTE: once testDocIdAugmenterRTG can pass, [docid] should be tested here as well. - // NOTE: once testFunctionsAndScoreRTG can pass, abs(val_i) should be tested here as well - ("id","[explain]","score","val_*","subj*"); + ("id","[explain]","score","val_*","subj*","abs(val_i)"); final int iters = atLeast(random, 10); for (int i = 0; i< iters; i++) { @@ -742,11 +735,11 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { ,"count(//doc)=1" ,"//doc/str[@name='id']" // ,"//doc/int[@name='[docid]']" // TODO - // ,"//doc/gloat[@name='abs(val_i)']" // TODO + ,"//doc/float[@name='abs(val_i)'][.='1.0']" // RTG: [explain] and score should be missing (ignored) ,"//doc/int[@name='val_i'][.=1]" ,"//doc/str[@name='subject']" - ,"//doc[count(*)=3]" + ,"//doc[count(*)=4]" ); } }