You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by mrkarthik <gi...@git.apache.org> on 2018/02/13 21:29:53 UTC
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
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 <kr...@...>
Date: 2018-02-13T21:26:34Z
SOLR-11731: LatLonPointSpatialField could be improved to return the lat/lon from docValues
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by mrkarthik <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by dsmiley <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/323#discussion_r173838565
--- 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 --
By "What does that translate to in the metric system?" I mean for example how many meters would this translate to with 6 decimal points?
"6 was just based on what i read" read where?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by mrkarthik <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by dsmiley <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/323#discussion_r173906040
--- 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 --
I understand that internally the type is SORTED_NUMERIC either way. But externally (from user's point of view) the visible behavior should be consistent with what would happen if the field were stored. Please ensure this distinction is tested too (if it isn't already)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/323#discussion_r173838087
--- 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 --
I'm not arguing with your choice; just trying to understand.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by dsmiley <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by dsmiley <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by dsmiley <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by dsmiley <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by dsmiley <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by dsmiley <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by dsmiley <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by mrkarthik <gi...@git.apache.org>.
Github user mrkarthik commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/323#discussion_r173911226
--- 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 --
Based on the article it would be close 111mm
https://gis.stackexchange.com/a/8674
https://en.wikipedia.org/wiki/Decimal_degrees
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by mrkarthik <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by mrkarthik <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by mrkarthik <gi...@git.apache.org>.
Github user mrkarthik commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/323#discussion_r173911215
--- 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 --
Yes, I have test case of both store and docValue
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr issue #323: SOLR-11731: LatLonPointSpatialField could be improve...
Posted by mrkarthik <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by mrkarthik <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/lucene-solr/pull/323
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by mrkarthik <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by mrkarthik <gi...@git.apache.org>.
Github user mrkarthik commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/323#discussion_r173832060
--- 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 --
Sure. will add
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by dsmiley <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by dsmiley <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr issue #323: SOLR-11731: LatLonPointSpatialField could be improve...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on the issue:
https://github.com/apache/lucene-solr/pull/323
In general my review here has a higher attention to detail than normal because I think there are some nitty gritty details here that should be explained so that not only me but future reviewers know _why_ we're doing what we're doing. Spatial / numerical stuff demands more attention to detail.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #323: SOLR-11731: LatLonPointSpatialField could be ...
Posted by mrkarthik <gi...@git.apache.org>.
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: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org