Support common format geo point #2801

bugmakerrrrrr · 2024-07-03T14:01:10Z

Description

Currently, we only support geo point in the format of an object with a latitude and longitude, this PR aims to expand our support to all common formats that are described in OpenSearch doc.

Issues Resolved

#1432

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.

bugmakerrrrrr · 2024-07-03T14:03:48Z

@dai-chen you might be interested in this :) Could you please take a look when you get a chance?

...h/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java

dai-chen · 2024-07-09T17:20:25Z

...earch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java

+   * @param supportArrays Parsing the whole array or not
+   * @return Geo point value parsed from content.
+   */
+  private ExprValue parseGeoPoint(Content content, boolean supportArrays) {


From the doc, there seems 6 ways to represent a geo point. Could you clarify in comment which code block is handling which format?

LantaoJin · 2024-07-10T00:22:58Z

The build failures contains low code test coverage: Execution failed for task ':opensearch:jacocoTestCoverageVerification'. Please run ./gradlew build in local and check the jacoco report file.

bugmakerrrrrr · 2024-07-11T14:02:44Z

...earch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java

+
+    var elements = content.array();
+    var first = elements.next();
+    // an array in the [longitude, latitude] format.


@dai-chen Actually, I have commented on one format. The other formats are handled by GeoUtils#parseGeoPoint.

LantaoJin · 2024-07-16T14:26:43Z

Seems the workflows were not triggered correctly. @bugmakerrrrrr Could u merge latest change from main and re-run test? (try to avoid force-push)

Signed-off-by: panguixin <[email protected]>

bugmakerrrrrr · 2024-07-16T16:24:07Z

Seems the workflows were not triggered correctly. @bugmakerrrrrr Could u merge latest change from main and re-run test? (try to avoid force-push)

After rebasing the main branch, it appears that I have to perform a force push.

LantaoJin · 2024-07-16T16:31:13Z

You can git merge latest upstream/main to your local dev branch then push to your origin/[dev branch].

LantaoJin · 2024-07-17T07:09:00Z

Unit tests passed, fundamentally LGTM, @bugmakerrrrrr could you add a new IT similar to integ-test/src/test/java/org/opensearch/sql/sql/DateTimeFormatsIT.java?

Now only one geo_point format case used in IT, ref

sql/integ-test/src/test/resources/datatypes.json

Line 2 in 2c29a1a

    
           {"boolean_value": true, "keyword_value": "keyword", "text_value": "text", "binary_value": "U29tZSBiaW5hcnkgYmxvYg==", "date_value": "2020-10-13 13:00:00", "date_nanos_value": "2019-03-23T21:34:46.123456789-04:00", "ip_value": "127.0.0.1", "object_value": {"first": "Dale", "last": "Dale"}, "nested_value": [{"first" : "John", "last" :  "Smith"}, {"first" : "Alice", "last" :  "White"}], "geo_point_value": { "lat": 40.71, "lon": 74.00 }}

, you can add more rows in this file or add a new json file.

Signed-off-by: panguixin <[email protected]>

LantaoJin

Thanks for the changes!
cc @dai-chen

bugmakerrrrrr · 2024-07-30T08:48:01Z

@LantaoJin @dai-chen can we merge this?

LantaoJin · 2024-07-31T03:52:40Z

@penghuo do you have a chance to review this?

...earch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java

penghuo

Thanks for the change!

opensearch-trigger-bot · 2024-08-01T15:10:27Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.x
# Create a new branch
git switch --create backport/backport-2801-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 82ef68e2b25c7c10740e74968bbe960c000c1cee
# Push it to GitHub
git push --set-upstream origin backport/backport-2801-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2801-to-2.x.

penghuo · 2024-08-01T15:11:33Z

@bugmakerrrrrr could u help resolve conflict and submit backport PR to 2.x. #2801 (comment)

--------- Signed-off-by: panguixin <[email protected]> (cherry picked from commit 82ef68e)

bugmakerrrrrr · 2024-08-05T07:30:37Z

@bugmakerrrrrr could u help resolve conflict and submit backport PR to 2.x. #2801 (comment)

#2896

--------- Signed-off-by: panguixin <[email protected]> (cherry picked from commit 82ef68e)

--------- Signed-off-by: panguixin <[email protected]>

bugmakerrrrrr requested review from ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vmmusings, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto, GumpacG and ykmr1224 as code owners July 3, 2024 14:01

LantaoJin reviewed Jul 8, 2024

View reviewed changes

...h/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java Show resolved Hide resolved

bugmakerrrrrr force-pushed the geopoint branch from 53471f7 to a984f54 Compare July 8, 2024 12:27

dai-chen reviewed Jul 9, 2024

View reviewed changes

bugmakerrrrrr commented Jul 11, 2024

View reviewed changes

bugmakerrrrrr force-pushed the geopoint branch from a984f54 to 46412a1 Compare July 11, 2024 14:30

bugmakerrrrrr added 3 commits July 17, 2024 00:21

Support common format geo point

c6c01dc

Signed-off-by: panguixin <[email protected]>

add unsupported format test

e8fa1cd

Signed-off-by: panguixin <[email protected]>

fix jacoco test

634d577

Signed-off-by: panguixin <[email protected]>

bugmakerrrrrr force-pushed the geopoint branch from 46412a1 to 634d577 Compare July 16, 2024 16:22

add IT

0de937f

Signed-off-by: panguixin <[email protected]>

LantaoJin approved these changes Jul 18, 2024

View reviewed changes

penghuo reviewed Aug 1, 2024

View reviewed changes

...earch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java Show resolved Hide resolved

penghuo approved these changes Aug 1, 2024

View reviewed changes

penghuo added the backport 2.x label Aug 1, 2024

penghuo merged commit 82ef68e into opensearch-project:main Aug 1, 2024
14 of 15 checks passed

opensearch-trigger-bot bot added the backport-failed label Aug 1, 2024

bugmakerrrrrr added a commit to bugmakerrrrrr/sql that referenced this pull request Aug 5, 2024

Support common format geo point (opensearch-project#2801)

db18ca5

--------- Signed-off-by: panguixin <[email protected]> (cherry picked from commit 82ef68e)

bugmakerrrrrr mentioned this pull request Aug 5, 2024

[Backport 2.x] Support common format geo point #2896

Merged

7 tasks

LantaoJin pushed a commit that referenced this pull request Aug 6, 2024

Support common format geo point (#2801) (#2896)

2df3855

--------- Signed-off-by: panguixin <[email protected]> (cherry picked from commit 82ef68e)

manasvinibs pushed a commit to manasvinibs/sql that referenced this pull request Aug 14, 2024

Support common format geo point (opensearch-project#2801)

8ba31ff

--------- Signed-off-by: panguixin <[email protected]>

jzonthemtn pushed a commit to jzonthemtn/sql that referenced this pull request Aug 28, 2024

Support common format geo point (opensearch-project#2801)

2fcf154

--------- Signed-off-by: panguixin <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support common format geo point #2801

Support common format geo point #2801

bugmakerrrrrr commented Jul 3, 2024

bugmakerrrrrr commented Jul 3, 2024

dai-chen Jul 9, 2024

LantaoJin commented Jul 10, 2024

bugmakerrrrrr Jul 11, 2024

LantaoJin commented Jul 16, 2024 •

edited

Loading

bugmakerrrrrr commented Jul 16, 2024

LantaoJin commented Jul 16, 2024

LantaoJin commented Jul 17, 2024 •

edited

Loading

LantaoJin left a comment

bugmakerrrrrr commented Jul 30, 2024

LantaoJin commented Jul 31, 2024

penghuo left a comment

opensearch-trigger-bot bot commented Aug 1, 2024

penghuo commented Aug 1, 2024

bugmakerrrrrr commented Aug 5, 2024

Support common format geo point #2801

Support common format geo point #2801

Conversation

bugmakerrrrrr commented Jul 3, 2024

Description

Issues Resolved

Check List

bugmakerrrrrr commented Jul 3, 2024

dai-chen Jul 9, 2024

Choose a reason for hiding this comment

LantaoJin commented Jul 10, 2024

bugmakerrrrrr Jul 11, 2024

Choose a reason for hiding this comment

LantaoJin commented Jul 16, 2024 • edited Loading

bugmakerrrrrr commented Jul 16, 2024

LantaoJin commented Jul 16, 2024

LantaoJin commented Jul 17, 2024 • edited Loading

LantaoJin left a comment

Choose a reason for hiding this comment

bugmakerrrrrr commented Jul 30, 2024

LantaoJin commented Jul 31, 2024

penghuo left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Aug 1, 2024

penghuo commented Aug 1, 2024

bugmakerrrrrr commented Aug 5, 2024

LantaoJin commented Jul 16, 2024 •

edited

Loading

LantaoJin commented Jul 17, 2024 •

edited

Loading