Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

package org.opensearch.sql.calcite.remote;

import java.io.IOException;
import org.opensearch.sql.ppl.GeoPointFormatsIT;

public class CalciteGeoPointFormatsIT extends GeoPointFormatsIT {
Expand All @@ -14,17 +13,4 @@ public void init() throws Exception {
super.init();
enableCalcite();
}

@Override
public void testReadingGeoHash() throws IOException {
withFallbackEnabled(
() -> {
try {
super.testReadingGeoHash();
} catch (IOException e) {
throw new RuntimeException(e);
}
},
"Need to support metadata, https://github.com/opensearch-project/sql/issues/3333");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,11 @@ public enum Index {
"dates",
getGeopointIndexMapping(),
"src/test/resources/geopoints.json"),
COMPLEX_GEO(
TestsConstants.TEST_INDEX_COMPLEX_GEO,
"complex_geo",
getComplexGeoIndexMapping(),
"src/test/resources/complex_geo.json"),
STATE_COUNTRY(
TestsConstants.TEST_INDEX_STATE_COUNTRY,
"state_country",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,11 @@ public static String getGeopointIndexMapping() {
return getMappingFile(mappingFile);
}

public static String getComplexGeoIndexMapping() {
String mappingFile = "complex_geo_index_mapping.json";
return getMappingFile(mappingFile);
}

public static String getJsonTestIndexMapping() {
String mappingFile = "json_test_index_mapping.json";
return getMappingFile(mappingFile);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ public class TestsConstants {
public static final String TEST_INDEX_MULTI_NESTED_TYPE = TEST_INDEX + "_multi_nested";
public static final String TEST_INDEX_NESTED_WITH_NULLS = TEST_INDEX + "_nested_with_nulls";
public static final String TEST_INDEX_GEOPOINT = TEST_INDEX + "_geopoint";
public static final String TEST_INDEX_COMPLEX_GEO = TEST_INDEX + "_complex_geo";
public static final String TEST_INDEX_JSON_TEST = TEST_INDEX + "_json_test";
public static final String TEST_INDEX_ALIAS = TEST_INDEX + "_alias";
public static final String TEST_INDEX_FLATTENED_VALUE = TEST_INDEX + "_flattened_value";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package org.opensearch.sql.ppl;

import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_COMPLEX_GEO;
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_GEOPOINT;
import static org.opensearch.sql.util.MatcherUtils.rows;
import static org.opensearch.sql.util.MatcherUtils.schema;
Expand All @@ -14,6 +15,7 @@
import java.io.IOException;
import java.util.Map;
import org.apache.commons.lang3.tuple.Pair;
import org.json.JSONArray;
import org.json.JSONObject;
import org.junit.jupiter.api.Test;
import org.opensearch.sql.sql.GeopointFormatsIT;
Expand All @@ -23,6 +25,7 @@ public class GeoPointFormatsIT extends PPLIntegTestCase {
public void init() throws Exception {
super.init();
loadIndex(Index.GEOPOINTS);
loadIndex(Index.COMPLEX_GEO);
}

@Test
Expand Down Expand Up @@ -51,4 +54,160 @@ public void testReadingGeoHash() throws IOException {
assertEquals(40.71, point.getLeft(), GeopointFormatsIT.TOLERANCE);
assertEquals(74, point.getRight(), GeopointFormatsIT.TOLERANCE);
}

// Complex geo tests - geo points within complex types (Maps)
@Test
public void testGeoPointInSimpleMap() throws IOException {
String query =
String.format(
"search source=%s | where id = '1' | fields location", TEST_INDEX_COMPLEX_GEO);

JSONObject result = executeQuery(query);
verifySchema(result, schema("location", null, "struct"));

// Verify the map contains the geo point properly converted
// Using exact precision from complex_geo.json: {"lat": 47.6062, "lon": -122.3321}
verifyDataRows(
result,
rows(
Map.of(
"name", "Seattle Office",
"point", Map.of("lat", 47.6062, "lon", -122.3321),
"city", "Seattle",
"country", "USA")));
}

@Test
public void testGeoPointInMapWithStringFormat() throws IOException {
String query =
String.format(
"search source=%s | where id = '2' | fields location", TEST_INDEX_COMPLEX_GEO);

JSONObject result = executeQuery(query);
verifySchema(result, schema("location", null, "struct"));

// Verify the map contains geo point parsed from string format
// Using exact precision from complex_geo.json: "35.6762,139.6503"
verifyDataRows(
result,
rows(
Map.of(
"name", "Tokyo Office",
"point", Map.of("lat", 35.6762, "lon", 139.6503),
"city", "Tokyo",
"country", "Japan")));
}

@Test
public void testNestedMapsWithGeoPoints() throws IOException {
String query =
String.format(
"search source=%s | where id = '3' | fields nested_locations", TEST_INDEX_COMPLEX_GEO);

JSONObject result = executeQuery(query);
verifySchema(result, schema("nested_locations", null, "struct"));

// Verify nested structure with multiple geo points
// Using exact precision from complex_geo.json
verifyDataRows(
result,
rows(
Map.of(
"primary",
Map.of(
"office", Map.of("lat", 37.7749, "lon", -122.4194),
"warehouse", Map.of("lat", 37.4419, "lon", -122.143)),
"secondary",
Map.of(
"branch", Map.of("lat", 37.3382, "lon", -121.8863),
"store", Map.of("lat", 37.3688, "lon", -122.0363)))));
}

@Test
public void testNestedMapsWithStringGeoPoints() throws IOException {
String query =
String.format(
"search source=%s | where id = '4' | fields nested_locations", TEST_INDEX_COMPLEX_GEO);

JSONObject result = executeQuery(query);
verifySchema(result, schema("nested_locations", null, "struct"));

// Verify nested structure with geo points in string format
// Using exact precision from complex_geo.json: "40.7128,-74.0060" etc.
verifyDataRows(
result,
rows(
Map.of(
"primary",
Map.of(
"office", Map.of("lat", 40.7128, "lon", -74.006),
"warehouse", Map.of("lat", 40.758, "lon", -73.9855)),
"secondary",
Map.of(
"branch", Map.of("lat", 40.7489, "lon", -73.968),
"store", Map.of("lat", 40.7614, "lon", -73.9776)))));
}

@Test
public void testMultipleOfficesWithGeoPoints() throws IOException {
String query =
String.format(
"search source=%s | where id = '5' | fields multiple_offices", TEST_INDEX_COMPLEX_GEO);

JSONObject result = executeQuery(query);
verifySchema(result, schema("multiple_offices", null, "struct"));

// Verify multiple offices structure
verifyDataRows(
result,
rows(
Map.of(
"headquarters",
Map.of(
"location", Map.of("lat", 51.5074, "lon", -0.1278), "address", "London HQ"),
"regional",
Map.of(
"location",
Map.of("lat", 48.8566, "lon", 2.3522),
"address",
"Paris Regional"))));
}

@Test
public void testGeoHashInMap() throws IOException {
String query =
String.format(
"search source=%s | where id = '6' | fields location", TEST_INDEX_COMPLEX_GEO);

JSONObject result = executeQuery(query);
verifySchema(result, schema("location", null, "struct"));

// Verify geo point converted from geohash "u33dc0cpke7v"
// Using tolerance for geohash conversion which has precision variations
JSONArray dataRows = (JSONArray) result.get("datarows");
JSONObject row = (JSONObject) ((JSONArray) dataRows.get(0)).get(0);

assertEquals("Berlin Office", row.getString("name"));
assertEquals("Berlin", row.getString("city"));
assertEquals("Germany", row.getString("country"));

JSONObject point = row.getJSONObject("point");
double lat = point.getDouble("lat");
double lon = point.getDouble("lon");

// Expected values from geohash decoding with tolerance
assertEquals(52.52003, lat, GeopointFormatsIT.TOLERANCE);
assertEquals(13.40489, lon, GeopointFormatsIT.TOLERANCE);
}

@Test
public void testComplexGeoAllDocumentsQuery() throws IOException {
String query = String.format("search source=%s | fields id | sort id", TEST_INDEX_COMPLEX_GEO);

JSONObject result = executeQuery(query);
verifySchema(result, schema("id", null, "string"));

// Verify all documents are indexed and queryable
verifyDataRows(result, rows("1"), rows("2"), rows("3"), rows("4"), rows("5"), rows("6"));
}
}
12 changes: 12 additions & 0 deletions integ-test/src/test/resources/complex_geo.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{"index": {"_id": "1"}}
{"id": "1", "location": {"name": "Seattle Office", "point": {"lat": 47.6062, "lon": -122.3321}, "city": "Seattle", "country": "USA"}}
{"index": {"_id": "2"}}
{"id": "2", "location": {"name": "Tokyo Office", "point": "35.6762,139.6503", "city": "Tokyo", "country": "Japan"}}
{"index": {"_id": "3"}}
{"id": "3", "nested_locations": {"primary": {"office": {"lat": 37.7749, "lon": -122.4194}, "warehouse": {"lat": 37.4419, "lon": -122.1430}}, "secondary": {"branch": {"lat": 37.3382, "lon": -121.8863}, "store": {"lat": 37.3688, "lon": -122.0363}}}}
{"index": {"_id": "4"}}
{"id": "4", "nested_locations": {"primary": {"office": "40.7128,-74.0060", "warehouse": "40.7580,-73.9855"}, "secondary": {"branch": "40.7489,-73.9680", "store": "40.7614,-73.9776"}}}
{"index": {"_id": "5"}}
{"id": "5", "multiple_offices": {"headquarters": {"location": {"lat": 51.5074, "lon": -0.1278}, "address": "London HQ"}, "regional": {"location": {"lat": 48.8566, "lon": 2.3522}, "address": "Paris Regional"}}}
{"index": {"_id": "6"}}
{"id": "6", "location": {"name": "Berlin Office", "point": "u33dc0cpke7v", "city": "Berlin", "country": "Germany"}}
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
{
"mappings": {
"properties": {
"id": {
"type": "keyword"
},
"location": {
"properties": {
"name": {
"type": "keyword"
},
"point": {
"type": "geo_point"
},
"city": {
"type": "keyword"
},
"country": {
"type": "keyword"
}
}
},
"nested_locations": {
"properties": {
"primary": {
"properties": {
"office": {
"type": "geo_point"
},
"warehouse": {
"type": "geo_point"
}
}
},
"secondary": {
"properties": {
"branch": {
"type": "geo_point"
},
"store": {
"type": "geo_point"
}
}
}
}
},
"multiple_offices": {
"properties": {
"headquarters": {
"properties": {
"location": {
"type": "geo_point"
},
"address": {
"type": "text"
}
}
},
"regional": {
"properties": {
"location": {
"type": "geo_point"
},
"address": {
"type": "text"
}
}
}
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import java.sql.SQLException;
import java.sql.Types;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
import lombok.experimental.UtilityClass;
import org.apache.calcite.avatica.util.ArrayImpl;
import org.apache.calcite.rel.type.RelDataType;
Expand Down Expand Up @@ -128,11 +130,47 @@ public static ExprValue getExprValueFromSqlType(
"Unchecked sql type: {}, return Object type {}",
sqlType,
value.getClass().getTypeName());
return ExprValueUtils.fromObjectValue(value);
return convertComplexValue(value);
}
} catch (SQLException e) {
LOG.error("Error converting SQL type {}: {}", sqlType, e.getMessage());
throw e;
}
}

/**
* Convert complex values like Maps that may contain geo points. This method recursively processes
* Maps to handle nested geo points and converts them to appropriate ExprValue representations.
*/
private static ExprValue convertComplexValue(Object value) {
Object converted = processValue(value);
return ExprValueUtils.fromObjectValue(converted);
}

/**
* Process values recursively, handling geo points and nested maps. Geo points are converted to
* OpenSearchExprGeoPointValue. Maps are recursively processed to handle nested structures.
*/
private static Object processValue(Object value) {
if (value == null) {
return null;
}

if (value instanceof Point) {
Point point = (Point) value;
return new OpenSearchExprGeoPointValue(point.getY(), point.getX());
}
Comment on lines +159 to +162
Copy link
Member

Choose a reason for hiding this comment

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

if the problem is we didn't cover the geo_point nested in a struct, why we need this? shouldn't be handled by https://github.com/opensearch-project/sql/pull/4325/files#diff-979074bb911fcfdb6b2eb25ab8b32da65cc8aa12b43b40b5880b876596f9c8d8R80?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That place can only handle the geo_point type in the 1st level of our mapping/schema. And it has special logic for handling the transformation that Avatica convert point into string.

For geo_point nested in a struct, we need do recursive parsing as it may be multiple-level nested. And when being nested in a struct, seems Avatica won't do that converting.

Copy link
Member

Choose a reason for hiding this comment

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

For geo_point nested in a struct, we need do recursive parsing as it may be multiple-level nested.

L164-L171 is the logic of recursive parsing. question is why we need handle Point in L159-L162? can these lines be deleted? @qianheng-aws

Copy link
Member

@LantaoJin LantaoJin Sep 18, 2025

Choose a reason for hiding this comment

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

I see, the getExprValueFromSqlType cannot handle nested type. So the patch recursively resolves the type in processValue rather than getExprValueFromSqlType.


if (value instanceof Map) {
Map<String, Object> map = (Map<String, Object>) value;
Map<String, Object> convertedMap = new HashMap<>();
for (Map.Entry<String, Object> entry : map.entrySet()) {
convertedMap.put(entry.getKey(), processValue(entry.getValue()));
}
return convertedMap;
}

// For other types, return as-is
return value;
}
}
Loading