[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
28 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...

computerlove
GitHub user mrkarthik opened a pull request:

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

    SOLR-11731: LatLonPointSpatialField could be improved to return the lat/lon from docValues

   

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

    $ git pull https://github.com/mrkarthik/lucene-solr jira/SOLR-11731

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

    https://github.com/apache/lucene-solr/pull/323.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 #323
   
----
commit 31f69836d45471bb60bb86f1fbb7724700017876
Author: Karthik Ramachandran <kramachandran@...>
Date:   2018-02-13T21:26:34Z

    SOLR-11731: LatLonPointSpatialField could be improved to return the lat/lon from docValues

----


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

computerlove
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/323#discussion_r172255852
 
    --- Diff: solr/core/src/test/org/apache/solr/search/TestSolr4Spatial.java ---
    @@ -157,7 +158,7 @@ public void testIntersectFilter() throws Exception {
       @Test
       public void checkResultFormat() throws Exception {
         //Check input and output format is the same
    -    String IN = "89.9,-130";//lat,lon
    --- End diff --
   
    Curious; was this necessary?  I think it's not.


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r172258894
 
    --- Diff: lucene/core/src/java/org/apache/lucene/geo/GeoEncodingUtils.java ---
    @@ -127,6 +130,10 @@ public static double decodeLatitude(int encoded) {
       public static double decodeLatitude(byte[] src, int offset) {
         return decodeLatitude(NumericUtils.sortableBytesToInt(src, offset));
       }
    +  
    +  public static double decodeLatitudeCeil(long encoded) {
    --- End diff --
   
    @nknize  could you please review this part?  It's not clear to me why GeoEncodingUtils has dual ceil/floor for encode but not for decode.


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r172255035
 
    --- Diff: solr/core/src/test/org/apache/solr/search/TestSolr4Spatial2.java ---
    @@ -122,19 +122,19 @@ public void testRptWithGeometryGeo3dField() throws Exception {
       public void testLatLonRetrieval() throws Exception {
         assertU(adoc("id", "0",
             "llp_1_dv_st", "-75,41",
    -        "llp_1_dv", "-80,20",
    -        "llp_1_dv_dvasst", "10,-30"));
    +        "llp_1_dv", "-80.0,20.0",
    +        "llp_1_dv_dvasst", "40.299599,-74.082728"));
         assertU(commit());
         assertJQ(req("q","*:*", "fl","*"),
             "response/docs/[0]/llp_1_dv_st=='-75,41'",
             // Right now we do not support decoding point value from dv field
    --- End diff --
   
    The above comment is obsolete.
    And why does llp_1_dv not seem to work below?  I believe useDocValuesAsStored defaults to true.


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r172255610
 
    --- Diff: solr/core/src/test/org/apache/solr/search/TestSolr4Spatial2.java ---
    @@ -122,19 +122,19 @@ public void testRptWithGeometryGeo3dField() throws Exception {
       public void testLatLonRetrieval() throws Exception {
         assertU(adoc("id", "0",
             "llp_1_dv_st", "-75,41",
    -        "llp_1_dv", "-80,20",
    -        "llp_1_dv_dvasst", "10,-30"));
    +        "llp_1_dv", "-80.0,20.0",
    +        "llp_1_dv_dvasst", "40.299599,-74.082728"));
         assertU(commit());
         assertJQ(req("q","*:*", "fl","*"),
             "response/docs/[0]/llp_1_dv_st=='-75,41'",
             // Right now we do not support decoding point value from dv field
    -        "!response/docs/[0]/llp_1_dv=='-80,20'",
    -        "!response/docs/[0]/llp_1_dv_dvasst=='10,-30'");
    +        "!response/docs/[0]/llp_1_dv=='-80.0,20.0'",
    +        "response/docs/[0]/llp_1_dv_dvasst=='40.299599,-74.082728'");
         assertJQ(req("q","*:*", "fl","llp_1_dv_st, llp_1_dv, llp_1_dv_dvasst"),
             "response/docs/[0]/llp_1_dv_st=='-75,41'",
             // Even when these fields are specified, we won't return them
    --- End diff --
   
    another obsolete comment.  Don't delete it though, just say we do it by decoding the docValue.
   
    Since we lose some precision on decoding, can we have a test value here that demonstrates that slight difference?


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r172248376
 
    --- Diff: solr/core/src/java/org/apache/solr/schema/LatLonPointSpatialField.java ---
    @@ -71,6 +74,10 @@ protected SpatialStrategy newSpatialStrategy(String fieldName) {
         SchemaField schemaField = schema.getField(fieldName); // TODO change AbstractSpatialFieldType so we get schemaField?
         return new LatLonPointSpatialStrategy(ctx, fieldName, schemaField.indexed(), schemaField.hasDocValues());
       }
    +  
    +  public String geoValueToStringValue(long value) {
    --- End diff --
   
    I'd prefer you rename this to `decodeDocValueToString` and make it static, and add some javadocs on the result format


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r172245429
 
    --- Diff: solr/core/src/java/org/apache/solr/search/SolrDocumentFetcher.java ---
    @@ -485,6 +486,10 @@ private Object decodeDVField(int localId, LeafReader leafReader, String fieldNam
           case SORTED_NUMERIC:
             final SortedNumericDocValues numericDv = leafReader.getSortedNumericDocValues(fieldName);
             if (numericDv != null && numericDv.advance(localId) == localId) {
    +          if (schemaField.getType() instanceof LatLonPointSpatialField) {
    --- End diff --
   
    I think this logic should go further below in the loop since there may be multiple points per document.  Actually we should put this in `decodeNumberFromDV` which is invoked not only from SORTED_NUMERIC but also from NUMERIC.


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r173213905
 
    --- Diff: lucene/core/src/java/org/apache/lucene/geo/GeoEncodingUtils.java ---
    @@ -127,6 +130,10 @@ public static double decodeLatitude(int encoded) {
       public static double decodeLatitude(byte[] src, int offset) {
         return decodeLatitude(NumericUtils.sortableBytesToInt(src, offset));
       }
    +  
    +  public static double decodeLatitudeCeil(long encoded) {
    --- End diff --
   
    Or maybe you know @mrkarthik since you felt this addition here was needed?


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r173216592
 
    --- Diff: lucene/core/src/java/org/apache/lucene/geo/GeoEncodingUtils.java ---
    @@ -127,6 +130,10 @@ public static double decodeLatitude(int encoded) {
       public static double decodeLatitude(byte[] src, int offset) {
         return decodeLatitude(NumericUtils.sortableBytesToInt(src, offset));
       }
    +  
    +  public static double decodeLatitudeCeil(long encoded) {
    --- End diff --
   
    @dsmiley i can add the other method to do the floor.


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r173277621
 
    --- Diff: solr/core/src/java/org/apache/solr/schema/LatLonPointSpatialField.java ---
    @@ -71,6 +74,10 @@ protected SpatialStrategy newSpatialStrategy(String fieldName) {
         SchemaField schemaField = schema.getField(fieldName); // TODO change AbstractSpatialFieldType so we get schemaField?
         return new LatLonPointSpatialStrategy(ctx, fieldName, schemaField.indexed(), schemaField.hasDocValues());
       }
    +  
    +  public String geoValueToStringValue(long value) {
    --- End diff --
   
    will make the change and update the 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 pull request #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r173278281
 
    --- Diff: solr/core/src/test/org/apache/solr/search/TestSolr4Spatial.java ---
    @@ -157,7 +158,7 @@ public void testIntersectFilter() throws Exception {
       @Test
       public void checkResultFormat() throws Exception {
         //Check input and output format is the same
    -    String IN = "89.9,-130";//lat,lon
    --- End diff --
   
    No not required, i will remove the change.


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r173278646
 
    --- Diff: solr/core/src/test/org/apache/solr/search/TestSolr4Spatial2.java ---
    @@ -122,19 +122,19 @@ public void testRptWithGeometryGeo3dField() throws Exception {
       public void testLatLonRetrieval() throws Exception {
         assertU(adoc("id", "0",
             "llp_1_dv_st", "-75,41",
    -        "llp_1_dv", "-80,20",
    -        "llp_1_dv_dvasst", "10,-30"));
    +        "llp_1_dv", "-80.0,20.0",
    +        "llp_1_dv_dvasst", "40.299599,-74.082728"));
         assertU(commit());
         assertJQ(req("q","*:*", "fl","*"),
             "response/docs/[0]/llp_1_dv_st=='-75,41'",
             // Right now we do not support decoding point value from dv field
    --- End diff --
   
    will change the comment, llp_1_dv is not retrieved stored="false" and useDocValuesAsStored="false"


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be improve...

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

    https://github.com/apache/lucene-solr/pull/323
 
    @dsmiley Updated the PR based on the 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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r173524870
 
    --- Diff: solr/core/src/java/org/apache/solr/search/SolrDocumentFetcher.java ---
    @@ -486,16 +486,14 @@ private Object decodeDVField(int localId, LeafReader leafReader, String fieldNam
           case SORTED_NUMERIC:
             final SortedNumericDocValues numericDv = leafReader.getSortedNumericDocValues(fieldName);
             if (numericDv != null && numericDv.advance(localId) == localId) {
    -          if (schemaField.getType() instanceof LatLonPointSpatialField) {
    -            long number = numericDv.nextValue();
    -            return ((LatLonPointSpatialField) schemaField.getType()).geoValueToStringValue(number);
    -          }
               final List<Object> outValues = new ArrayList<>(numericDv.docValueCount());
               for (int i = 0; i < numericDv.docValueCount(); i++) {
                 long number = numericDv.nextValue();
                 Object value = decodeNumberFromDV(schemaField, number, true);
                 // return immediately if the number is not decodable, hence won't return an empty list.
                 if (value == null) return null;
    +            // return the value as "lat, lon" if its not multi-valued
    --- End diff --
   
    This is not consistent with how Solr normally does things.  If the field type is declared as multiValued then we normally always return as a list; otherwise we never do.  Here I think you're making that vary per document dependent on how many values the document has.


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r173526922
 
    --- Diff: solr/core/src/test/org/apache/solr/search/TestSolr4Spatial2.java ---
    @@ -120,21 +120,27 @@ public void testRptWithGeometryGeo3dField() throws Exception {
       
       @Test
       public void testLatLonRetrieval() throws Exception {
    -    assertU(adoc("id", "0",
    -        "llp_1_dv_st", "-75,41",
    -        "llp_1_dv", "-80.0,20.0",
    -        "llp_1_dv_dvasst", "40.299599,-74.082728"));
    +    assertU(adoc("id", "0", "llp_1_dv_st", "-75,41")); // stored
    --- End diff --
   
    Please also test -90 and +90, -180 and +180.  This will help ensure there aren't edge cases (literally) in the DV decoding logic.


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r173525391
 
    --- Diff: solr/core/src/java/org/apache/solr/schema/LatLonPointSpatialField.java ---
    @@ -75,8 +77,16 @@ protected SpatialStrategy newSpatialStrategy(String fieldName) {
         return new LatLonPointSpatialStrategy(ctx, fieldName, schemaField.indexed(), schemaField.hasDocValues());
       }
       
    -  public String geoValueToStringValue(long value) {
    -    return new String(decodeLatitudeCeil(value) + "," + decodeLongitudeCeil(value));
    +  /**
    +   * Converts to "lat, lon"
    +   * @param value Non-null; stored location field data
    +   * @return Non-null; "lat, lon" with 6 decimal point precision
    --- End diff --
   
    Why 6 decimal points?  Is that sufficient to represent the data to as much precision as is decoded?  Perhaps instead of putting the constant '6' in the code, it should be calculated so that we can see how 6 is arrived at.  What does that translate to in the metric system?


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r173526505
 
    --- Diff: solr/core/src/java/org/apache/solr/schema/LatLonPointSpatialField.java ---
    @@ -75,8 +77,16 @@ protected SpatialStrategy newSpatialStrategy(String fieldName) {
         return new LatLonPointSpatialStrategy(ctx, fieldName, schemaField.indexed(), schemaField.hasDocValues());
       }
       
    -  public String geoValueToStringValue(long value) {
    -    return new String(decodeLatitudeCeil(value) + "," + decodeLongitudeCeil(value));
    +  /**
    +   * Converts to "lat, lon"
    +   * @param value Non-null; stored location field data
    +   * @return Non-null; "lat, lon" with 6 decimal point precision
    +   */
    +  public static String decodeDocValueToString(long value) {
    +    double latitudeDecoded = BigDecimal.valueOf(decodeLatitude((int) (value >> 32))).setScale(6, HALF_UP).doubleValue();
    --- End diff --
   
    Lets have some comments explaining why this algorithm here is what it is.  Why HALF_UP?
   
    Can we skip the doubleValue and just do toPlainString (avoiding exponent notation of toString) since we're composing a string in the end?  In other words, lets avoid the pointless double primitive intermediary.


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r173832000
 
    --- Diff: solr/core/src/java/org/apache/solr/search/SolrDocumentFetcher.java ---
    @@ -486,16 +486,14 @@ private Object decodeDVField(int localId, LeafReader leafReader, String fieldNam
           case SORTED_NUMERIC:
             final SortedNumericDocValues numericDv = leafReader.getSortedNumericDocValues(fieldName);
             if (numericDv != null && numericDv.advance(localId) == localId) {
    -          if (schemaField.getType() instanceof LatLonPointSpatialField) {
    -            long number = numericDv.nextValue();
    -            return ((LatLonPointSpatialField) schemaField.getType()).geoValueToStringValue(number);
    -          }
               final List<Object> outValues = new ArrayList<>(numericDv.docValueCount());
               for (int i = 0; i < numericDv.docValueCount(); i++) {
                 long number = numericDv.nextValue();
                 Object value = decodeNumberFromDV(schemaField, number, true);
                 // return immediately if the number is not decodable, hence won't return an empty list.
                 if (value == null) return null;
    +            // return the value as "lat, lon" if its not multi-valued
    --- End diff --
   
    The only reason I had to do this is LatLonDocValuesField type is SORTED_NUMERIC, so even for non-multivalued data, we would be returning an array.  If that is what is excepted?
    If the field is stored then it would return string for non-multi-valued data and string array for multi-valued data.


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r173832025
 
    --- Diff: solr/core/src/java/org/apache/solr/schema/LatLonPointSpatialField.java ---
    @@ -75,8 +77,16 @@ protected SpatialStrategy newSpatialStrategy(String fieldName) {
         return new LatLonPointSpatialStrategy(ctx, fieldName, schemaField.indexed(), schemaField.hasDocValues());
       }
       
    -  public String geoValueToStringValue(long value) {
    -    return new String(decodeLatitudeCeil(value) + "," + decodeLongitudeCeil(value));
    +  /**
    +   * Converts to "lat, lon"
    +   * @param value Non-null; stored location field data
    +   * @return Non-null; "lat, lon" with 6 decimal point precision
    --- End diff --
   
    6 was just based on what i read, For 40.299599,-74.082728 it is 40°17'58.52",74°4'57.79".  I will remove it


---

---------------------------------------------------------------------
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 #323: SOLR-11731: LatLonPointSpatialField could be ...

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

    https://github.com/apache/lucene-solr/pull/323#discussion_r173832038
 
    --- Diff: solr/core/src/java/org/apache/solr/schema/LatLonPointSpatialField.java ---
    @@ -75,8 +77,16 @@ protected SpatialStrategy newSpatialStrategy(String fieldName) {
         return new LatLonPointSpatialStrategy(ctx, fieldName, schemaField.indexed(), schemaField.hasDocValues());
       }
       
    -  public String geoValueToStringValue(long value) {
    -    return new String(decodeLatitudeCeil(value) + "," + decodeLongitudeCeil(value));
    +  /**
    +   * Converts to "lat, lon"
    +   * @param value Non-null; stored location field data
    +   * @return Non-null; "lat, lon" with 6 decimal point precision
    +   */
    +  public static String decodeDocValueToString(long value) {
    +    double latitudeDecoded = BigDecimal.valueOf(decodeLatitude((int) (value >> 32))).setScale(6, HALF_UP).doubleValue();
    --- End diff --
   
    HALF_UP is only for ceil, I will remove the rounding.


---

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

12