[GitHub] lucene-solr pull request #455: SOLR-12638

classic Classic list List threaded Threaded
35 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: SOLR-12638

moshebla
GitHub user moshebla opened a pull request:

    https://github.com/apache/lucene-solr/pull/455

    SOLR-12638

   

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/moshebla/lucene-solr SOLR-12638

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/lucene-solr/pull/455.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #455
   
----
commit 34e2b0b3fac2f8fb6c085fe4db23130a0b634924
Author: Moshe <moshebla@...>
Date:   2018-08-23T08:11:57Z

    SOLR-12638: first unittest

commit ce09bdb208b5cd8d9fb69a7c22377752d3c19200
Author: Moshe <moshebla@...>
Date:   2018-08-30T06:36:32Z

    SOLR-12638: start working on RTG fetch block

commit 1fb442915ed16f6a6d844ebffb89bdfbda641c0d
Author: Moshe <moshebla@...>
Date:   2018-09-06T07:13:16Z

    SOLR-12638: tests for child docs atomic updates

commit a9d759546784d2ed3674bcc0a85342466b7cc9ad
Author: Moshe <moshebla@...>
Date:   2018-09-16T13:10:10Z

    SOLR-12638: edit tests and delete old block by version if exists

commit 8a840a3e56ffc525250389aceae1149eedf336fc
Author: Moshe <moshebla@...>
Date:   2018-09-17T05:16:16Z

    SOLR-12638: minor fixes

commit 5617013725a67002914f59acc064fe09c937c07f
Author: Moshe <moshebla@...>
Date:   2018-09-17T11:42:41Z

    SOLR-12368: separate configs for atomic block update tests

commit 06b3436e929165cf7df68643f480995bbe2e5c76
Author: Moshe <moshebla@...>
Date:   2018-09-17T13:05:09Z

    SOLR-12368: tidy up for PR

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr issue #455: WIP: SOLR-12638

moshebla
Github user moshebla commented on the issue:

    https://github.com/apache/lucene-solr/pull/455
 
    Currently Some Solr Core tests do not pass.
    I will work on those, since this is a WIP.
    The test that concerns me the most is ConvertedLegacyTest, which fails only if VersionInfo#lookupVersion is called.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr issue #455: WIP: SOLR-12638

moshebla
In reply to this post by moshebla
Github user dsmiley commented on the issue:

    https://github.com/apache/lucene-solr/pull/455
 
    ConvertedLegacyTest fails in part because line 306 adds with overwrite=false -- a bit of a dubious Solr feature that is probably not properly compatible with the UpdateLog which makes various assumptions about the uniqueKey being unique.  I'll email the dev list to see what others think but I'm inclined to think overwrite=false ought to be explicitly forbidden with an UpdateLog in place.  That ancient legacy test can use a config that doesn't have an updateLog.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: WIP: SOLR-12638

