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.

Sign up for GitHub

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

SQL: Improve null handling in Geo Functions #40708

Merged
merged 8 commits into from
Apr 10, 2019
Merged

SQL: Improve null handling in Geo Functions #40708

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ab8e2a3
SQL: Improve null handling in Geo Functions
imotov Apr 1, 2019
c7f35ca
Address review comments
imotov Apr 2, 2019
dd3e122
Merge remote-tracking branch 'elastic/geosql' into null-handling-for-…
imotov Apr 2, 2019
4004d7a
Address review comments
imotov Apr 8, 2019
8feb38c
Address final review comments
imotov Apr 9, 2019
284ec3b
Address @costin's review comments
imotov Apr 9, 2019
e49c1de
Merge remote-tracking branch 'elastic/geosql' into null-handling-for-…
imotov Apr 9, 2019
cf96e18
Merge remote-tracking branch 'elastic/geosql' into null-handling-for-…
imotov Apr 9, 2019
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
12 changes: 12 additions & 0 deletions x-pack/plugin/sql/qa/src/main/resources/geo/geosql.csv-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,3 +201,15 @@ SELECT COUNT(*) count, FIRST(region) region FROM geo GROUP BY FLOOR(ST_Distance(
3 |Asia
2 |Asia
;

selectWktToSqlOfNull
SELECT ST_ASWKT(ST_WktToSql(NULL)) shape;
shape:s
null
;

selectWktToSqlOfNull
SELECT ST_Distance(ST_WktToSql(NULL), ST_WktToSQL('POINT (-71 42)')) shape;
shape:d
null
;
55 changes: 29 additions & 26 deletions ...c/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,32 +127,8 @@ private Object unwrapMultiValue(Object values) {
if (values == null) {
return null;
}
if (dataType == DataType.GEO_POINT) {
Object value;
if (values instanceof List && ((List<?>) values).size() == 1) {
value = ((List<?>) values).get(0);
} else {
value = values;
}
try {
GeoPoint geoPoint = GeoUtils.parseGeoPoint(value, true);
return new GeoShape(geoPoint.lon(), geoPoint.lat());
} catch (ElasticsearchParseException ex) {
throw new SqlIllegalArgumentException("Cannot parse geo_point value (returned by [{}])", fieldName);
}
}
if (dataType == DataType.GEO_SHAPE) {
Object value;
if (values instanceof List && ((List<?>) values).size() == 1) {
value = ((List<?>) values).get(0);
} else {
value = values;
}
try {
return new GeoShape(value);
} catch (IOException ex) {
throw new SqlIllegalArgumentException("Cannot read geo_shape value (returned by [{}])", fieldName);
}
if (dataType == DataType.GEO_POINT || dataType == DataType.GEO_SHAPE) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth adding an isGeoBased() method to DataType to perform this double check.

return extractGeoShape(values);
}
if (values instanceof List) {
List<?> list = (List<?>) values;
Expand Down Expand Up @@ -180,6 +156,33 @@ private Object unwrapMultiValue(Object values) {
throw new SqlIllegalArgumentException("Type {} (returned by [{}]) is not supported", values.getClass().getSimpleName(), fieldName);
}

private Object extractGeoShape(Object values) {
Object value;
if (values instanceof List) {
if (((List<?>) values).size() > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the correct approach here. If I'm not mistaken you are extracting here the first value from a field that has an array of geo shapes. SQL has added recently a "multi field leniency" attribute to both REST and drivers support: https://www.elastic.co/guide/en/elasticsearch/reference/6.7/sql-rest.html#sql-rest-fields.
Code-wise, this leniency is handled here and here at extraction time. I think you need to include extractGeoShape() in the array leniency logic probably only in unwrapMultiValue method.

Also, would be good to see some tests with arrays of geo shapes and not only in one-level fields, but also multi nested fields (not the nested field type): field.subfield1.subfield2.my_geo_shapes_array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay and thanks for bringing this to my attention. That's very good point and it turned out to be quite tricky to get right. I am still working on it. I might need to make some changes in master first thought to change the way extractSource handles a.b.c. names. I hope to open this pull request monday.

value = ((List<?>) values).get(0);
} else {
return null;
}
} else {
value = values;
}
if (dataType == DataType.GEO_SHAPE) {
try {
return new GeoShape(value);
} catch (IOException ex) {
throw new SqlIllegalArgumentException("Cannot read geo_shape value (returned by [{}])", fieldName);
}
} else {
try {
GeoPoint geoPoint = GeoUtils.parseGeoPoint(value, true);
return new GeoShape(geoPoint.lon(), geoPoint.lat());
} catch (ElasticsearchParseException ex) {
throw new SqlIllegalArgumentException("Cannot parse geo_point value (returned by [{}])", fieldName);
}
}
}

@SuppressWarnings({ "unchecked", "rawtypes" })
Object extractFromSource(Map<String, Object> map) {
Object value = null;
Expand Down
6 changes: 5 additions & 1 deletion .../java/org/elasticsearch/xpack/sql/expression/function/scalar/geo/StDistanceProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@ protected Object doProcess(Object left, Object right) {
return process(left, right);
}

public static double process(Object source1, Object source2) {
public static Double process(Object source1, Object source2) {
if (source1 == null || source2 == null) {
return null;
}

if (source1 instanceof GeoShape == false) {
throw new SqlIllegalArgumentException("A geo_point or geo_shape with type point is required; received [{}]", source1);
}
Expand Down
23 changes: 23 additions & 0 deletions ...t/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.elasticsearch.test.AbstractWireSerializingTestCase;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.sql.SqlException;
import org.elasticsearch.xpack.sql.expression.function.scalar.geo.GeoShape;
import org.elasticsearch.xpack.sql.type.DataType;
import org.elasticsearch.xpack.sql.util.DateUtils;

Expand Down Expand Up @@ -451,6 +452,28 @@ public void testObjectsForSourceValue() throws IOException {
assertThat(ex.getMessage(), is("Objects (returned by [" + fieldName + "]) are not supported"));
}

public void testGeoShapeExtraction() {
String fieldName = randomAlphaOfLength(5);
FieldHitExtractor fe = new FieldHitExtractor(fieldName, DataType.GEO_SHAPE, UTC, false);
Map<String, Object> map = new HashMap<>();
map.put(fieldName, "POINT (1 2)");
assertEquals(new GeoShape(1, 2), fe.extractFromSource(map));

map = new HashMap<>();
assertNull(fe.extractFromSource(map));
}

public void testGeoPointExtraction() {
String fieldName = randomAlphaOfLength(5);
FieldHitExtractor fe = new FieldHitExtractor(fieldName, DataType.GEO_POINT, UTC, true);
SearchHit hit = new SearchHit(1);
DocumentField field = new DocumentField(fieldName, singletonList("2, 1"));
hit.fields(singletonMap(fieldName, field));
assertEquals(new GeoShape(1, 2), fe.extract(hit));
hit = new SearchHit(1);
assertNull(fe.extract(hit));
}

private FieldHitExtractor getFieldHitExtractor(String fieldName, boolean useDocValue) {
return new FieldHitExtractor(fieldName, null, UTC, useDocValue);
}
Expand Down
13 changes: 5 additions & 8 deletions .../org/elasticsearch/xpack/sql/expression/function/scalar/geo/StDistanceProcessorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ public void testApply() {
assertEquals(GeoUtils.arcDistance(20, 10, 40, 30), (double) result, 0.000001);
}

public void testNullHandling() {
assertNull(new StDistance(EMPTY, l(new GeoShape(1, 2)), l(null)).makePipe().asProcessor().process(null));
assertNull(new StDistance(EMPTY, l(null), l(new GeoShape(1, 2))).makePipe().asProcessor().process(null));
}

public void testTypeCheck() {
SqlIllegalArgumentException siae = expectThrows(SqlIllegalArgumentException.class,
() -> new StDistance(EMPTY, l("foo"), l(new GeoShape(1, 2))).makePipe().asProcessor().process(null));
Expand All @@ -52,14 +57,6 @@ public void testTypeCheck() {
siae = expectThrows(SqlIllegalArgumentException.class,
() -> new StDistance(EMPTY, l(new GeoShape(1, 2)), l("bar")).makePipe().asProcessor().process(null));
assertEquals("A geo_point or geo_shape with type point is required; received [bar]", siae.getMessage());

siae = expectThrows(SqlIllegalArgumentException.class,
() -> new StDistance(EMPTY, l(new GeoShape(1, 2)), l(null)).makePipe().asProcessor().process(null));
assertEquals("A geo_point or geo_shape with type point is required; received [null]", siae.getMessage());

siae = expectThrows(SqlIllegalArgumentException.class,
() -> new StDistance(EMPTY, l(null), l(new GeoShape(1, 2))).makePipe().asProcessor().process(null));
assertEquals("A geo_point or geo_shape with type point is required; received [null]", siae.getMessage());
}

@Override
Expand Down