-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[#6187, #6222] Fixing the GeoShapes GeoHash and GeoTile Aggregations Integration tests to make them more robust. #6242
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
navneet1v
requested review from
reta,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
gbbafna,
setiah,
kartg,
kotwanikunal,
mch2,
nknize,
owaiskazi19,
adnapibar,
Rishikesh1159,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
tlfeng,
VachaShah and
xuezhou25
as code owners
February 8, 2023 20:09
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Fixing these tests. |
…es GeoHash and GeoTile Aggregations Integration tests. Changes done: * Fixed the ArrayIndexOutOfBoundsException. * Reduced the precision for GeoShapes Aggregation IT testing. Signed-off-by: Navneet Verma <navneev@amazon.com>
Gradle Check (Jenkins) Run Completed with:
|
nknize
reviewed
Feb 9, 2023
...ain/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/GeoShapeCellValues.java
Show resolved
Hide resolved
nknize
approved these changes
Feb 10, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thx!
The backport to
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-6242-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 9386cde55a55d8b4779a0c58ba9a6240d04cbe2c
# Push it to GitHub
git push --set-upstream origin backport/backport-6242-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 |
This was referenced Mar 8, 2023
heemin32
pushed a commit
to heemin32/OpenSearch
that referenced
this pull request
Jun 28, 2023
…es GeoHash and GeoTile Aggregations Integration tests. (opensearch-project#6242) Changes done: * Fixed the ArrayIndexOutOfBoundsException. * Reduced the precision for GeoShapes Aggregation IT testing. Signed-off-by: Navneet Verma <navneev@amazon.com>
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 <navneev@amazon.com> * [opensearch-project/geospatial#212] Fixing the IT for GeoTilesAggrega… (#6120) Fixing the IT for GeoTilesAggregation. Signed-off-by: Navneet Verma <navneev@amazon.com> * [#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 <navneev@amazon.com> * [#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 <navneev@amazon.com> --------- Signed-off-by: Navneet Verma <navneev@amazon.com> Signed-off-by: Heemin Kim <heemin@amazon.com> Co-authored-by: Navneet Verma <navneev@amazon.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixing the GeoShapes GeoHash and GeoTile Aggregations Integration tests to make them more robust.
Signed-off-by: Navneet Verma navneev@amazon.com
The issue was happening few random seeds. For GeoHash aggregations on shape we are getting ArrayIndexOut of bounds exception because we were not resetting the values of SortedNumericDocs leading to access of memory area not initialized by the vectors.
Changes done:
For improving the testing for bounded geoshape aggregation I am creating a new Github issue. #6251
Issues Resolved
#6187, #6222
Testing
Done using the failed gradle flows in the issues.
Check List
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.