diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 1e9c7751f6a..4fe3ca2183a 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -123,6 +123,8 @@ Bug Fixes * SOLR-9285: Fixed AIOOBE when using ValueSourceAugmenter in single node RTG (hossman) +* SOLR-9288: Fix [docid] transformer to return -1 when used in RTG with uncommitted doc (hossman) + Optimizations ---------------------- diff --git a/solr/core/src/java/org/apache/solr/response/transform/DocIdAugmenterFactory.java b/solr/core/src/java/org/apache/solr/response/transform/DocIdAugmenterFactory.java index 2f037c9468e..e95ac1e379e 100644 --- a/solr/core/src/java/org/apache/solr/response/transform/DocIdAugmenterFactory.java +++ b/solr/core/src/java/org/apache/solr/response/transform/DocIdAugmenterFactory.java @@ -21,7 +21,10 @@ import org.apache.solr.common.params.SolrParams; import org.apache.solr.request.SolrQueryRequest; /** - * + * Augments the document with a [docid] integer containing it's current + * (internal) id in the lucene index. May be -1 if this document did not come from the + * index (ie: a RealTimeGet from the transaction log) + * * @since solr 4.0 */ public class DocIdAugmenterFactory extends TransformerFactory @@ -49,9 +52,8 @@ class DocIdAugmenter extends DocTransformer @Override public void transform(SolrDocument doc, int docid, float score) { - if( docid >= 0 ) { - doc.setField( name, docid ); - } + assert -1 <= docid; + doc.setField( name, docid ); } } diff --git a/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java b/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java index 8cf1129dfcb..682d6a03aac 100644 --- a/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java +++ b/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java @@ -47,6 +47,7 @@ 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.response.transform.DocTransformer; // jdocs import org.apache.solr.util.RandomizeSSL; import org.apache.lucene.util.TestUtil; @@ -90,8 +91,6 @@ public class TestRandomFlRTGCloud extends SolrCloudTestCase { (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"), @@ -119,6 +118,9 @@ public class TestRandomFlRTGCloud extends SolrCloudTestCase { new RenameFieldValueValidator("id", "my_id_alias"), new RenameFieldValueValidator("bbb_i", "my_int_field_alias"), new RenameFieldValueValidator("ddd_s", "my_str_field_alias"))); + // SOLR-9289... + FL_VALIDATORS.add(new DocIdValidator()); + FL_VALIDATORS.add(new DocIdValidator("my_docid_alias")); } else { // No-Op // No known transformers that only work in distrib cloud but fail in singleCoreMode @@ -428,7 +430,7 @@ public class TestRandomFlRTGCloud extends SolrCloudTestCase { } /** - * abstraction for diff types of things that can be added to an 'fl' param that can validate + * 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 { @@ -441,6 +443,21 @@ public class TestRandomFlRTGCloud extends SolrCloudTestCase { } params.add(buildCommaSepParams(random(), "fl", fls)); } + + /** + * Indicates if this validator is for a transformer that returns true from + * {@link DocTransformer#needsSolrIndexSearcher}. Other validators for transformers that + * do not require a re-opened searcher (but may have slightly diff behavior depending + * on wether a doc comesfrom the index or from the update log) may use this information to + * decide wether they wish to enforce stricter assertions on the resulting document. + * + * The default implementation always returns false + * + * @see DocIdValidator + */ + public default boolean requiresRealtimeSearcherReOpen() { + return false; + } /** * Must return a non null String that can be used in an fl param -- either by itself, @@ -496,6 +513,42 @@ public class TestRandomFlRTGCloud extends SolrCloudTestCase { public String getFlParam() { return actualFieldName + ":" + expectedFieldName; } } + /** + * enforces that a valid [docid] is present in the response, possibly using a + * resultKey alias. By default the only validation of docId values is that they are an integer + * greater than or equal to -1 -- but if any other validator in use returns true + * from {@link #requiresRealtimeSearcherReOpen} then the constraint is tightened and values must + * be greater than or equal to 0 + */ + private static class DocIdValidator implements FlValidator { + private final String resultKey; + public DocIdValidator(final String resultKey) { + this.resultKey = resultKey; + } + public DocIdValidator() { + this("[docid]"); + } + public String getFlParam() { return "[docid]".equals(resultKey) ? resultKey : resultKey+":[docid]"; } + public Collection assertRTGResults(final Collection validators, + final SolrInputDocument expected, + final SolrDocument actual) { + final Object value = actual.getFirstValue(resultKey); + assertNotNull(getFlParam() + " => no value in actual doc", value); + assertTrue("[docid] must be an Integer: " + value, value instanceof Integer); + + int minValidDocId = -1; // if it comes from update log + for (FlValidator other : validators) { + if (other.requiresRealtimeSearcherReOpen()) { + minValidDocId = 0; + break; + } + } + assertTrue("[docid] must be >= " + minValidDocId + ": " + value, + minValidDocId <= ((Integer)value).intValue()); + return Collections.singleton(resultKey); + } + } + /** Trivial validator of a ValueSourceAugmenter */ private static class FunctionValidator implements FlValidator { private static String func(String fieldName) { @@ -515,6 +568,8 @@ public class TestRandomFlRTGCloud extends SolrCloudTestCase { this.resultKey = resultKey; this.fieldName = fieldName; } + /** always returns true */ + public boolean requiresRealtimeSearcherReOpen() { return true; } public String getFlParam() { return fl; } public Collection assertRTGResults(final Collection validators, final SolrInputDocument expected, 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 68f0773dec5..87f3d89c021 100644 --- a/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java +++ b/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java @@ -531,21 +531,13 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { } } - @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9288") public void testDocIdAugmenterRTG() throws Exception { - // NOTE: once this test is fixed to pass, testAugmentersRTG should also be updated to test [docid] - - // TODO: behavior of fl=[docid] should be consistent regardless of wether doc is committed - // what should behavior be? - // right now, for an uncommited doc, [docid] is silently ignored and no value included in result - // perhaps it should be "null" or "-1" ? - - // behavior shouldn't matter if we are committed or uncommitted + // for an uncommitted doc, we should get -1 for (String id : Arrays.asList("42","99")) { assertQ(id + ": fl=[docid]", req("qt","/get","id",id, "wt","xml", "fl","[docid]") ,"count(//doc)=1" - ,"//doc/int[@name='[docid]']" + ,"//doc/int[@name='[docid]'][.>=-1]" ,"//doc[count(*)=1]" ); } @@ -554,22 +546,21 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { public void testAugmentersRTG() throws Exception { // 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. 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)"))) { + (params("fl","[docid],[shard],[explain],x_alias:[value v=10 t=int],abs(val_i)"), + params("fl","[docid],[shard],abs(val_i)","fl","[explain],x_alias:[value v=10 t=int]"), + params("fl","[docid],[shard]","fl","[explain],x_alias:[value v=10 t=int]","fl","abs(val_i)"), + params("fl","[docid]","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/int[@name='[docid]'][.>=-1]" ,"//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(*)=3]" + ,"//doc[count(*)=4]" ); } } @@ -595,21 +586,20 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { public void testAugmentersAndExplicitRTG() throws Exception { // 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. 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)"))) { + (params("fl","id,[docid],[explain],x_alias:[value v=10 t=int],abs(val_i)"), + params("fl","id,[docid],abs(val_i)","fl","[explain],x_alias:[value v=10 t=int]"), + params("fl","id","fl","[docid]","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/int[@name='[docid]'][.>=-1]" ,"//doc/float[@name='abs(val_i)'][.='1.0']" // RTG: [explain] should be missing (ignored) ,"//doc/int[@name='x_alias'][.=10]" - ,"//doc[count(*)=3]" + ,"//doc[count(*)=4]" ); } } @@ -646,29 +636,28 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { public void testAugmentersAndScoreRTG() throws Exception { // 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. assertQ(id, req("qt","/get","id",id, "wt","xml", - "fl","x_alias:[value v=10 t=int],score,abs(val_i)") - // ,"//doc/int[@name='[docid]']" // TODO + "fl","x_alias:[value v=10 t=int],score,abs(val_i),[docid]") + ,"//doc/int[@name='[docid]'][.>=-1]" ,"//doc/float[@name='abs(val_i)'][.='1.0']" ,"//doc/int[@name='x_alias'][.=10]" - ,"//doc[count(*)=2]" + ,"//doc[count(*)=3]" ); - 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)"))) { + for (SolrParams p : Arrays.asList(params("fl","[docid],x_alias:[value v=10 t=int],[explain],score,abs(val_i)"), + params("fl","x_alias:[value v=10 t=int],[explain]","fl","[docid],score,abs(val_i)"), + params("fl","[docid]","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/int[@name='[docid]']" // 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(*)=2]" + ,"//doc[count(*)=3]" ); } } @@ -713,8 +702,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. - ("id","[explain]","score","val_*","subj*","abs(val_i)"); + ("id","[explain]","score","val_*","subj*","abs(val_i)","[docid]"); final int iters = atLeast(random, 10); for (int i = 0; i< iters; i++) { @@ -734,12 +722,12 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 { req(p, "qt","/get","id",id, "wt","xml") ,"count(//doc)=1" ,"//doc/str[@name='id']" - // ,"//doc/int[@name='[docid]']" // TODO + ,"//doc/int[@name='[docid]'][.>=-1]" ,"//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(*)=4]" + ,"//doc[count(*)=5]" ); } }