-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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: Add support for shape type #46464
SQL: Add support for shape type #46464
Conversation
Enables support for cartesian geometries shape type. We still need to decide how to handle the distance function since it is currently using the haversine distance formula and returns results in meters, which doesn't make any sense for cartesian geometries. Closes elastic#46412 Relates to elastic#43644
Pinging @elastic/es-search |
Pinging @elastic/es-analytics-geo |
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
@@ -73,25 +73,25 @@ private static String createOGCIndexRequest() throws Exception { | |||
createString("name", createIndex); |
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.
Since geo_shape
support is still around, it would help to have both geo_shape
and shape
field types in the same dataset for both testing and example purposes. Unless I'm missing something it looks like all the geo_ fields were renamed.
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.
geo_shape
and shape
are both here to stay, they share pretty much most of the code paths, there is some test duplication between OGC tests and other tests, and shape
implementation is much closer to OGC standard. So, I thought it just switching OGC test to shape
would make sense and shouldn't reduce overall test coverage and we don't bloat the tests in the process.
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.
@costin does this makes sense, or should I make a copy of the tests for shape
?
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.
@imotov A full copy is not needed however it would good to have a test that touches on a shape
field (even if its just listing or doing a basic query) alongside a geo_shape
.
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.
👍
@costin could you take another look? |
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. Thanks!
Enables support for Cartesian geometries shape type. We still need to
decide how to handle the distance function since it is currently using
the haversine distance formula and returns results in meters, which
doesn't make any sense for Cartesian geometries.
Closes #46412
Relates to #43644