SOLR-9308: Fix distributed RTG to forward request params, fixes fq and non-default fl params

solr/CHANGES.txt

@@ -182,6 +182,8 @@ Bug Fixes
 
 * SOLR-8379: UI Cloud->Tree view now shows .txt files correctly (Alexandre Rafalovitch via janhoy)
 
+* SOLR-9308: Fix distributed RTG to forward request params, fixes fq and non-default fl params (hossman)
+
 Optimizations
 ----------------------
 

solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java

@@ -249,7 +249,8 @@ public class RealTimeGetComponent extends SearchComponent
          docid = segid + ctx.docBase;
 
          if (rb.getFilters() != null) {
-           for (Query q : rb.getFilters()) {
+           for (Query raw : rb.getFilters()) {
+             Query q = raw.rewrite(searcher.getIndexReader());
              Scorer scorer = searcher.createWeight(q, false, 1f).scorer(ctx);
              if (scorer == null || segid != scorer.iterator().advance(segid)) {
                // filter doesn't match.
@@ -448,7 +449,7 @@ public class RealTimeGetComponent extends SearchComponent
     ZkController zkController = rb.req.getCore().getCoreDescriptor().getCoreContainer().getZkController();
 
     // if shards=... then use that
-    if (zkController != null && params.get("shards") == null) {
+    if (zkController != null && params.get(ShardParams.SHARDS) == null) {
       CloudDescriptor cloudDescriptor = rb.req.getCore().getCoreDescriptor().getCloudDescriptor();
 
       String collection = cloudDescriptor.getCollectionName();
@@ -470,32 +471,18 @@ public class RealTimeGetComponent extends SearchComponent
 
       for (Map.Entry<String,List<String>> entry : sliceToId.entrySet()) {
         String shard = entry.getKey();
-        String shardIdList = StrUtils.join(entry.getValue(), ',');
 
-        ShardRequest sreq = new ShardRequest();
+        ShardRequest sreq = createShardRequest(rb, entry.getValue());
-
-        sreq.purpose = 1;
         // sreq.shards = new String[]{shard};    // TODO: would be nice if this would work...
         sreq.shards = sliceToShards(rb, collection, shard);
         sreq.actualShards = sreq.shards;
-        sreq.params = new ModifiableSolrParams();
-        sreq.params.set(ShardParams.SHARDS_QT,"/get");      // TODO: how to avoid hardcoding this and hit the same handler?
-        sreq.params.set("distrib",false);
-        sreq.params.set("ids", shardIdList);
         
         rb.addRequest(this, sreq);
       }      
     } else {
-      String shardIdList = StrUtils.join(reqIds.allIds, ',');
+      ShardRequest sreq = createShardRequest(rb, reqIds.allIds);
-      ShardRequest sreq = new ShardRequest();
-
-      sreq.purpose = 1;
       sreq.shards = null;  // ALL
       sreq.actualShards = sreq.shards;
-      sreq.params = new ModifiableSolrParams();
-      sreq.params.set(ShardParams.SHARDS_QT,"/get");      // TODO: how to avoid hardcoding this and hit the same handler?
-      sreq.params.set("distrib",false);
-      sreq.params.set("ids", shardIdList);
 
       rb.addRequest(this, sreq);
     }
@@ -503,6 +490,28 @@ public class RealTimeGetComponent extends SearchComponent
     return ResponseBuilder.STAGE_DONE;
   }
 
+  /**
+   * Helper method for creating a new ShardRequest for the specified ids, based on the params 
+   * specified for the current request.  The new ShardRequest does not yet know anything about 
+   * which shard/slice it will be sent to.
+   */
+  private ShardRequest createShardRequest(final ResponseBuilder rb, final List<String> ids) {
+    final ShardRequest sreq = new ShardRequest();
+    sreq.purpose = 1;
+    sreq.params = new ModifiableSolrParams(rb.req.getParams());
+
+    // TODO: how to avoid hardcoding this and hit the same handler?
+    sreq.params.set(ShardParams.SHARDS_QT,"/get");      
+    sreq.params.set("distrib",false);
+
+    sreq.params.remove(ShardParams.SHARDS);
+    sreq.params.remove("id");
+    sreq.params.remove("ids");
+    sreq.params.set("ids", StrUtils.join(ids, ','));
+    
+    return sreq;
+  }
+  
   private String[] sliceToShards(ResponseBuilder rb, String collection, String slice) {
     String lookup = collection + '_' + slice;  // seems either form may be filled in rb.slices?
     

solr/core/src/test/org/apache/solr/cloud/TestCloudPseudoReturnFields.java

@@ -51,6 +51,7 @@ import org.apache.lucene.util.TestUtil;
 import org.apache.commons.lang.StringUtils;
 
 import org.junit.AfterClass;
+import org.junit.Before;
 import org.junit.BeforeClass;
 
 /** 
@@ -104,7 +105,13 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
     assertEquals(0, CLOUD_CLIENT.add(sdoc("id", "46", "val_i", "3", "ssto", "X", "subject", "ggg")).getStatus());
     assertEquals(0, CLOUD_CLIENT.commit().getStatus());;
     
-    // uncommitted doc in transaction log
+  }
+  
+  @Before
+  private void addUncommittedDoc99() throws Exception {
+    // uncommitted doc in transaction log at start of every test
+    // Even if an RTG causes ulog to re-open realtime searcher, next test method
+    // will get another copy of doc 99 in the ulog
     assertEquals(0, CLOUD_CLIENT.add(sdoc("id", "99", "val_i", "1", "ssto", "X",
                                           "subject", "uncommitted")).getStatus());
   }
@@ -170,13 +177,12 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
     assertEquals(""+doc, 10L, doc.getFieldValue("val2_ss"));
   }
   
-  @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9286")
   public void testMultiValuedRTG() throws Exception {
     SolrDocument doc = null;
 
     // check same results as testMultiValued via RTG (committed doc)
     doc = getRandClient(random()).getById("42", params("fl","val_ss:val_i, val2_ss:10, subject"));
-    assertEquals(""+doc, 2, doc.size());
+    assertEquals(""+doc, 3, doc.size());
     assertEquals(""+doc, 1, doc.getFieldValue("val_ss"));
     assertEquals(""+doc, 10L, doc.getFieldValue("val2_ss"));
     assertEquals(""+doc, "aaa", doc.getFieldValue("subject"));
@@ -219,6 +225,21 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
     }
   }
   
+  public void testFilterAndOneRealFieldRTG() throws Exception {
+    SolrParams params = params("fl","id,val_i",
+                               "fq","{!field f='subject' v=$my_var}",
+                               "my_var","uncommitted");
+    SolrDocumentList docs = getRandClient(random()).getById(Arrays.asList("42","99"), params);
+    final String msg = params + " => " + docs;
+    assertEquals(msg, 1, docs.size());
+    assertEquals(msg, 1, docs.getNumFound());
+    
+    SolrDocument doc = docs.get(0);
+    assertEquals(msg, 2, doc.size());
+    assertEquals(msg, "99", doc.getFieldValue("id"));
+    assertEquals(msg, 1, doc.getFieldValue("val_i"));
+  }
+
   public void testScoreAndAllRealFields() throws Exception {
     for (String fl : TestPseudoReturnFields.SCORE_AND_REAL_FIELDS) {
       SolrDocumentList docs = assertSearch(params("q", "*:*", "rows", "10", "fl",fl));
@@ -304,7 +325,6 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
     }
   }
 
-  @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9286")
   public void testFunctionsRTG() throws Exception {
     // if we use RTG (committed or otherwise) functions should behave the same
     for (String id : Arrays.asList("42","99")) {
@@ -334,7 +354,6 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
     }
   }
   
-  @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9286")
   public void testFunctionsAndExplicitRTG() throws Exception {
     // shouldn't matter if we use RTG (committed or otherwise)
     for (String id : Arrays.asList("42","99")) {
@@ -382,7 +401,6 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
     }
   }
 
-  @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9286")
   public void testFunctionsAndScoreRTG() throws Exception {
 
     // if we use RTG (committed or otherwise) score should be ignored
@@ -578,40 +596,35 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
     }
   }
 
-  @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9289")
   public void testDocIdAugmenterRTG() throws Exception {
-    // NOTE: once this test is fixed to pass, testAugmentersRTG should also be updated to test [docid]
+    // for an uncommitted doc, we should get -1
-
-    // TODO: in single node, [docid] is silently ignored for uncommited docs (see SOLR-9288) ...
-    // here we see even more confusing:  [docid] is silently ignored for both committed & uncommited docs
-    
-    // behavior shouldn't matter if we are committed or uncommitted
     for (String id : Arrays.asList("42","99")) {
       SolrDocument doc = getRandClient(random()).getById(id, params("fl","[docid]"));
       String msg = id + ": fl=[docid] => " + doc;
       assertEquals(msg, 1, doc.size());
       assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer);
+      assertTrue(msg, -1 <= ((Integer)doc.getFieldValue("[docid]")).intValue());
     }
   }
   
-  @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9286")
   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
-      for (SolrParams p : Arrays.asList(params("fl","[shard],[explain],x_alias:[value v=10 t=int]"),
+             (params("fl","[docid],[shard],[explain],x_alias:[value v=10 t=int]"),
-                                        params("fl","[shard]","fl","[explain],x_alias:[value v=10 t=int]"),
+              params("fl","[docid],[shard]","fl","[explain],x_alias:[value v=10 t=int]"),
-                                        params("fl","[shard]","fl","[explain]","fl","x_alias:[value v=10 t=int]"))) {
+              params("fl","[docid]","fl","[shard]","fl","[explain]","fl","x_alias:[value v=10 t=int]"))) {
         
         SolrDocument doc = getRandClient(random()).getById(id, p);
         String msg = id + ": " + p + " => " + doc;
         
-        assertEquals(msg, 2, doc.size());
+        assertEquals(msg, 3, doc.size());
-        // assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer); // TODO
         assertTrue(msg, doc.getFieldValue("[shard]") instanceof String);
         // RTG: [explain] should be ignored
         assertTrue(msg, doc.getFieldValue("x_alias") instanceof Integer);
         assertEquals(msg, 10, doc.getFieldValue("x_alias"));
+        assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer);
+        assertTrue(msg, -1 <= ((Integer)doc.getFieldValue("[docid]")).intValue());
       }
     }
   }
@@ -635,23 +648,22 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
     }
   }
   
-  @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9286")
   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,[docid],[explain],x_alias:[value v=10 t=int]"),
-      for (SolrParams p : Arrays.asList(params("fl","id,[explain],x_alias:[value v=10 t=int]"),
+                                        params("fl","id,[docid]","fl","[explain],x_alias:[value v=10 t=int]"),
-                                        params("fl","id","fl","[explain],x_alias:[value v=10 t=int]"),
+                                        params("fl","id","fl","[docid]","fl","[explain]","fl","x_alias:[value v=10 t=int]"))) {
-                                        params("fl","id","fl","[explain]","fl","x_alias:[value v=10 t=int]"))) {
         SolrDocument doc = getRandClient(random()).getById(id, p);
         String msg = id + ": " + p + " => " + doc;
         
-        assertEquals(msg, 2, doc.size());
+        assertEquals(msg, 3, doc.size());
         assertTrue(msg, doc.getFieldValue("id") instanceof String);
-        // assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer); // TODO
         // RTG: [explain] should be missing (ignored)
         assertTrue(msg, doc.getFieldValue("x_alias") instanceof Integer);
         assertEquals(msg, 10, doc.getFieldValue("x_alias"));
+        assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer);
+        assertTrue(msg, -1 <= ((Integer)doc.getFieldValue("[docid]")).intValue());
       }
     }
   }
@@ -688,32 +700,29 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
     }
   }
   
-  @AwaitsFix(bugUrl="https://issues.apache.org/jira/browse/SOLR-9286")
   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.
       SolrDocument doc = getRandClient(random()).getById(id, params("fl","x_alias:[value v=10 t=int],score"));
       String msg = id + " => " + doc;
       
       assertEquals(msg, 1, doc.size());
-      // assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer); // TODO
       assertTrue(msg, doc.getFieldValue("x_alias") instanceof Integer);
       assertEquals(msg, 10, doc.getFieldValue("x_alias"));
 
-      for (SolrParams p : Arrays.asList(params("fl","x_alias:[value v=10 t=int],[explain],score"),
+      for (SolrParams p : Arrays.asList(params("fl","d_alias:[docid],x_alias:[value v=10 t=int],[explain],score"),
-                                        params("fl","x_alias:[value v=10 t=int],[explain]","fl","score"),
+                                        params("fl","d_alias:[docid],x_alias:[value v=10 t=int],[explain]","fl","score"),
-                                        params("fl","x_alias:[value v=10 t=int]","fl","[explain]","fl","score"))) {
+                                        params("fl","d_alias:[docid]","fl","x_alias:[value v=10 t=int]","fl","[explain]","fl","score"))) {
         
         doc = getRandClient(random()).getById(id, p);
         msg = id + ": " + p + " => " + doc;
         
-        assertEquals(msg, 1, doc.size());
+        assertEquals(msg, 2, doc.size());
-        assertTrue(msg, doc.getFieldValue("id") instanceof String);
-        // assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer); // TODO
         assertTrue(msg, doc.getFieldValue("x_alias") instanceof Integer);
         assertEquals(msg, 10, doc.getFieldValue("x_alias"));
         // RTG: [explain] and score should be missing (ignored)
+        assertTrue(msg, doc.getFieldValue("d_alias") instanceof Integer);
+        assertTrue(msg, -1 <= ((Integer)doc.getFieldValue("d_alias")).intValue());
       }
     }
   }
@@ -758,8 +767,7 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
 
     // NOTE: 'ssto' is the missing one
     final List<String> fl = Arrays.asList
-      // NOTE: once testDocIdAugmenterRTG can pass, [docid] should be tested here as well.
+      ("id","[docid]","[explain]","score","val_*","subj*");
-      ("id","[explain]","score","val_*","subj*");
     
     final int iters = atLeast(random, 10);
     for (int i = 0; i< iters; i++) {
@@ -778,12 +786,13 @@ public class TestCloudPseudoReturnFields extends SolrCloudTestCase {
           SolrDocument doc = getRandClient(random()).getById(id, params);
           String msg = id + ": " + params + " => " + doc;
         
-          assertEquals(msg, 3, doc.size());
+          assertEquals(msg, 4, doc.size());
           assertTrue(msg, doc.getFieldValue("id") instanceof String);
-          // assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer); // TODO
           assertTrue(msg, doc.getFieldValue("val_i") instanceof Integer);
           assertEquals(msg, 1, doc.getFieldValue("val_i"));
           assertTrue(msg, doc.getFieldValue("subject") instanceof String);
+          assertTrue(msg, doc.getFieldValue("[docid]") instanceof Integer);
+          assertTrue(msg, -1 <= ((Integer)doc.getFieldValue("[docid]")).intValue());
           // RTG: [explain] and score should be missing (ignored)
         }
       }

solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java

@@ -95,8 +95,15 @@ public class TestRandomFlRTGCloud extends SolrCloudTestCase {
       new GlobValidator("*_i"),
       new GlobValidator("*_s"),
       new GlobValidator("a*"),
+      new DocIdValidator(),
+      new DocIdValidator("my_docid_alias"),
       new SimpleFieldValueValidator("aaa_i"),
       new SimpleFieldValueValidator("ccc_s"),
+      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"),
       new NotIncludedValidator("bogus_unused_field_ss"),
       new NotIncludedValidator("bogus_alias","bogus_alias:other_bogus_field_i"),
       new NotIncludedValidator("explain_alias","explain_alias:[explain]"),
@@ -111,20 +118,12 @@ public class TestRandomFlRTGCloud extends SolrCloudTestCase {
     //  - 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
+      // No-Op
-      FL_VALIDATORS.addAll(Arrays.asList
+      // At the moment, there are no known transformers that (we have FlValidators for and) only
-                           (new FunctionValidator("aaa_i"), // fq field
+      // work in single core mode.
-                            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")));
-      // 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
-
     }
     // TODO: SOLR-9314: programatically compare FL_VALIDATORS with all known transformers.
     // (ala QueryEqualityTest) can't be done until we eliminate the need for "singleCodeMode"
@@ -301,15 +300,11 @@ public class TestRandomFlRTGCloud extends SolrCloudTestCase {
     // 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 = usually() ? null : random().nextInt();
-    // // random fq -- nothing fancy, secondary concern for our test
+    if (null != FQ_MAX) {
-    final Integer FQ_MAX = null;                                           // TODO: replace this...
+      params.add("fq", "aaa_i:[* TO " + FQ_MAX + "]");
-    // 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<FlValidator> validators = new HashSet<>();
     validators.add(ID_VALIDATOR); // always include id so we can be confident which doc we're looking at

solr/core/src/test/org/apache/solr/cloud/TestStressCloudBlindAtomicUpdates.java

@@ -320,7 +320,7 @@ public class TestStressCloudBlindAtomicUpdates extends SolrCloudTestCase {
       // sometimes include an fq on the expected value to ensure the updated values
       // are "visible" for searching
       final SolrParams p = (0 != TestUtil.nextInt(random(), 0,15))
-        ? params() : params("fq",numericFieldName + ":" + expect);
+        ? params() : params("fq",numericFieldName + ":\"" + expect + "\"");
       SolrDocument doc = getRandClient(random()).getById(docId, p);
       
       final boolean foundWithFilter = (null != doc);

solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java

@@ -30,6 +30,7 @@ import org.apache.solr.common.params.ModifiableSolrParams;
 
 import org.apache.commons.lang.StringUtils;
 
+import org.junit.Before;
 import org.junit.BeforeClass;
 
 
@@ -61,9 +62,14 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 {
     assertU(adoc("id", "46", "val_i", "3", "ssto", "X", "subject", "ggg"));
     assertU(commit());
 
-    // uncommitted doc in transaction log
+  }
-    assertU(adoc("id", "99", "val_i", "1", "ssto", "X", "subject", "uncommitted"));
   
+  @Before
+  private void addUncommittedDoc99() throws Exception {
+    // uncommitted doc in transaction log at start of every test
+    // Even if an RTG causes ulog to re-open realtime searcher, next test method
+    // will get another copy of doc 99 in the ulog
+    assertU(adoc("id", "99", "val_i", "1", "ssto", "X", "subject", "uncommitted"));
   }
 
   public void testMultiValued() throws Exception {
@@ -140,8 +146,19 @@ public class TestPseudoReturnFields extends SolrTestCaseJ4 {
                 );
       }
     }
+  }
   
-    
+  public void testFilterAndOneRealFieldRTG() throws Exception {
+    // shouldn't matter if we use RTG (committed or otherwise)
+    // only one of these docs should match...
+    assertQ("RTG w/ 2 ids & fq that only matches 1 uncommitted doc",
+            req("qt","/get","ids","42,99", "wt","xml","fl","id,val_i",
+                "fq","{!field f='subject' v=$my_var}","my_var","uncommitted")
+            ,"//result[@numFound='1']"
+            ,"//result/doc/str[@name='id'][.='99']"
+            ,"//result/doc/int[@name='val_i'][.='1']"
+            ,"//result/doc[count(*)=2]"
+            );
   }
 
   public void testScoreAndAllRealFields() throws Exception {