lucene-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From hoss...@apache.org
Subject [2/2] lucene-solr:branch_6x: SOLR-9288: Fix [docid] transformer to return -1 when used in RTG with uncommitted doc
Date Tue, 19 Jul 2016 17:51:53 GMT
SOLR-9288: Fix [docid] transformer to return -1 when used in RTG with uncommitted doc

(cherry picked from commit 08019f42889a537764384429c4184515d233a2cb)


Project: http://git-wip-us.apache.org/repos/asf/lucene-solr/repo
Commit: http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/9feeee4e
Tree: http://git-wip-us.apache.org/repos/asf/lucene-solr/tree/9feeee4e
Diff: http://git-wip-us.apache.org/repos/asf/lucene-solr/diff/9feeee4e

Branch: refs/heads/branch_6x
Commit: 9feeee4e2764add63afab8f5b3784274f300a94f
Parents: dfa3f61
Author: Chris Hostetter <hossman@apache.org>
Authored: Tue Jul 19 10:50:02 2016 -0700
Committer: Chris Hostetter <hossman@apache.org>
Committed: Tue Jul 19 10:51:25 2016 -0700

----------------------------------------------------------------------
 solr/CHANGES.txt                                |  2 +
 .../transform/DocIdAugmenterFactory.java        | 10 ++--
 .../apache/solr/cloud/TestRandomFlRTGCloud.java | 61 +++++++++++++++++++-
 .../solr/search/TestPseudoReturnFields.java     | 60 ++++++++-----------
 4 files changed, 90 insertions(+), 43 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/9feeee4e/solr/CHANGES.txt
----------------------------------------------------------------------
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 1e9c775..4fe3ca2 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
 ----------------------
 

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/9feeee4e/solr/core/src/java/org/apache/solr/response/transform/DocIdAugmenterFactory.java
----------------------------------------------------------------------
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 2f037c9..e95ac1e 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 <code>[docid]</code> integer containing it's
current
+ * (internal) id in the lucene index.  May be <code>-1</code> 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 );
   }
 }
 

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/9feeee4e/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java
----------------------------------------------------------------------
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 8cf1129..682d6a0 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.<FlValidator>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 <em>not</em> 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 <code>false</code>
+     *
+     * @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 <code>[docid]</code> 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 <code>-1</code> -- 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 <code>0</code> 
+   */
+  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<String> assertRTGResults(final Collection<FlValidator>
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.<String>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<String> assertRTGResults(final Collection<FlValidator>
validators,
                                                final SolrInputDocument expected,

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/9feeee4e/solr/core/src/test/org/apache/solr/search/TestPseudoReturnFields.java
----------------------------------------------------------------------
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 68f0773..87f3d89 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<String> 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]"
                   );
         }
       }


Mime
View raw message