Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GeoBoundingBox query should work on bounding box with equal latitude or longitude #85788

Merged
merged 4 commits into from Apr 26, 2022

Conversation

jchrys
Copy link
Contributor

@jchrys jchrys commented Apr 11, 2022

This PR removes the validation checkers for latitude/ longitude equality and adds test cases that covers the changes.

fixes #77717

@cla-checker-service
Copy link

cla-checker-service bot commented Apr 11, 2022

💚 CLA has been signed

@elasticsearchmachine elasticsearchmachine added v8.3.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Apr 11, 2022
@iverase iverase self-assigned this Apr 12, 2022
@iverase iverase added >non-issue :Analytics/Geo Indexing, search aggregations of geo points and shapes labels Apr 12, 2022
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 12, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@iverase
Copy link
Contributor

iverase commented Apr 12, 2022

@elasticmachine test this please

Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great. For completeness, could we add a test on GeoBoundingBoxQueryIntegTestCase?

@jchrys
Copy link
Contributor Author

jchrys commented Apr 12, 2022

This is looking great. For completeness, could we add a test on GeoBoundingBoxQueryIntegTestCase?

Sure.
Let me add a test

@iverase
Copy link
Contributor

iverase commented Apr 22, 2022

Just checking in here, how are you doing with the requested change?

@jchrys
Copy link
Contributor Author

jchrys commented Apr 22, 2022

Hello, @iverase .
Thanks very much for stopping by the issue and my progress.
I added the example test below in the class GeoBoundingBoxQueryIntegTestCase but it failed.
I expected that if I query like the test below, I would get the value in the corresponding point.
However, the test below succeeded in some cases(GeoBoundingBoxQueryLegacyGeoShapeIT,GeoBoundingBoxQueryLegacyGeoShapeWithDocValuesIT) and failed in others(GeoBoundingBoxQueryGeoPointIT,GeoBoundingBoxQueryGeoShapeIT,GeoBoundingBoxQueryGeoShapeWithDocValuesIT).
The actual result was different from what I expected, so I was doing my own research.
But Could I get some help with the test or hint?

    public void testSimpleBoundingBoxTest() throws Exception {
        Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, randomSupportedVersion()).build();
        XContentBuilder xContentBuilder = getMapping();
        assertAcked(prepareCreate("test").setSettings(settings).setMapping(xContentBuilder));
        ensureGreen();

        client().prepareIndex("test")
            .setId("1")
            .setSource(jsonBuilder().startObject().field("name", "New York").field("location", "POINT(-74.0059731 40.7143528)").endObject())
            .get();

        client().admin().indices().prepareRefresh().get();

        SearchResponse searchResponse = client().prepareSearch()
            .setQuery(geoBoundingBoxQuery("location").setCorners(40.7143528, -74.0059731, 40.7143528, -74.0059731))
            .get();
        assertThat(searchResponse.getHits().getTotalHits().value, equalTo(1L));
        assertThat(searchResponse.getHits().getHits().length, equalTo(1));
        for (SearchHit hit : searchResponse.getHits()) {
            assertThat(hit.getId(), equalTo("1"));
        }
    }

@iverase
Copy link
Contributor

iverase commented Apr 25, 2022

Umm, I tried to reproduce it but I cannot. Maybe you need to update your branch with the latest master, there was a change recently that might help here: #85441

@jchrys
Copy link
Contributor Author

jchrys commented Apr 25, 2022

Thank you very much again!
let me check the update and I will test with lastest master 👍

@jchrys
Copy link
Contributor Author

jchrys commented Apr 25, 2022

I rebased PR onto latest master(9ae5885).
and I added tests on GeoBoundingBoxQueryIntegTestCase.

@iverase
Copy link
Contributor

iverase commented Apr 26, 2022

@elasticmachine test this please

@iverase
Copy link
Contributor

iverase commented Apr 26, 2022

It seems some of the checks are failing:

10:25:14 > Task :test:framework:checkstyleMain
10:25:14 [ant:checkstyle] [ERROR] /dev/shm/elastic+elasticsearch+pull-request+part-1/test/framework/src/main/java/org/elasticsearch/search/geo/GeoBoundingBoxQueryIntegTestCase.java:233: Line is longer than 140 characters (found 153). [LineLength]
10:25:14 [ant:checkstyle] [ERROR] /dev/shm/elastic+elasticsearch+pull-request+part-1/test/framework/src/main/java/org/elasticsearch/search/geo/GeoBoundingBoxQueryIntegTestCase.java:258: Line is longer than 140 characters (found 152). [LineLength]
10:25:14 [ant:checkstyle] [ERROR] /dev/shm/elastic+elasticsearch+pull-request+part-1/test/framework/src/main/java/org/elasticsearch/search/geo/GeoBoundingBoxQueryIntegTestCase.java:265: Line is longer than 140 characters (found 154). [LineLength]
10:25:14 [ant:checkstyle] [ERROR] /dev/shm/elastic+elasticsearch+pull-request+part-1/test/framework/src/main/java/org/elasticsearch/search/geo/GeoBoundingBoxQueryIntegTestCase.java:354: Line is longer than 140 characters (found 161). [LineLength]
10:25:14 [ant:checkstyle] [ERROR] /dev/shm/elastic+elasticsearch+pull-request+part-1/test/framework/src/main/java/org/elasticsearch/search/geo/GeoBoundingBoxQueryIntegTestCase.java:358: Line is longer than 140 characters (found 164). [LineLength]

My advise is to run locally on the PR ./gradlew precommit and fix the errors that might come up.

@jchrys
Copy link
Contributor Author

jchrys commented Apr 26, 2022

Oh, I'm sorry.
Let me check it and fix it.

@jchrys
Copy link
Contributor Author

jchrys commented Apr 26, 2022

Hello, @iverase.
I have checked and fixed it.

@iverase
Copy link
Contributor

iverase commented Apr 26, 2022

@elasticmachine test this please

Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@iverase iverase merged commit 4a4f3d4 into elastic:master Apr 26, 2022
@iverase
Copy link
Contributor

iverase commented Apr 26, 2022

Thanks @jchrys for the contribution, the PR is now merged. @glennvill pinging you so you can check the final PR.

@jchrys
Copy link
Contributor Author

jchrys commented Apr 26, 2022

@iverase
Thank you so much for guiding me.
I will keep my eyes on the issues menu and If there are issues that I can contribute, I will actively pick them up and try to contribute.
Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes external-contributor Pull request authored by a developer outside the Elasticsearch team >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GeoBoundingBox query should work on bounding box with equal latitude or longitude
4 participants