SQL: Improve null handling in Geo Functions #40708

imotov · 2019-04-01T21:44:01Z

ST_Distance function returns null now instead of throwing an error
when one of the arguments in null and cleans up null handling
logic in the field extractor.

@bpintea, could you take a look to see if that would work better for you in terms of null handling?

ST_Distance function returns null now instead of throwing an error when one of the arguments in null.

elasticmachine · 2019-04-01T21:44:03Z

Pinging @elastic/es-analytics-geo

elasticmachine · 2019-04-01T21:44:04Z

Pinging @elastic/es-search

matriv

Could you please also add some unit test in FieldHitExtractorTests ?

matriv · 2019-04-01T21:48:52Z

.../src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java

@@ -143,8 +148,12 @@ private Object unwrapMultiValue(Object values) {
        }
        if (dataType == DataType.GEO_SHAPE) {
            Object value;
-            if (values instanceof List && ((List<?>) values).size() == 1) {
-                value = ((List<?>) values).get(0);
+            if (values instanceof List) {


Could you please extract it to a method as it seems duplicate with the GEO_POINT method?

bpintea

Gave it a try and it LGTM.

matriv

LGTM

…geo-points

astefan

I left one comment about arrays of geo shapes, if my assumption about the code is correct.

astefan · 2019-04-03T10:14:36Z

.../src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java

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


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.

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.

costin

LGTM

costin · 2019-04-07T19:03:56Z

.../src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java

-            } catch (IOException ex) {
-                throw new SqlIllegalArgumentException("Cannot read geo_shape value (returned by [{}])", fieldName);
-            }
+        if (dataType == DataType.GEO_POINT || dataType == DataType.GEO_SHAPE) {


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

@astefan

@astefan thanks a lot for assistance and brainstorming earlier today!

astefan

LGTM. Left few minor comments.

astefan · 2019-04-09T12:23:37Z

.../src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java

+                    value = ((List<?>) values).get(0);
+                } else {
+                    // This could be a single point in [lon, lat] format or multiple points
+                    if ((list.size() == 2 || list.size() == 3) && (list.get(0) instanceof Number)) {


It would be good if you can add a comment explaining the size() == 3 scenario when this can happen. Something like // This could be a single point in [lon, lat(, altitude)] (where altitude is optional) format or multiple points

astefan · 2019-04-09T12:36:04Z

.../src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java

+            try {
+                return new GeoShape(values);
+            } catch (IOException ex) {
+                throw new SqlIllegalArgumentException("Cannot read geo_shape value (returned by [{}])", fieldName);


Please, include the actual value that cannot be read in the error message, to be consistent with the rest of the error messages.

astefan · 2019-04-09T12:36:44Z

...test/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractorTests.java

+            String name = randomAlphaOfLength(10);
+            pathCombined = pathCombined + name;
+            source.field(name, randomPoint(lat, lon));
+            for (int i = layers -2; i >= 0; i--) {


Really small: white space between the minus sign and 2: i = layers - 2.

astefan · 2019-04-09T12:38:27Z

.../src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java

@@ -207,7 +218,7 @@ Object extractFromSource(Map<String, Object> map) {

                if (node instanceof List) {
                    List listOfValues = (List) node;
-                    if (listOfValues.size() == 1 || arrayLeniency) {
+                    if ((i < path.length - 1) && (listOfValues.size() == 1 || arrayLeniency)) {


I would appreciate it if you can add a comment reflecting the reasoning behind this change, in the context of GEO and arrays of values.

costin

I've left some comments about FieldHitExtractor.

costin · 2019-04-09T14:42:52Z

.../src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java

+                    // we get geo_points from doc values and they were not specified for this record
+                    return null;
+                }
+                if (((List<?>) values).size() == 1) {


use list instead of recasting values.

costin · 2019-04-09T14:46:32Z

.../src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java

@@ -142,6 +173,13 @@ private Object unwrapMultiValue(Object values) {
                }
            }
        }
+        if (dataType == DataType.GEO_SHAPE) {


Please move the dataType check below the instanceof checks (like DATETIME) to keep the same style.

I cannot. I has to be before check for map because shape can be a map in case of geojson.

costin · 2019-04-09T14:47:49Z

.../src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java

@@ -127,8 +127,39 @@ private Object unwrapMultiValue(Object values) {
        if (values == null) {
            return null;
        }
-        if (dataType == DataType.GEO_POINT || dataType == DataType.GEO_SHAPE) {
-            return extractGeoShape(values);
+        if (dataType == DataType.GEO_POINT) {


I think it's worth handling the GEO_POINT case inside the instanceof List check since the two IFs share a bit of code.

costin · 2019-04-09T14:48:45Z

.../src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java

+                        value = values;
+                    } else {
+                        // most likely we have multiple points
+                        if (arrayLeniency) {


This looks similar to the if inside instanceof List check below no?

…geo-points

imotov · 2019-04-09T19:11:15Z

@elasticmachine run elasticsearch-ci/2

…geo-points

imotov · 2019-04-10T16:07:10Z

Thanks a lot everyone for the reviews! That was really helpful.

SQL: Improve null handling in Geo Functions

ab8e2a3

ST_Distance function returns null now instead of throwing an error when one of the arguments in null.

imotov added >enhancement :Analytics/Geo Indexing, search aggregations of geo points and shapes :Analytics/SQL SQL querying v8.0.0 labels Apr 1, 2019

imotov requested review from costin, astefan, bpintea and matriv April 1, 2019 21:44

matriv reviewed Apr 1, 2019

View reviewed changes

bpintea approved these changes Apr 2, 2019

View reviewed changes

Address review comments

c7f35ca

matriv approved these changes Apr 2, 2019

View reviewed changes

Merge remote-tracking branch 'elastic/geosql' into null-handling-for-…

dd3e122

…geo-points

astefan requested changes Apr 3, 2019

View reviewed changes

costin approved these changes Apr 7, 2019

View reviewed changes

Address review comments

4004d7a

@astefan thanks a lot for assistance and brainstorming earlier today!

astefan approved these changes Apr 9, 2019

View reviewed changes

Address final review comments

8feb38c

costin reviewed Apr 9, 2019

View reviewed changes

imotov added 2 commits April 9, 2019 20:05

Address @costin's review comments

284ec3b

Merge remote-tracking branch 'elastic/geosql' into null-handling-for-…

e49c1de

…geo-points

Merge remote-tracking branch 'elastic/geosql' into null-handling-for-…

cf96e18

…geo-points

imotov merged commit d7647c8 into elastic:geosql Apr 10, 2019

imotov removed the v8.0.0 label Apr 24, 2019

imotov deleted the null-handling-for-geo-points branch May 1, 2020 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Improve null handling in Geo Functions #40708

SQL: Improve null handling in Geo Functions #40708

imotov commented Apr 1, 2019

elasticmachine commented Apr 1, 2019

elasticmachine commented Apr 1, 2019

matriv left a comment

matriv Apr 1, 2019

bpintea left a comment

matriv left a comment

astefan left a comment

astefan Apr 3, 2019

imotov Apr 7, 2019

costin left a comment

costin Apr 7, 2019

astefan left a comment

astefan Apr 9, 2019

astefan Apr 9, 2019

astefan Apr 9, 2019

astefan Apr 9, 2019

costin left a comment

costin Apr 9, 2019

costin Apr 9, 2019

imotov Apr 9, 2019

costin Apr 9, 2019

costin Apr 9, 2019

imotov commented Apr 9, 2019

imotov commented Apr 10, 2019

SQL: Improve null handling in Geo Functions #40708

SQL: Improve null handling in Geo Functions #40708

Conversation

imotov commented Apr 1, 2019

elasticmachine commented Apr 1, 2019

elasticmachine commented Apr 1, 2019

matriv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea left a comment

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imotov commented Apr 9, 2019

imotov commented Apr 10, 2019