Copilot commented on code in PR #3923:
URL: https://github.com/apache/solr/pull/3923#discussion_r2591027825
##########
solr/core/src/test/org/apache/solr/search/TestSolr4Spatial2.java:
##########
@@ -551,4 +551,45 @@ public void
testGeodistSortPossibleWithLatLonPointSpatialFieldOrSpatialRecursive
"sort", "min(geodist(),geodist(55.4721936,-2.24703,llp)) asc"),
"/response/docs/[0]/id=='2'");
}
+
+ @Test
+ public void testSOLR18006_GeodistDescWithFilterQuery() throws Exception {
+ // SOLR-18006: geodist() desc sorting causes NPE when spatial query is in
filter query
+ // Reproduction from JIRA issue with exact coordinates and parameters
+ String fieldName = "llp_km";
+
+ // Index sample documents from JIRA issue
+ assertU(adoc("id", "pt-001", fieldName, "48.106651,11.628476"));
+ assertU(adoc("id", "pt-002", fieldName, "48.113089,11.622016"));
+ assertU(adoc("id", "pt-003", fieldName, "48.137154,11.576124"));
+ assertU(adoc("id", "pt-004", fieldName, "48.135125,11.581981"));
+ assertU(adoc("id", "pt-005", fieldName, "48.121,11.612"));
+ assertU(adoc("id", "pt-006", fieldName, "48.09,11.64"));
+ assertU(commit());
+
+ // Test descending sort with filter query - exact query from JIRA that
triggers NPE
+ // Expected order by distance DESC from
pt=48.11308880280511,11.622015740056845:
+ // pt-003 (48.137154,11.576124) - farthest
+ // pt-004 (48.135125,11.581981)
+ // pt-005 (48.121,11.612)
+ // pt-001 (48.106651,11.628476)
+ // pt-006 (48.09,11.64)
Review Comment:
The expected order in the comment doesn't match the assertions. According to
the comment, the order should be pt-003, pt-004, pt-005, pt-001, pt-006,
pt-002, but the assertions expect pt-003, pt-004, pt-006, pt-005, pt-001,
pt-002. Either the comment or the assertions need to be corrected to match the
actual calculated distances.
```suggestion
// pt-006 (48.09,11.64)
// pt-005 (48.121,11.612)
// pt-001 (48.106651,11.628476)
```
##########
solr/core/src/java/org/apache/solr/schema/LatLonPointSpatialField.java:
##########
@@ -311,11 +309,27 @@ public String toString() {
@Override
public SortField getSortField(boolean reverse) {
+ // Use the optimized Lucene distance sort
+ SortField distanceSort =
+ LatLonDocValuesField.newDistanceSort(fieldName, queryPoint.getY(),
queryPoint.getX());
+
+ // If descending, we need to wrap it to reverse the sort order
+ // Note: We can't just use super.getSortField(true) because that uses
a different code path
+ // that doesn't work correctly with the spatial filter query context
if (reverse) {
- return super.getSortField(true); // will use an impl that calls
getValues
+ // Create a reverse sort field that delegates to the distance sort
comparator
+ return new SortField(distanceSort.getField(),
distanceSort.getType(), true) {
+ @Override
+ public FieldComparator<?> getComparator(int numHits, Pruning
pruning) {
+ // Get the base comparator (for ascending order)
+ @SuppressWarnings("unchecked")
+ FieldComparator<Double> baseComparator =
+ (FieldComparator<Double>)
distanceSort.getComparator(numHits, pruning);
+ return baseComparator;
+ }
+ };
Review Comment:
The reverse sort implementation doesn't actually reverse the comparator. The
SortField is created with `reverse=true` in the constructor (line 321), but the
overridden `getComparator()` method returns the base comparator from
`distanceSort.getComparator()` without reversing it. This means the sorting
will still be in ascending order instead of descending order.
To fix this, you should either:
1. Use `ReversedSortFieldComparatorSource` or a similar wrapper that
reverses the comparison
2. Wrap the comparator in a ReverseFieldComparator
3. Let the parent SortField class handle the reversal by not overriding
getComparator() in the reverse case
Note that simply setting `reverse=true` in the SortField constructor doesn't
automatically reverse the comparator when you override `getComparator()`.
##########
solr/core/src/java/org/apache/solr/schema/LatLonPointSpatialField.java:
##########
@@ -311,11 +309,27 @@ public String toString() {
@Override
public SortField getSortField(boolean reverse) {
+ // Use the optimized Lucene distance sort
+ SortField distanceSort =
+ LatLonDocValuesField.newDistanceSort(fieldName, queryPoint.getY(),
queryPoint.getX());
+
+ // If descending, we need to wrap it to reverse the sort order
+ // Note: We can't just use super.getSortField(true) because that uses
a different code path
+ // that doesn't work correctly with the spatial filter query context
Review Comment:
This comment is misleading. The issue is not that `super.getSortField(true)`
uses a "different code path that doesn't work correctly with the spatial filter
query context." The actual problem is that when `reverse=true`, the previous
implementation was calling `super.getSortField(true)` which used the
`getValues()` method to create a comparator, but this approach had performance
or correctness issues.
However, the current fix doesn't actually reverse the sort order. The
comment should explain what the actual fix is doing (or will do once corrected).
```suggestion
// Note: We can't just use super.getSortField(true) because that
would use getValues(),
// which has performance or correctness issues for distance sorting.
The current workaround
// wraps the Lucene distance sort in a reverse SortField, but this
does not actually reverse
// the sort order unless the comparator is also reversed. Further
work may be needed to ensure
// correct descending sort behavior.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