moshebla
In reply to this post by moshebla
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r218293921
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java ---
    @@ -1184,7 +1196,16 @@ protected boolean versionAdd(AddUpdateCommand cmd) throws IOException {
     
             // TODO: possibly set checkDeleteByQueries as a flag on the command?
             doLocalAdd(cmd);
    -        
    +
    +        if(lastKnownVersion != null && req.getSchema().isUsableForChildDocs() &&
    --- End diff --
   
    This may very well be right but can you tell me why you added this delete here at this line and why the delete is version-dependent?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: WIP: SOLR-12638

moshebla
In reply to this post by moshebla
Github user moshebla commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r219034542
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java ---
    @@ -1184,7 +1196,16 @@ protected boolean versionAdd(AddUpdateCommand cmd) throws IOException {
     
             // TODO: possibly set checkDeleteByQueries as a flag on the command?
             doLocalAdd(cmd);
    -        
    +
    +        if(lastKnownVersion != null && req.getSchema().isUsableForChildDocs() &&
    --- End diff --
   
    This delete ensures the updated block is the latest block, deleting all previous ones.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr issue #455: WIP: SOLR-12638

moshebla
In reply to this post by moshebla
Github user moshebla commented on the issue:

    https://github.com/apache/lucene-solr/pull/455
 
    Currently the following update operations are supported for child documents:
   
    - add
    - set
    - remove


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: WIP: SOLR-12638

moshebla
In reply to this post by moshebla
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r223243871
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java ---
    @@ -461,5 +466,33 @@ private static boolean isChildDoc(Object obj) {
         }
         return objValues.iterator().next() instanceof SolrDocumentBase;
       }
    +
    +  private void removeObj(Collection original, Object toRemove, String fieldName) {
    +    if(isChildDoc(toRemove)) {
    +      removeChildDoc(original, (SolrInputDocument) toRemove);
    +    } else {
    +      original.remove(getNativeFieldValue(fieldName, toRemove));
    +    }
    +  }
    +
    +  private static void removeChildDoc(Collection original, SolrInputDocument docToRemove) {
    +    for(SolrInputDocument doc: (Collection<SolrInputDocument>) original) {
    +      if(isDerivedFromDoc(doc, docToRemove)) {
    +        original.remove(doc);
    --- End diff --
   
    eh... I'm not sure if it's as simple as `original.remove(doc)` since if it was, we wouldn't need the method `isDerivedFromDoc`.  Assuming we do need that "derived" predicate method, then we probably need to call remove on an iterator here.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: WIP: SOLR-12638

moshebla
In reply to this post by moshebla
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r223244186
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java ---
    @@ -461,5 +466,33 @@ private static boolean isChildDoc(Object obj) {
         }
         return objValues.iterator().next() instanceof SolrDocumentBase;
       }
    +
    +  private void removeObj(Collection original, Object toRemove, String fieldName) {
    +    if(isChildDoc(toRemove)) {
    +      removeChildDoc(original, (SolrInputDocument) toRemove);
    +    } else {
    +      original.remove(getNativeFieldValue(fieldName, toRemove));
    +    }
    +  }
    +
    +  private static void removeChildDoc(Collection original, SolrInputDocument docToRemove) {
    +    for(SolrInputDocument doc: (Collection<SolrInputDocument>) original) {
    +      if(isDerivedFromDoc(doc, docToRemove)) {
    +        original.remove(doc);
    +        return;
    +      }
    +    }
    +  }
    +
    +  private static boolean isDerivedFromDoc(SolrInputDocument fullDoc, SolrInputDocument subDoc) {
    +    for(SolrInputField subSif: subDoc) {
    +      String fieldName = subSif.getName();
    +      if(!fullDoc.containsKey(fieldName)) return false;
    +      Collection<Object> fieldValues = subDoc.getFieldValues(fieldName);
    --- End diff --
   
    Can't we get this directly off the subSif via subSif.getValues()?  And why would subSif.getValueCount ever be different than fieldValues.size()?  Comments would help.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: WIP: SOLR-12638

moshebla
In reply to this post by moshebla
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r223244019
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java ---
    @@ -461,5 +466,33 @@ private static boolean isChildDoc(Object obj) {
         }
         return objValues.iterator().next() instanceof SolrDocumentBase;
       }
    +
    +  private void removeObj(Collection original, Object toRemove, String fieldName) {
    +    if(isChildDoc(toRemove)) {
    +      removeChildDoc(original, (SolrInputDocument) toRemove);
    +    } else {
    +      original.remove(getNativeFieldValue(fieldName, toRemove));
    +    }
    +  }
    +
    +  private static void removeChildDoc(Collection original, SolrInputDocument docToRemove) {
    +    for(SolrInputDocument doc: (Collection<SolrInputDocument>) original) {
    +      if(isDerivedFromDoc(doc, docToRemove)) {
    +        original.remove(doc);
    +        return;
    +      }
    +    }
    +  }
    +
    +  private static boolean isDerivedFromDoc(SolrInputDocument fullDoc, SolrInputDocument subDoc) {
    --- End diff --
   
    Was does it mean to be "derived" here?  Perhaps do you mean that subDoc is a partial update?  Javadocs would help.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: WIP: SOLR-12638

moshebla
In reply to this post by moshebla
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r223244909
 
    --- Diff: solr/core/src/test/org/apache/solr/update/processor/AtomicUpdateBlockTest.java ---
    @@ -181,6 +182,180 @@ public void testBlockRealTimeGet() throws Exception {
         );
       }
     
    +  @Test
    +  public void testBlockAtomicSet() throws Exception {
    +    SolrInputDocument doc = sdoc("id", "1",
    +        "cat_ss", new String[] {"aaa", "ccc"},
    +        "child1", Collections.singleton(sdoc("id", "2", "cat_ss", "child"))
    +    );
    +    json(doc);
    +    addDoc(adoc(doc), "nested-rtg");
    +
    +    BytesRef rootDocId = new BytesRef("1");
    +    SolrCore core = h.getCore();
    +    SolrInputDocument block = RealTimeGetComponent.getInputDocument(core, rootDocId, true);
    +    // assert block doc has child docs
    +    assertTrue(block.containsKey("child1"));
    +
    +    assertJQ(req("q","id:1")
    +        ,"/response/numFound==0"
    +    );
    +
    +    // commit the changes
    +    assertU(commit());
    +
    +    SolrInputDocument committedBlock = RealTimeGetComponent.getInputDocument(core, rootDocId, true);
    +    BytesRef childDocId = new BytesRef("2");
    +    // ensure the whole block is returned when resolveBlock is true and id of a child doc is provided
    +    assertEquals(committedBlock.toString(), RealTimeGetComponent.getInputDocument(core, childDocId, true).toString());
    +
    +    assertJQ(req("q","id:1")
    +        ,"/response/numFound==1"
    +    );
    +
    +    assertJQ(req("qt","/get", "id","1", "fl","id, cat_ss, child1, [child]")
    +        ,"=={\"doc\":{'id':\"1\"" +
    +            ", cat_ss:[\"aaa\",\"ccc\"], child1:[{\"id\":\"2\",\"cat_ss\":[\"child\"]}]" +
    +            "       }}"
    +    );
    +
    +    assertU(commit());
    +
    +    assertJQ(req("qt","/get", "id","1", "fl","id, cat_ss, child1, [child]")
    +        ,"=={\"doc\":{'id':\"1\"" +
    +            ", cat_ss:[\"aaa\",\"ccc\"], child1:[{\"id\":\"2\",\"cat_ss\":[\"child\"]}]" +
    +            "       }}"
    +    );
    +
    +    doc = sdoc("id", "1",
    +        "cat_ss", ImmutableMap.of("set", Arrays.asList("aaa", "bbb")),
    --- End diff --
   
    Minor: in cases where the JDK has alternatives to Guava, use them.  Here, it's `Collections.singletonMap` I recall.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: WIP: SOLR-12638

moshebla
In reply to this post by moshebla
Github user moshebla commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r223336215
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java ---
    @@ -461,5 +466,33 @@ private static boolean isChildDoc(Object obj) {
         }
         return objValues.iterator().next() instanceof SolrDocumentBase;
       }
    +
    +  private void removeObj(Collection original, Object toRemove, String fieldName) {
    +    if(isChildDoc(toRemove)) {
    +      removeChildDoc(original, (SolrInputDocument) toRemove);
    +    } else {
    +      original.remove(getNativeFieldValue(fieldName, toRemove));
    +    }
    +  }
    +
    +  private static void removeChildDoc(Collection original, SolrInputDocument docToRemove) {
    +    for(SolrInputDocument doc: (Collection<SolrInputDocument>) original) {
    +      if(isDerivedFromDoc(doc, docToRemove)) {
    +        original.remove(doc);
    +        return;
    +      }
    +    }
    +  }
    +
    +  private static boolean isDerivedFromDoc(SolrInputDocument fullDoc, SolrInputDocument subDoc) {
    +    for(SolrInputField subSif: subDoc) {
    +      String fieldName = subSif.getName();
    +      if(!fullDoc.containsKey(fieldName)) return false;
    +      Collection<Object> fieldValues = subDoc.getFieldValues(fieldName);
    --- End diff --
   
    Oh yes you are correct.
    I will get to this and test this ASAP.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: WIP: SOLR-12638

moshebla
In reply to this post by moshebla
Github user moshebla commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r223355767
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java ---
    @@ -461,5 +466,33 @@ private static boolean isChildDoc(Object obj) {
         }
         return objValues.iterator().next() instanceof SolrDocumentBase;
       }
    +
    +  private void removeObj(Collection original, Object toRemove, String fieldName) {
    +    if(isChildDoc(toRemove)) {
    +      removeChildDoc(original, (SolrInputDocument) toRemove);
    +    } else {
    +      original.remove(getNativeFieldValue(fieldName, toRemove));
    +    }
    +  }
    +
    +  private static void removeChildDoc(Collection original, SolrInputDocument docToRemove) {
    +    for(SolrInputDocument doc: (Collection<SolrInputDocument>) original) {
    +      if(isDerivedFromDoc(doc, docToRemove)) {
    +        original.remove(doc);
    +        return;
    +      }
    +    }
    +  }
    +
    +  private static boolean isDerivedFromDoc(SolrInputDocument fullDoc, SolrInputDocument subDoc) {
    +    for(SolrInputField subSif: subDoc) {
    +      String fieldName = subSif.getName();
    +      if(!fullDoc.containsKey(fieldName)) return false;
    +      Collection<Object> fieldValues = subDoc.getFieldValues(fieldName);
    --- End diff --
   
    fieldValues was supposed to be:
    `fullDoc.getFieldValues(fieldName);`


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: WIP: SOLR-12638

moshebla
In reply to this post by moshebla
Github user moshebla commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r223355907
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java ---
    @@ -461,5 +466,33 @@ private static boolean isChildDoc(Object obj) {
         }
         return objValues.iterator().next() instanceof SolrDocumentBase;
       }
    +
    +  private void removeObj(Collection original, Object toRemove, String fieldName) {
    +    if(isChildDoc(toRemove)) {
    +      removeChildDoc(original, (SolrInputDocument) toRemove);
    +    } else {
    +      original.remove(getNativeFieldValue(fieldName, toRemove));
    +    }
    +  }
    +
    +  private static void removeChildDoc(Collection original, SolrInputDocument docToRemove) {
    +    for(SolrInputDocument doc: (Collection<SolrInputDocument>) original) {
    +      if(isDerivedFromDoc(doc, docToRemove)) {
    +        original.remove(doc);
    +        return;
    +      }
    +    }
    +  }
    +
    +  private static boolean isDerivedFromDoc(SolrInputDocument fullDoc, SolrInputDocument subDoc) {
    --- End diff --
   
    Exactly,
    I will add Javadocs for this.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: WIP: SOLR-12638

moshebla
In reply to this post by moshebla
Github user moshebla commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r223359889
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java ---
    @@ -461,5 +466,33 @@ private static boolean isChildDoc(Object obj) {
         }
         return objValues.iterator().next() instanceof SolrDocumentBase;
       }
    +
    +  private void removeObj(Collection original, Object toRemove, String fieldName) {
    +    if(isChildDoc(toRemove)) {
    +      removeChildDoc(original, (SolrInputDocument) toRemove);
    +    } else {
    +      original.remove(getNativeFieldValue(fieldName, toRemove));
    +    }
    +  }
    +
    +  private static void removeChildDoc(Collection original, SolrInputDocument docToRemove) {
    +    for(SolrInputDocument doc: (Collection<SolrInputDocument>) original) {
    +      if(isDerivedFromDoc(doc, docToRemove)) {
    +        original.remove(doc);
    --- End diff --
   
    I was thinking that if doc {"id": "4"} was supplied, then the **whole** child doc with that id(4) is to be removed.
    Or perhaps you have a different scenario in mind, where a field of a a particular child document is to be removed while the rest of the child document is left intact?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: WIP: SOLR-12638

moshebla
In reply to this post by moshebla
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r223561565
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java ---
    @@ -461,5 +466,33 @@ private static boolean isChildDoc(Object obj) {
         }
         return objValues.iterator().next() instanceof SolrDocumentBase;
       }
    +
    +  private void removeObj(Collection original, Object toRemove, String fieldName) {
    +    if(isChildDoc(toRemove)) {
    +      removeChildDoc(original, (SolrInputDocument) toRemove);
    +    } else {
    +      original.remove(getNativeFieldValue(fieldName, toRemove));
    +    }
    +  }
    +
    +  private static void removeChildDoc(Collection original, SolrInputDocument docToRemove) {
    +    for(SolrInputDocument doc: (Collection<SolrInputDocument>) original) {
    +      if(isDerivedFromDoc(doc, docToRemove)) {
    +        original.remove(doc);
    +        return;
    +      }
    +    }
    +  }
    +
    +  private static boolean isDerivedFromDoc(SolrInputDocument fullDoc, SolrInputDocument subDoc) {
    --- End diff --
   
    I see the new docs; still it's unclear without the context of our conversation.  Should "subDoc" be "partialDoc"?  How is it that the fullDoc is "the document to be tested" when the return value is "wether subDoc is a subset of fullDoc"?  Isn't the subject of the test subDoc?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: SOLR-12638

moshebla
In reply to this post by moshebla
Github user moshebla commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r223741723
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java ---
    @@ -461,5 +466,33 @@ private static boolean isChildDoc(Object obj) {
         }
         return objValues.iterator().next() instanceof SolrDocumentBase;
       }
    +
    +  private void removeObj(Collection original, Object toRemove, String fieldName) {
    +    if(isChildDoc(toRemove)) {
    +      removeChildDoc(original, (SolrInputDocument) toRemove);
    +    } else {
    +      original.remove(getNativeFieldValue(fieldName, toRemove));
    +    }
    +  }
    +
    +  private static void removeChildDoc(Collection original, SolrInputDocument docToRemove) {
    +    for(SolrInputDocument doc: (Collection<SolrInputDocument>) original) {
    +      if(isDerivedFromDoc(doc, docToRemove)) {
    +        original.remove(doc);
    +        return;
    +      }
    +    }
    +  }
    +
    +  private static boolean isDerivedFromDoc(SolrInputDocument fullDoc, SolrInputDocument subDoc) {
    --- End diff --
   
    Just pushed a new commit to address your comments.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: SOLR-12638

moshebla
In reply to this post by moshebla
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r223749656
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/DistributedUpdateProcessor.java ---
    @@ -111,6 +114,8 @@
       public static final String DISTRIB_FROM = "distrib.from";
       public static final String DISTRIB_INPLACE_PREVVERSION = "distrib.inplace.prevversion";
       private static final String TEST_DISTRIB_SKIP_SERVERS = "test.distrib.skip.servers";
    +  private static final char PATH_SEP_CHAR = '/';
    --- End diff --
   
    Please don't create frivolous constants like this.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: SOLR-12638

moshebla
In reply to this post by moshebla
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r224209409
 
    --- Diff: solr/core/src/test/org/apache/solr/update/processor/AtomicUpdateBlockTest.java ---
    @@ -0,0 +1,370 @@
    +/*
    + * 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.update.processor;
    +
    +import java.util.Arrays;
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.List;
    +
    +import org.apache.lucene.util.BytesRef;
    +import org.apache.solr.SolrTestCaseJ4;
    +import org.apache.solr.common.SolrInputDocument;
    +import org.apache.solr.common.SolrInputField;
    +import org.apache.solr.core.SolrCore;
    +import org.apache.solr.handler.component.RealTimeGetComponent;
    +import org.junit.AfterClass;
    +import org.junit.Before;
    +import org.junit.BeforeClass;
    +import org.junit.Test;
    +
    +public class AtomicUpdateBlockTest extends SolrTestCaseJ4 {
    +
    +  private final static String VERSION = "_version_";
    +  private static String PREVIOUS_ENABLE_UPDATE_LOG_VALUE;
    +
    +  @BeforeClass
    +  public static void beforeTests() throws Exception {
    +    PREVIOUS_ENABLE_UPDATE_LOG_VALUE = System.getProperty("enable.update.log");
    +    System.setProperty("enable.update.log", "true");
    +    initCore("solrconfig-block-atomic-update.xml", "schema-nest.xml"); // use "nest" schema
    +  }
    +
    +  @AfterClass
    +  public static void afterTests() throws Exception {
    +    // restore enable.update.log
    +    System.setProperty("enable.update.log", PREVIOUS_ENABLE_UPDATE_LOG_VALUE);
    +  }
    +
    +  @Before
    +  public void before() {
    +    clearIndex();
    +    assertU(commit());
    +  }
    +
    +  @Test
    +  public void testMergeChildDoc() throws Exception {
    +    SolrInputDocument doc = new SolrInputDocument();
    +    doc.setField("id", "1");
    +    doc.setField("cat_ss", new String[]{"aaa", "ccc"});
    +    doc.setField("child", Collections.singletonList(sdoc("id", "2", "cat_ss", "child")));
    +    addDoc(adoc(doc), "nested-rtg");
    +
    +    BytesRef rootDocId = new BytesRef("1");
    +    SolrCore core = h.getCore();
    +    SolrInputDocument block = RealTimeGetComponent.getInputDocument(core, rootDocId, true);
    +    // assert block doc has child docs
    +    assertTrue(block.containsKey("child"));
    +
    +    assertJQ(req("q","id:1")
    +        ,"/response/numFound==0"
    +    );
    +
    +    // commit the changes
    +    assertU(commit());
    +
    +    SolrInputDocument newChildDoc = sdoc("id", "3", "cat_ss", "child");
    +    SolrInputDocument addedDoc = sdoc("id", "1",
    +        "cat_ss", Collections.singletonMap("add", "bbb"),
    +        "child", Collections.singletonMap("add", sdocs(newChildDoc)));
    +    block = RealTimeGetComponent.getInputDocument(core, rootDocId, true);
    +    block.removeField(VERSION);
    +    SolrInputDocument preMergeDoc = new SolrInputDocument(block);
    +    AtomicUpdateDocumentMerger docMerger = new AtomicUpdateDocumentMerger(req());
    +    docMerger.merge(addedDoc, block);
    +    assertEquals("merged document should have the same id", preMergeDoc.getFieldValue("id"), block.getFieldValue("id"));
    +    assertDocContainsSubset(preMergeDoc, block);
    +    assertDocContainsSubset(addedDoc, block);
    +    assertDocContainsSubset(newChildDoc, (SolrInputDocument) ((List) block.getFieldValues("child")).get(1));
    +    assertEquals(doc.getFieldValue("id"), block.getFieldValue("id"));
    +  }
    +
    +  @Test
    +  public void testBlockAtomicAdd() throws Exception {
    +
    +    SolrInputDocument doc = sdoc("id", "1",
    +        "cat_ss", new String[] {"aaa", "ccc"},
    +        "child1", sdoc("id", "2", "cat_ss", "child")
    +    );
    +    json(doc);
    +    addDoc(adoc(doc), "nested-rtg");
    +
    +    BytesRef rootDocId = new BytesRef("1");
    +    SolrCore core = h.getCore();
    +    SolrInputDocument block = RealTimeGetComponent.getInputDocument(core, rootDocId, true);
    +    // assert block doc has child docs
    +    assertTrue(block.containsKey("child1"));
    +
    +    assertJQ(req("q","id:1")
    +        ,"/response/numFound==0"
    +    );
    +
    +    // commit the changes
    +    assertU(commit());
    +
    +    SolrInputDocument committedBlock = RealTimeGetComponent.getInputDocument(core, rootDocId, true);
    +    BytesRef childDocId = new BytesRef("2");
    +    // ensure the whole block is returned when resolveBlock is true and id of a child doc is provided
    +    assertEquals(committedBlock.toString(), RealTimeGetComponent.getInputDocument(core, childDocId, true).toString());
    --- End diff --
   
    Can we avoid the low-level API use and instead stay at the top level (SolrJ) / integration test?  More specifically, can we avoid access to RealTimeGetComponent completely from this test method and the others here?  It's _appears_ we wouldn't lose coverage.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: SOLR-12638

moshebla
In reply to this post by moshebla
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r224131571
 
    --- Diff: solr/core/src/test/org/apache/solr/update/processor/AtomicUpdateBlockTest.java ---
    @@ -0,0 +1,370 @@
    +/*
    + * 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.update.processor;
    +
    +import java.util.Arrays;
    +import java.util.Collection;
    +import java.util.Collections;
    +import java.util.List;
    +
    +import org.apache.lucene.util.BytesRef;
    +import org.apache.solr.SolrTestCaseJ4;
    +import org.apache.solr.common.SolrInputDocument;
    +import org.apache.solr.common.SolrInputField;
    +import org.apache.solr.core.SolrCore;
    +import org.apache.solr.handler.component.RealTimeGetComponent;
    +import org.junit.AfterClass;
    +import org.junit.Before;
    +import org.junit.BeforeClass;
    +import org.junit.Test;
    +
    +public class AtomicUpdateBlockTest extends SolrTestCaseJ4 {
    +
    +  private final static String VERSION = "_version_";
    +  private static String PREVIOUS_ENABLE_UPDATE_LOG_VALUE;
    +
    +  @BeforeClass
    +  public static void beforeTests() throws Exception {
    +    PREVIOUS_ENABLE_UPDATE_LOG_VALUE = System.getProperty("enable.update.log");
    +    System.setProperty("enable.update.log", "true");
    +    initCore("solrconfig-block-atomic-update.xml", "schema-nest.xml"); // use "nest" schema
    +  }
    +
    +  @AfterClass
    +  public static void afterTests() throws Exception {
    +    // restore enable.update.log
    +    System.setProperty("enable.update.log", PREVIOUS_ENABLE_UPDATE_LOG_VALUE);
    +  }
    +
    +  @Before
    +  public void before() {
    +    clearIndex();
    +    assertU(commit());
    +  }
    +
    +  @Test
    +  public void testMergeChildDoc() throws Exception {
    +    SolrInputDocument doc = new SolrInputDocument();
    +    doc.setField("id", "1");
    +    doc.setField("cat_ss", new String[]{"aaa", "ccc"});
    +    doc.setField("child", Collections.singletonList(sdoc("id", "2", "cat_ss", "child")));
    +    addDoc(adoc(doc), "nested-rtg");
    +
    +    BytesRef rootDocId = new BytesRef("1");
    +    SolrCore core = h.getCore();
    +    SolrInputDocument block = RealTimeGetComponent.getInputDocument(core, rootDocId, true);
    +    // assert block doc has child docs
    +    assertTrue(block.containsKey("child"));
    +
    +    assertJQ(req("q","id:1")
    +        ,"/response/numFound==0"
    +    );
    +
    +    // commit the changes
    +    assertU(commit());
    +
    +    SolrInputDocument newChildDoc = sdoc("id", "3", "cat_ss", "child");
    +    SolrInputDocument addedDoc = sdoc("id", "1",
    +        "cat_ss", Collections.singletonMap("add", "bbb"),
    +        "child", Collections.singletonMap("add", sdocs(newChildDoc)));
    +    block = RealTimeGetComponent.getInputDocument(core, rootDocId, true);
    +    block.removeField(VERSION);
    +    SolrInputDocument preMergeDoc = new SolrInputDocument(block);
    +    AtomicUpdateDocumentMerger docMerger = new AtomicUpdateDocumentMerger(req());
    +    docMerger.merge(addedDoc, block);
    +    assertEquals("merged document should have the same id", preMergeDoc.getFieldValue("id"), block.getFieldValue("id"));
    +    assertDocContainsSubset(preMergeDoc, block);
    +    assertDocContainsSubset(addedDoc, block);
    +    assertDocContainsSubset(newChildDoc, (SolrInputDocument) ((List) block.getFieldValues("child")).get(1));
    +    assertEquals(doc.getFieldValue("id"), block.getFieldValue("id"));
    +  }
    +
    +  @Test
    +  public void testBlockAtomicAdd() throws Exception {
    +
    +    SolrInputDocument doc = sdoc("id", "1",
    +        "cat_ss", new String[] {"aaa", "ccc"},
    +        "child1", sdoc("id", "2", "cat_ss", "child")
    +    );
    +    json(doc);
    --- End diff --
   
    accidentally added this json line?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #455: SOLR-12638

moshebla
In reply to this post by moshebla
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/455#discussion_r223747918
 
    --- Diff: solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java ---
    @@ -442,5 +442,58 @@ protected void doRemoveRegex(SolrInputDocument toDoc, SolrInputField sif, Object
         }
         return patterns;
       }
    +
    +  private Object getNativeFieldValue(String fieldName, Object val) {
    +    if(isChildDoc(val)) {
    +      return val;
    +    }
    +    SchemaField sf = schema.getField(fieldName);
    +    return sf.getType().toNativeType(val);
    +  }
    +
    +  private static boolean isChildDoc(Object obj) {
    +    if(!(obj instanceof Collection)) {
    +      return obj instanceof SolrDocumentBase;
    +    }
    +    Collection objValues = (Collection) obj;
    +    if(objValues.size() == 0) {
    +      return false;
    +    }
    +    return objValues.iterator().next() instanceof SolrDocumentBase;
    +  }
    +
    +  private void removeObj(Collection original, Object toRemove, String fieldName) {
    +    if(isChildDoc(toRemove)) {
    +      removeChildDoc(original, (SolrInputDocument) toRemove);
    +    } else {
    +      original.remove(getNativeFieldValue(fieldName, toRemove));
    +    }
    +  }
    +
    +  private static void removeChildDoc(Collection original, SolrInputDocument docToRemove) {
    +    for(SolrInputDocument doc: (Collection<SolrInputDocument>) original) {
    +      if(isDerivedFromDoc(doc, docToRemove)) {
    +        original.remove(doc);
    +        return;
    +      }
    +    }
    +  }
    +
    +  /**
    +   *
    +   * @param fullDoc the document to be tested
    +   * @param subDoc the sub document that should be a subset of fullDoc
    +   * @return whether subDoc is a subset of fullDoc
    +   */
    +  private static boolean isDerivedFromDoc(SolrInputDocument fullDoc, SolrInputDocument subDoc) {
    +    for(SolrInputField subSif: subDoc) {
    +      String fieldName = subSif.getName();
    +      if(!fullDoc.containsKey(fieldName)) return false;
    --- End diff --
   
    This results in a double-lookup of the values with the next line.  Remove this line and after the next one simply do a null-check.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

12