From c897917c718eef75d66c5d0006f409d5c95260c7 Mon Sep 17 00:00:00 2001 From: yonik Date: Thu, 28 Apr 2016 15:52:17 -0400 Subject: [PATCH] SOLR-9034: fix atomic updates for copyField w/ docValues --- solr/CHANGES.txt | 3 ++ .../component/RealTimeGetComponent.java | 2 +- .../apache/solr/search/SolrIndexSearcher.java | 16 +++++++ .../solr/collection1/conf/schema.xml | 32 +++++++++----- .../update/processor/AtomicUpdatesTest.java | 44 +++++++++++++++++++ 5 files changed, 85 insertions(+), 12 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 0d69bcac88c..8ac956f9df3 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -159,6 +159,9 @@ Bug Fixes * SOLR-9046: Fix solr.cmd that wrongly assumes Jetty will always listen on 0.0.0.0. (Bram Van Dam, Uwe Schindler) +* SOLR-9034: Atomic updates failed to work when there were copyField targets that had docValues + enabled. (Karthik Ramachandran, Ishan Chattopadhyaya, yonik) + Optimizations ---------------------- * SOLR-8722: Don't force a full ZkStateReader refresh on every Overseer operation. 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 8332c797fa6..0908f00a8a7 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 @@ -342,7 +342,7 @@ public class RealTimeGetComponent extends SearchComponent if (docid < 0) return null; Document luceneDocument = searcher.doc(docid); sid = toSolrInputDocument(luceneDocument, core.getLatestSchema()); - searcher.decorateDocValueFields(sid, docid, searcher.getNonStoredDVs(false)); + searcher.decorateDocValueFields(sid, docid, searcher.getNonStoredDVsWithoutCopyTargets()); } } finally { if (searcherHolder != null) { diff --git a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java index 4c9790a4249..6ff54699a8a 100644 --- a/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java +++ b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java @@ -183,6 +183,9 @@ public class SolrIndexSearcher extends IndexSearcher implements Closeable, SolrI /** Contains the names/patterns of all docValues=true,stored=false,useDocValuesAsStored=true fields in the schema. */ private final Set nonStoredDVsUsedAsStored; + /** Contains the names/patterns of all docValues=true,stored=false fields, excluding those that are copyField targets in the schema. */ + private final Set nonStoredDVsWithoutCopyTargets; + private Collection storedHighlightFieldNames; private DirectoryFactory directoryFactory; @@ -359,6 +362,8 @@ public class SolrIndexSearcher extends IndexSearcher implements Closeable, SolrI final Set nonStoredDVsUsedAsStored = new HashSet<>(); final Set allNonStoredDVs = new HashSet<>(); + final Set nonStoredDVsWithoutCopyTargets = new HashSet<>(); + this.fieldInfos = leafReader.getFieldInfos(); for (FieldInfo fieldInfo : fieldInfos) { final SchemaField schemaField = schema.getFieldOrNull(fieldInfo.name); @@ -367,11 +372,15 @@ public class SolrIndexSearcher extends IndexSearcher implements Closeable, SolrI nonStoredDVsUsedAsStored.add(fieldInfo.name); } allNonStoredDVs.add(fieldInfo.name); + if (!schema.isCopyFieldTarget(schemaField)) { + nonStoredDVsWithoutCopyTargets.add(fieldInfo.name); + } } } this.nonStoredDVsUsedAsStored = Collections.unmodifiableSet(nonStoredDVsUsedAsStored); this.allNonStoredDVs = Collections.unmodifiableSet(allNonStoredDVs); + this.nonStoredDVsWithoutCopyTargets = Collections.unmodifiableSet(nonStoredDVsWithoutCopyTargets); // We already have our own filter cache setQueryCache(null); @@ -876,6 +885,13 @@ public class SolrIndexSearcher extends IndexSearcher implements Closeable, SolrI return onlyUseDocValuesAsStored ? nonStoredDVsUsedAsStored : allNonStoredDVs; } + /** + * Returns an unmodifiable set of names of non-stored docValues fields, except those that are targets of a copy field. + */ + public Set getNonStoredDVsWithoutCopyTargets() { + return nonStoredDVsWithoutCopyTargets; + } + /* ********************** end document retrieval *************************/ //////////////////////////////////////////////////////////////////////////////// diff --git a/solr/core/src/test-files/solr/collection1/conf/schema.xml b/solr/core/src/test-files/solr/collection1/conf/schema.xml index 64bca9a953c..44b3d72a5ab 100644 --- a/solr/core/src/test-files/solr/collection1/conf/schema.xml +++ b/solr/core/src/test-files/solr/collection1/conf/schema.xml @@ -648,20 +648,30 @@ - - - - - - + + + + + + - - - - - + + + + + + + + + + + + + + + text diff --git a/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdatesTest.java b/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdatesTest.java index e1726f82a4b..a91a34b92da 100644 --- a/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdatesTest.java +++ b/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdatesTest.java @@ -43,6 +43,7 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 { h.update("*:*"); assertU(commit()); } + @Test public void testRemove() throws Exception { SolrInputDocument doc; @@ -1065,6 +1066,49 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 { "/response/docs/[0]/multi_ii_dvo/[0]==100", "/response/docs/[0]/multi_ii_dvo/[1]==" + Integer.MAX_VALUE); } + + @Test + public void testAtomicUpdatesOnNonStoredDocValuesCopyField() throws Exception { + assertU(adoc(sdoc("id", 101, "title", "title2", "single_i_dvn", 100))); + assertU(adoc(sdoc("id", 102, "title", "title3", "single_d_dvn", 3.14))); + assertU(adoc(sdoc("id", 103, "single_s_dvn", "abc", "single_i_dvn", 1))); + assertU(commit()); + + // Do each one twice... the first time it will be retrieved from the index, and the second time from the transaction log. + for (int i=0; i<2; i++) { + assertU(adoc(sdoc("id", 101, "title", ImmutableMap.of("set", "newtitle2"), + "single_i_dvn", ImmutableMap.of("inc", 1)))); + assertU(adoc(sdoc("id", 102, "title", ImmutableMap.of("set", "newtitle3"), + "single_d_dvn", ImmutableMap.of("inc", 1)))); + assertU(adoc(sdoc("id", 103, "single_i_dvn", ImmutableMap.of("inc", 1)))); + } + assertU(commit()); + + assertJQ(req("q", "id:101"), + "/response/docs/[0]/id==101", + "/response/docs/[0]/title/[0]=='newtitle2'", + "/response/docs/[0]/single_i_dvn==102"); + + assertJQ(req("q", "id:102"), + 1e-4, + "/response/docs/[0]/id==102", + "/response/docs/[0]/title/[0]=='newtitle3'", + "/response/docs/[0]/single_d_dvn==5.14"); + + assertJQ(req("q", "id:103"), + "/response/docs/[0]/id==103", + "/response/docs/[0]/single_s_dvn=='abc'", + "/response/docs/[0]/single_i_dvn==3"); + + // test that non stored docvalues was carried forward for a non-docvalue update + assertU(adoc(sdoc("id", 103, "single_s_dvn", ImmutableMap.of("set", "abcupdate"), + "single_i_dvn", ImmutableMap.of("set", 5)))); + assertU(commit()); + assertJQ(req("q", "id:103"), + "/response/docs/[0]/id==103", + "/response/docs/[0]/single_s_dvn=='abcupdate'", + "/response/docs/[0]/single_i_dvn==5"); + } @Test public void testInvalidOperation() {