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

[#7101] Fixing the GeoTileIT#testMultivaluedGeoPointsAggregation testcase #7166

Merged
merged 1 commit into from
Apr 17, 2023

Conversation

navneet1v
Copy link
Contributor

Description

Fixing the GeoTileIT#testMultivaluedGeoPointsAggregation testcase

The issue was happening because we encode the GeoPoint as long and error comes in the precision due to that encoding. The error was not taken care while generating the exepected tiles count for expected output. The error was happening for a tile: 13/1627/7465

Solution

The idea to fix this is simple enough, while generating the expected buckets for geo-points I will encode and decode the lat-lon values. This will make sure that the precision loss comes in during the Tile creation, as showed in the below snippet.

Code ref:

protected Set<String> generateBucketsForGeoPoint(final GeoPoint geoPoint) {
Set<String> buckets = new HashSet<>();
for (int precision = GEOPOINT_MAX_PRECISION; precision > 0; precision--) {
final String tile = GeoTileUtils.stringEncode(geoPoint.getLon(), geoPoint.getLat(), precision);
buckets.add(tile);
}
return buckets;

protected Set<String> generateBucketsForGeoPoint(final GeoPoint geoPoint) {
        Set<String> buckets = new HashSet<>();
        for (int precision = GEOPOINT_MAX_PRECISION; precision > 0; precision--) {
            final String tile =
                    GeoTileUtils.stringEncode(GeoEncodingUtils.decodeLongitude(GeoEncodingUtils.encodeLongitude(geoPoint.getLon())), GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(geoPoint.getLat())), precision);
            buckets.add(tile);
        }
        return buckets;
    }

Tested by running:

./gradlew ':modules:geo:internalClusterTest' --tests "org.opensearch.geo.search.aggregations.bucket.GeoTileGridIT.testMultivaluedGeoPointsAggregation" -Dtests.seed=73A6EB980B2F13B4 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=is -Dtests.timezone=Africa/Addis_Ababa

Issues Resolved

#7101

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@navneet1v
Copy link
Contributor Author

Gradle Check (Jenkins) Run Completed with:

Flaky tests. Issues: https://github.com/opensearch-project/OpenSearch/issues?q=is%3Aissue+is%3Aopen+MixedClusterClientYamlTestSuiteIT

@navneet1v
Copy link
Contributor Author

@nknize can you help review this PR.

Copy link
Member

@dblock dblock 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 fine, but does feel a little hacky. Perhaps, adding a function that rounds the lon/lat explicitly to avoid the precision problem would make it clearer?

@navneet1v
Copy link
Contributor Author

This is fine, but does feel a little hacky. Perhaps, adding a function that rounds the lon/lat explicitly to avoid the precision problem would make it clearer?

Let me abstract this call under a private function, so that this logic is not exposed in the tests. @dblock does that work?

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

…tsAggregation test case.

The issue was happening because we encode the GeoPoint as long and error comes in the precision due to that encoding. The error was not taken care while generating the exepected tiles count for execpected output.

Signed-off-by: Navneet Verma <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link
Collaborator

@nknize nknize left a comment

Choose a reason for hiding this comment

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

Let me abstract this call under a private function,

+1.. we do this on Lucene as a protected Encoder to ensure the expected test coordinates match the quantized spatial values retrieved from the docvalue / index. Once we add support for XYShape we'll want to abstract this in a similar manner.

@navneet1v
Copy link
Contributor Author

Let me abstract this call under a private function,

+1.. we do this on Lucene as a protected Encoder to ensure the expected test coordinates match the quantized spatial values retrieved from the docvalue / index. Once we add support for XYShape we'll want to abstract this in a similar manner.

I have abstracted this atleast for tests.

@navneet1v
Copy link
Contributor Author

@nknize , @dblock can we merge this?

@nknize nknize merged commit 4897582 into opensearch-project:main Apr 17, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-7166-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 48975828fd9a2bc8e5a792b6c0646dd99b75c124
# Push it to GitHub
git push --set-upstream origin backport/backport-7166-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7166-to-2.x.

@navneet1v navneet1v removed the backport 2.x Backport to 2.x branch label Apr 17, 2023
@navneet1v
Copy link
Contributor Author

Removing backport label, as this tests case currently not present in 2.x

austintlee pushed a commit to austintlee/OpenSearch that referenced this pull request Apr 28, 2023
…tsAggregation test case. (opensearch-project#7166)

The issue was happening because we encode the GeoPoint as long and error comes in the precision due to that encoding. The error was not taken care while generating the exepected tiles count for execpected output.

Signed-off-by: Navneet Verma <[email protected]>
heemin32 pushed a commit to heemin32/OpenSearch that referenced this pull request Jun 27, 2023
…tsAggregation test case. (opensearch-project#7166)

The issue was happening because we encode the GeoPoint as long and error comes in the precision due to that encoding. The error was not taken care while generating the exepected tiles count for execpected output.

Signed-off-by: Navneet Verma <[email protected]>
heemin32 pushed a commit to heemin32/OpenSearch that referenced this pull request Jun 28, 2023
…tsAggregation test case. (opensearch-project#7166)

The issue was happening because we encode the GeoPoint as long and error comes in the precision due to that encoding. The error was not taken care while generating the exepected tiles count for execpected output.

Signed-off-by: Navneet Verma <[email protected]>
andrross pushed a commit that referenced this pull request Jun 30, 2023
* Add GeoTile and GeoHash Grid aggregations on GeoShapes. (#5589)

Src files for GeoTile and GeoHash Aggregations on GeoShape with integration
tests.

Signed-off-by: Navneet Verma <[email protected]>

* [opensearch-project/geospatial#212] Fixing the IT for GeoTilesAggrega… (#6120)

Fixing the IT for GeoTilesAggregation.

Signed-off-by: Navneet Verma <[email protected]>

* [#6187, #6222] Fixing the GeoShapes GeoHash and GeoTile Aggregations Integration tests. (#6242)

Changes done:
* Fixed the ArrayIndexOutOfBoundsException.
* Reduced the precision for GeoShapes Aggregation IT testing.

Signed-off-by: Navneet Verma <[email protected]>

* [#7101] Fixing the GeoTileIT#testMultivaluedGeoPointsAggregation test case. (#7166)

The issue was happening because we encode the GeoPoint as long and error comes in the precision due to that encoding. The error was not taken care while generating the exepected tiles count for execpected output.

Signed-off-by: Navneet Verma <[email protected]>

---------

Signed-off-by: Navneet Verma <[email protected]>
Signed-off-by: Heemin Kim <[email protected]>
Co-authored-by: Navneet Verma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Geospatial skip-changelog :test Adding or fixing a test >test-failure Test failure from CI, local build, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants