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.

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

Support geopoint type #309

Merged
merged 30 commits into from
Jul 27, 2023
Merged

Support geopoint type #309

merged 30 commits into from
Jul 27, 2023

Conversation

GumpacG
Copy link

@GumpacG GumpacG commented Jul 21, 2023

Description

Supports geo_point by adding properties for lat and lon in order for Tableau to access locations.
This PR will only support the format {"lat": number, "lon": number}.
Geopoint will be treated as an ExprTupleValue to allow access to lat and lon properties.

Expect results on Tableau when opensearch-project#1793 is resolved:
Screenshot 2023-07-10 at 8 18 32 AM

Follow up work:

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #309 (cc54f39) into integ-geopoint (430d7a9) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@                 Coverage Diff                  @@
##             integ-geopoint     #309      +/-   ##
====================================================
- Coverage             97.42%   97.42%   -0.01%     
+ Complexity             4647     4645       -2     
====================================================
  Files                   408      408              
  Lines                 11526    11525       -1     
  Branches                839      841       +2     
====================================================
- Hits                  11229    11228       -1     
  Misses                  290      290              
  Partials                  7        7              
Flag Coverage Δ
sql-engine 97.42% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...h/sql/opensearch/data/type/OpenSearchDataType.java 100.00% <ø> (ø)
...l/opensearch/data/type/OpenSearchGeoPointType.java 100.00% <100.00%> (ø)
...l/opensearch/data/utils/OpenSearchJsonContent.java 100.00% <100.00%> (ø)
...search/data/value/OpenSearchExprGeoPointValue.java 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@MitchellGale
Copy link

Tested with:
An object with a latitude and longitude
A string in the “latitude,longitude” format
A geohash

found in https://opensearch.org/docs/2.3/opensearch/supported-field-types/geo-point/

LGTM.

@MitchellGale
Copy link

SQL plugin currently only supports the format :code:`{"lat": number, "lon": number}`. Can you add this to the PR description please

}

@Test
public void test_geo_point_unsupported_format() throws IOException {

Choose a reason for hiding this comment

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

Consider adding a test for lat issue and separate test for lon issue from opensearch/src/main/java/org/opensearch/sql/opensearch/data/utils/OpenSearchJsonContent.java.

Copy link
Author

Choose a reason for hiding this comment

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

We actually cant load non-number values in to lat and lon to OpenSearch. So integtests won't work. I believe this is just a fail safe for the valueFactory. There are unit tests for this I think that would be enough. Thoughts?

@Yury-Fridlyand
Copy link

Please, fix tests.

Copy link

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Please, rebase or merge on upstream to fix CI

docs/user/general/datatypes.rst Outdated Show resolved Hide resolved
GumpacG added 8 commits July 27, 2023 11:16
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
GumpacG and others added 22 commits July 27, 2023 11:16
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
@GumpacG GumpacG merged commit 937f82b into integ-geopoint Jul 27, 2023
GumpacG added a commit that referenced this pull request Jul 31, 2023
* Added changes from POC PR

Signed-off-by: Guian Gumpac <[email protected]>

* Added geopoint parser for value factory

Signed-off-by: Guian Gumpac <[email protected]>

* Fixed old tests

Signed-off-by: Guian Gumpac <[email protected]>

* Fixed all old tests

Signed-off-by: Guian Gumpac <[email protected]>

* Removed irrelevant changes

Signed-off-by: Guian Gumpac <[email protected]>

* Removed irrelevant changes

Signed-off-by: Guian Gumpac <[email protected]>

* Added integration tests

Signed-off-by: Guian Gumpac <[email protected]>

* Fixed not throwing exception for geojson

Signed-off-by: Guian Gumpac <[email protected]>

* Made GeoPointValue an ExprTupleValue to enable access to lat and lon

Signed-off-by: Guian Gumpac <[email protected]>

* Fixed checkstyle

Signed-off-by: Guian Gumpac <[email protected]>

* Added unit tests and removed unnecessary code

Signed-off-by: Guian Gumpac <[email protected]>

* reverted irrelevant changes

Signed-off-by: Guian Gumpac <[email protected]>

* Added to docs

Signed-off-by: Guian Gumpac <[email protected]>

* Fixed doc typo

Signed-off-by: Guian Gumpac <[email protected]>

* Test doctests

Signed-off-by: Guian Gumpac <[email protected]>

* Test doctests

Signed-off-by: Guian Gumpac <[email protected]>

* Remove geopoint data from doctests

Signed-off-by: Guian Gumpac <[email protected]>

* Fixed doctest failure

Signed-off-by: Guian Gumpac <[email protected]>

* Fixed doctest failure

Signed-off-by: Guian Gumpac <[email protected]>

* Fix doctest clean up.

Signed-off-by: Yury-Fridlyand <[email protected]>

* Minimized doc example

Signed-off-by: Guian Gumpac <[email protected]>

* Fixed doctest

Signed-off-by: Guian Gumpac <[email protected]>

* Fixed doctest

Signed-off-by: Guian Gumpac <[email protected]>

* Fixed doctest

Signed-off-by: Guian Gumpac <[email protected]>

* Fixed doctest

Signed-off-by: Guian Gumpac <[email protected]>

* Fixed reordering of results

Signed-off-by: Guian Gumpac <[email protected]>

* Added more rows to doctests and removed IOException:

Signed-off-by: Guian Gumpac <[email protected]>

* Fixed doctest

Signed-off-by: Guian Gumpac <[email protected]>

* Fixed doctest

Signed-off-by: Guian Gumpac <[email protected]>

* Addressed PR comments

Signed-off-by: Guian Gumpac <[email protected]>

---------

Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
@Yury-Fridlyand Yury-Fridlyand deleted the dev-geopoint branch August 8, 2023 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants