Null field when getting point values for geopoint sort

[ajleong623]

Description

This change allows us to skip getting PointValues for fields that are null because null fields cannot hold any values.

Related Issues

Resolves opensearch-project/security#5503 #18420

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

✅ Gradle check result for c94c434: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.80%. Comparing base (d4151d0) to head (686f7e6).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #18843      +/-   ##
============================================
- Coverage     72.87%   72.80%   -0.08%     
+ Complexity    69182    69112      -70     
============================================
  Files          5633     5633              
  Lines        317951   317953       +2     
  Branches      45986    45987       +1     
============================================
- Hits         231706   231471     -235     
- Misses        67438    67762     +324     
+ Partials      18807    18720      -87     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

❕ Gradle check result for de82425: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[owaiskazi19]

@owaiskazi19 right. I did that. But how could I prove that the changes worked other than just testing it manually like that? What I had been doing was running the snapshot after the assemble steps and trying the queries against that.

It's fine if you tested the query manually. Can you provide the request and response sample with your change?

[ajleong623]

@owaiskazi19 I was able to recreate the errors then swapped out the change I made. Here are the results.

Create Index

Request:

PUT https://localhost:9200/geonames

{
    "mappings": {
        "properties": {
            "admin": {
                "type": "keyword"
            },
            "location": {
                "type": "geo_point"
            }
        }
    }
}

Response:

{
    "acknowledged": true,
    "shards_acknowledged": true,
    "index": "geonames"
}

Create documents

Document1:
Request:

POST https://localhost:9200/geonames/_doc/1

{
    "admin": "11",
    "location": {
        "lat": 48.8331,
        "lon": 2.3264
    }
}

Response:

{
    "_index": "geonames",
    "_id": "1",
    "_version": 1,
    "result": "created",
    "_shards": {
        "total": 2,
        "successful": 1,
        "failed": 0
    },
    "_seq_no": 0,
    "_primary_term": 1
}

Document2:
Request:

POST https://localhost:9200/geonames/_doc/2

{
    "admin": "11"
    "location": {
        "lat": 48.8412,
        "lon": 2.3003
    },
}

Response:

{
    "_index": "geonames",
    "_id": "2",
    "_version": 1,
    "result": "created",
    "_shards": {
        "total": 2,
        "successful": 1,
        "failed": 0
    },
    "_seq_no": 1,
    "_primary_term": 1
}

Search 1 Before Change

Request:

POST https://localhost:9200/geonames/_search

{
    "query": {
        "match_all": {}
    },
    "sort": [
        {
            "_geo_distance": {
                "location": [
                    9.227400,
                    49.189800
                ],
                "order": "desc",
                "unit": "km",
                "distance_type": "arc",
                "mode": "min",
                "ignore_unmapped": true
            }
        }
    ],
    "size": 10
}

Response:

{
    "error": {
        "root_cause": [
            {
                "type": "null_pointer_exception",
                "reason": "Cannot invoke \"String.endsWith(String)\" because \"field\" is null"
            }
        ],
        "type": "search_phase_execution_exception",
        "reason": "all shards failed",
        "phase": "query",
        "grouped": true,
        "failed_shards": [
            {
                "shard": 0,
                "index": "geonames",
                "node": "GhX6M2UTTneVM8CDNhESYg",
                "reason": {
                    "type": "null_pointer_exception",
                    "reason": "Cannot invoke \"String.endsWith(String)\" because \"field\" is null"
                }
            }
        ],
        "caused_by": {
            "type": "null_pointer_exception",
            "reason": "Cannot invoke \"String.endsWith(String)\" because \"field\" is null",
            "caused_by": {
                "type": "null_pointer_exception",
                "reason": "Cannot invoke \"String.endsWith(String)\" because \"field\" is null"
            }
        }
    },
    "status": 500
}

Search 2 Before Change

Request:

POST https://localhost:9200/geonames/_search

{
    "query": {
        "bool": {
            "filter": {
                "exists": {
                    "field": "location"
                }
            }
        }
    },
    "sort": [
        {
            "_geo_distance": {
                "location": {
                    "lat": 40.7128,
                    "lon": -74.0060
                },
                "ignore_unmapped": true,
                "order": "desc",
                "unit": "km"
            }
        }
    ],
    "size": 4
}

Response:

