-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
SQL: Improve error messages for geo_shape #41923
SQL: Improve error messages for geo_shape #41923
Conversation
Improves error messages when geo_shapes are used in unsupported manner and adds description of current geo function limitations. Fixes elastic#41791
Pinging @elastic/es-analytics-geo |
Pinging @elastic/es-search |
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.
Looks good overall, but the limitation should be added to a different page (as well).
@@ -5,6 +5,18 @@ | |||
|
|||
The geo functions work with geometries stored in `geo_point` and `geo_shape` fields, or returned by other geo functions. | |||
|
|||
==== Limitations |
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.
We have a Limitations page where various bits and pieces are listed in a central place: https://www.elastic.co/guide/en/elasticsearch/reference/master/sql-limitations.html
It's probably ok to list this limitation here, but I would like to see it listed in the general Limitations page, as well.
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.
👍
@@ -773,4 +773,24 @@ public void testAggregateAliasInFilter() { | |||
public void testProjectUnresolvedAliasInFilter() { | |||
assertEquals("1:8: Unknown column [tni]", error("SELECT tni AS i FROM test WHERE i > 10 GROUP BY i")); | |||
} | |||
|
|||
public void testGeoShapeInWhereClause() { |
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.
Can you add one more test where all three unsupported scenarios take place? WHERE .... GROUP BY .... ORDER BY ....
so that all three messages are logged.
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.
I would have to two other shape fields to the mapping for that. The failure messages are grouped by their source node, which is shape in this case. So, only one failure message would show up. Do you think it is worth adding other shape fields?
@@ -582,8 +582,7 @@ public void testTranslateStAsWktForPoints() { | |||
} | |||
|
|||
public void testTranslateStAsWktForShapes() { |
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.
The method name should be changed, since it's only testing geo points.
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.
Good catch. It should be removed, actually. We already have a method that tests this for points.
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
…ng-of-geoshapes-and-doc-fields
…ng-of-geoshapes-and-doc-fields
@elasticmachine run elasticsearch-ci/1 |
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
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
Improves error messages when geo_shapes are used in unsupported manner
and adds description of current geo function limitations.
Fixes #41791