[CI/CD] Test failure in GeoHashGridIT#testMultivaluedGeoPointsAggregation

[nknize]

Describe the bug
Caught on PR #7094 GeoHashGridIT#testMultivaluedGeoPointsAggregation reproducibly fails.

To Reproduce
Steps to reproduce the behavior:

./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 -Druntime.java=19

Expected behavior
Passing test

[nknize]

/cc @navneet1v

[navneet1v]

@nknize will look into this. This is really interesting to see GeoPoints Aggregation Tests getting failed. Will see if there were some changes in the tests that lead to this failure. because these tests are written when OS got forked from ES.

[navneet1v]

@nknize I found out the issue and will be raising the PR soon. The issue was happening mainly because of the way we encode the GeoPoint and error that comes in the precision due to that encoding. The error was happening for a tile: 13/1627/7465

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.

I just have 1 question, should I do this for GeoHex also? I think I should, but need your input here.

Code ref:

OpenSearch/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java

Lines 160 to 166 in 9386cde

	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;
    }

[navneet1v]

@nknize

I just have 1 question, should I do this for GeoHex also? I think I should, but need your input here.

On doing some more deep-dive what I can see is this encoding of geo points to long values is already happening as part of creating Geohash for a geo-point. Hence the issue will not happen for Geohash buckets and not required for GeoHashAggregationIT.

Code reference:

OpenSearch/libs/geo/src/main/java/org/opensearch/geometry/utils/Geohash.java

Line 298 in 8394f54

long interleaved = encodeLatLon(lat, lon);

OpenSearch/libs/geo/src/main/java/org/opensearch/geometry/utils/Geohash.java

Lines 376 to 381 in 8394f54

	private static long encodeLatLon(final double lat, final double lon) {
	// encode lat/lon flipping the sign bit so negative ints sort before positive ints
	final int latEnc = encodeLatitude(lat) ^ 0x80000000;
	final int lonEnc = encodeLongitude(lon) ^ 0x80000000;
	return BitUtil.interleave(latEnc, lonEnc) >>> 2;
	}

OpenSearch/libs/geo/src/main/java/org/opensearch/geometry/utils/Geohash.java

Lines 384 to 389 in 8394f54

	public static int encodeLatitude(double latitude) {
	// the maximum possible value cannot be encoded without overflow
	if (latitude == 90.0D) {
	latitude = Math.nextDown(latitude);
	}
	return (int) Math.floor(latitude / LAT_DECODE);

Encoding of geo-points in lucene just before storing the points:
https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/geo/GeoEncodingUtils.java#L58-L64

[navneet1v]

PR for the fix: #7166

@nknize

[navneet1v]

PR is merged. Resolving the issue.