{
    "error": {
        "root_cause": [
            {
                "type": "null_pointer_exception",
                "reason": "Cannot invoke \"String.endsWith(String)\" because \"field\" is null"
            }
        ],
        "type": "search_phase_execution_exception",
        "reason": "all shards failed",
        "phase": "query",
        "grouped": true,
        "failed_shards": [
            {
                "shard": 0,
                "index": "geonames",
                "node": "GhX6M2UTTneVM8CDNhESYg",
                "reason": {
                    "type": "null_pointer_exception",
                    "reason": "Cannot invoke \"String.endsWith(String)\" because \"field\" is null"
                }
            }
        ],
        "caused_by": {
            "type": "null_pointer_exception",
            "reason": "Cannot invoke \"String.endsWith(String)\" because \"field\" is null",
            "caused_by": {
                "type": "null_pointer_exception",
                "reason": "Cannot invoke \"String.endsWith(String)\" because \"field\" is null"
            }
        }
    },
    "status": 500
}

Search 1 After Change

Request:

POST https://localhost:9200/geonames/_search

{
    "query": {
        "match_all": {}
    },
    "sort": [
        {
            "_geo_distance": {
                "location": [
                    9.227400,
                    49.189800
                ],
                "order": "desc",
                "unit": "km",
                "distance_type": "arc",
                "mode": "min",
                "ignore_unmapped": true
            }
        }
    ],
    "size": 10
}

Response:

{
    "took": 21,
    "timed_out": false,
    "_shards": {
        "total": 1,
        "successful": 1,
        "skipped": 0,
        "failed": 0
    },
    "hits": {
        "total": {
            "value": 2,
            "relation": "eq"
        },
        "max_score": null,
        "hits": [
            {
                "_index": "geonames",
                "_id": "2",
                "_score": null,
                "_source": {
                    "location": {
                        "lat": 48.8412,
                        "lon": 2.3003
                    },
                    "admin": "11"
                },
                "sort": [
                    506.4845658187347
                ]
            },
            {
                "_index": "geonames",
                "_id": "1",
                "_score": null,
                "_source": {
                    "admin": "11",
                    "location": {
                        "lat": 48.8331,
                        "lon": 2.3264
                    }
                },
                "sort": [
                    504.6993671378117
                ]
            }
        ]
    }
}

Search 2 After Change

Request:

POST https://localhost:9200/geonames/_search

{
    "query": {
        "bool": {
            "filter": {
                "exists": {
                    "field": "location"
                }
            }
        }
    },
    "sort": [
        {
            "_geo_distance": {
                "location": {
                    "lat": 40.7128,
                    "lon": -74.0060
                },
                "ignore_unmapped": true,
                "order": "desc",
                "unit": "km"
            }
        }
    ],
    "size": 4
}

Response:

{
    "took": 233,
    "timed_out": false,
    "_shards": {
        "total": 1,
        "successful": 1,
        "skipped": 0,
        "failed": 0
    },
    "hits": {
        "total": {
            "value": 2,
            "relation": "eq"
        },
        "max_score": null,
        "hits": [
            {
                "_index": "geonames",
                "_id": "1",
                "_score": null,
                "_source": {
                    "admin": "11",
                    "location": {
                        "lat": 48.8331,
                        "lon": 2.3264
                    }
                },
                "sort": [
                    5836.466117653775
                ]
            },
            {
                "_index": "geonames",
                "_id": "2",
                "_score": null,
                "_source": {
                    "location": {
                        "lat": 48.8412,
                        "lon": 2.3003
                    },
                    "admin": "11"
                },
                "sort": [
                    5834.358041392651
                ]
            }
        ]
    }
}

[github-actions]

❌ Gradle check result for a8d94d9: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for 02e7de5: SUCCESS

[ajleong623]

@owaiskazi19 When I stepped through the GeoDistanceSortBuilderIT tests, I was able to hit the path where the field name is null, but it seems like the codecov did not cover that. Are the IT tests not included in the code coverage?

[owaiskazi19]

Are the IT tests not included in the code coverage?

I don't think so. Can you add a UT for it?

[github-actions]

❌ Gradle check result for 0a10fc2: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[ajleong623]

@owaiskazi19 I found a preexisting unit test with ExitableDirectoryReader then added a step to that.

[ajleong623]

@owaiskazi19 let me know if you have any suggestions.

[owaiskazi19]

@owaiskazi19 let me know if you have any suggestions.

restarted gradle check

[github-actions]

❌ Gradle check result for 0a10fc2: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 42b6129: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for 686f7e6: SUCCESS

[owaiskazi19]

@cwperks another look?