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

feat: update aggregation config #3974

Closed
wants to merge 5 commits into from

Conversation

raintygao
Copy link
Contributor

@raintygao raintygao commented May 3, 2023

Description

  1. Add geo_shape type field support in geo_tile and geo_hash aggregation since OpenSearch has supported it.
    image
    image

  2. Add a centroid switch in geo_tile aggregation to keep consistent with geo_hash aggregation, which can't be switched before and is true by default.
    image

  3. In centroid editor, set value false and switch disabled when field type is geo_shape because currently OpenSearch only supports geo_shape aggregation without centroid.
    image

Target version

2.8

Validation steps

  1. Create Visualize -> Coordinate Maps
  2. Select a data source with geo_point and geo_shape type field.
  3. Click add button and you can see it.
    image
    image
  4. Fill the form and click update, you can see the right response in maps and browser devtools.

Issues Resolved

#3887 Resolved No.2.

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
@codecov-commenter
Copy link

codecov-commenter commented May 3, 2023

Codecov Report

Merging #3974 (38ad551) into main (ca0bb8f) will increase coverage by 0.03%.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3974      +/-   ##
==========================================
+ Coverage   66.38%   66.42%   +0.03%     
==========================================
  Files        3229     3229              
  Lines       62068    62077       +9     
  Branches     9599     9601       +2     
==========================================
+ Hits        41204    41234      +30     
+ Misses      18556    18539      -17     
+ Partials     2308     2304       -4     
Flag Coverage Δ
Linux 66.36% <0.00%> (?)
Windows 66.37% <0.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...lugins/data/common/search/aggs/buckets/geo_hash.ts 87.50% <ø> (ø)
...lugins/data/common/search/aggs/buckets/geo_tile.ts 30.00% <ø> (ø)
...default_editor/public/components/agg_params_map.ts 100.00% <ø> (ø)
...tor/public/components/controls/use_geocentroid.tsx 0.00% <0.00%> (ø)

... and 5 files with indirect coverage changes

@raintygao
Copy link
Contributor Author

@ashwin-pc @VijayanB @navneet1v FYI.

@navneet1v
Copy link

@raintygao some of the github workflows are failing. Let's make sure that all of them succeeds.

@ananzh
Copy link
Member

ananzh commented May 3, 2023

@raintygao @navneet1v the ftr tests fail due to chromedrive incompatibility issue #3975. We also raised a PR which should unblock all the tests.

@navneet1v
Copy link

@raintygao @navneet1v the ftr tests fail due to chromedrive incompatibility issue #3975. We also raised a PR which should unblock all the tests.

@ananzh, @raintygao So the overall code looks good to me. I am approving the PR, as GHA failure is known issue.

@raintygao
Copy link
Contributor Author

raintygao commented May 4, 2023

@ananzh Hi, when will my PR be in the review process from core team? Because the release status of our feature is associated with the merge status of PR.

@ashwin-pc
Copy link
Member

ashwin-pc commented May 4, 2023

@raintygao Can you also add steps in the PR description on how we can validate the change? Also can you attach a GIF or a video for the UI changes? I cant quote about the exact date the change will go through, but we usually target to have the change reviewed in at most a week since its been assigned. After that it depends on how long it takes for the changes needed to be addressed. Thats why giving us screenshots and steps to validate helps because it makes the review process quicker.

@ashwin-pc ashwin-pc self-assigned this May 4, 2023
@raintygao
Copy link
Contributor Author

@raintygao Can you also add steps in the PR description on how we can validate the change? Also can you attach a GIF or a video for the UI changes? I cant quote about the exact date the change will go through, but we usually target to have the change reviewed in at most a week since its been assigned. After that it depends on how long it takes for the changes needed to be addressed. Thats why giving us screenshots and steps to validate helps because it makes the review process quicker.

@ashwin-pc Done.

@raintygao
Copy link
Contributor Author

raintygao commented May 5, 2023

Currently set the status of this pr to frozen .

@ashwin-pc
Copy link
Member

@raintygao why is this PR frozen?

@joshuarrrr
Copy link
Member

@ananzh Hi, when will my PR be in the review process from core team? Because the release status of our feature is associated with the merge status of PR.

@raintygao I don't see this PR included in the platform upgrade list for 2.8.0: #3981, which means it's not planned for that release, although we'll still do our best to provide timely reviews. If you think it should be included, please add more information about the dependency and feature there.

@raintygao
Copy link
Contributor Author

Thanks, due to the adjustment of external plugin and changes on ui, the technical solution may be changed. Will close PR first.

@raintygao raintygao closed this May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants