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

date aggregation component updates #3887

Open
raintygao opened this issue Apr 19, 2023 · 14 comments
Open

date aggregation component updates #3887

raintygao opened this issue Apr 19, 2023 · 14 comments
Labels
enhancement New feature or request maps Issues or PRs related to the Maps Service

Comments

@raintygao
Copy link
Contributor

Is your feature request related to a problem? Please describe.

My team has a new feature target for 2.8 in external plugin, in this feature we will do data aggregation. I tried to build using agg_params component that is consistent with OSD and found two features that are not supported in OSD but are supported in OpenSearch.

  1. agg_params component supports geohash and geotile aggregation, but it doesn’t seem to support geohex grid aggregation. Can we apply this in agg_params component?
  2. Currently geotile aggregation and geohash aggregation in OSD only support GEO_POINT type field, can we support GEO_SHAPE type field?

Describe the solution you'd like
We hope that the OSD core team can update these on the original agg_params component, or please let me know if there are already other ways to implement it.

@raintygao raintygao added the enhancement New feature or request label Apr 19, 2023
@ashwin-pc
Copy link
Member

@raintygao thanks for raising this issue. Can you contribute the change to the repo for the new agg params that you are trying to use? I can help support you with the change necessary. It looks like you will need to add these agg types to the data plugin like the geohash and then expose them in the agg_types file. There may be a few ore chnages necessary other than this, but let me know if you run into any issues making these changes.

@raintygao
Copy link
Contributor Author

raintygao commented Apr 20, 2023

@ashwin-pc Thank you Ashwin. I may be able to find some time to contribute, but it may be late and take a long time because agg_params component is currently used by many plugins and i'm not familiar with OSD code, so I need to spend a lot of time testing and verifying the impact after building.
Since this is the first time I have encountered a case where we need to add features to OSD, can core team usually support this?

@navneet1v
Copy link

@raintygao I see that for enabling the GeoShapes, the changes is very very minimal, can we pick up that change to make sure that we can enable GeoShape aggregations?

Given that input and output of the aggregation is same and as @ashwin-pc has already suggested that he can do the PR, I feel its a quick change.

@raintygao
Copy link
Contributor Author

raintygao commented Apr 21, 2023

Sure I think I can do that if changes are minimal.

  1. According to my previous rough estimate, for enabling GeoShapes type field, we may only need to modify the filterFieldTypes of specific aggregation type into an array containing GeoShapes, right? @navneet1v @ashwin-pc What do you guys think?

  2. @VijayanB Currently, the steps to create a new bucket aggregation are as shown in the screenshot, do we need to update other styles accordingly if we add GeoShapes type field? Such as changing geo coordinates to some other text.
    image
    image

@navneet1v
Copy link

According to my previous rough estimate, for enabling GeoShapes type field, we may only need to modify the filterFieldTypes of specific aggregation type into an array containing GeoShapes, right? @navneet1v @ashwin-pc What do you guys think?

@raintygao yes that is what is required. The aggregation on geo shapes have same input and output payload as that of Geopoint

@VijayanB
Copy link
Member

VijayanB commented Apr 21, 2023

@raintygao We don't have to update the Buckets component/section. Will build our own buckets since it is applicable for Maps only.

@joshuarrrr joshuarrrr added the maps Issues or PRs related to the Maps Service label Apr 25, 2023
@raintygao
Copy link
Contributor Author

raintygao commented Apr 27, 2023

@navneet1v @VijayanB Hi guys, I just want to confirm again, can we directly change the default field type of geotile aggregation and geohash aggregation from [geo_point] to [geo_point, geo_shape] ? As you guys replied before, the aggregation on geo shapes have same input and output payload as that of Geopoint. If we do this, other components that use these two aggregations will also be affected, such as coordinate maps, which will also support geo_shape. It would be great if it's ok.

@navneet1v
Copy link

I see no issues. Even later on also the fields needs to be updated.

@VijayanB
Copy link
Member

@raintygao I believe this will not affect coordinate map to show geo_shape fields for selection.

@raintygao
Copy link
Contributor Author

raintygao commented Apr 29, 2023

@navneet1v @VijayanB There is another very important issue. I use latest public 2.6 version of OpenSearch and created an index with geo_shape filed. But the aggregatable attribute of this field is false, which means that we can't aggregate this attribute directly in dashboard anyway. On the contrary, geo_point is ok. This may be caused by the default logic of OpenSearch and I don't find any ways for users to modify this value. Do you know why? This will directly affect our support for geo_shape.

image

@navneet1v
Copy link

The aggregation code to do geoshape aggregation is not in 2.c branch. It is in main branch. I would recommend using 3.0 version. Once we know that in which version of Opensearch geoshape aggregation is going we are going to backport the code during in version. This is the reason you don't see geoshape aggregation enabled in anybof 2.x version.

@raintygao
Copy link
Contributor Author

raintygao commented Apr 30, 2023

@navneet1v OK, I think the support of geo shape in OSD depends on the support in OS. So do you mean I can directly use main branch to do integration test?

Once we know that in which version of Opensearch geoshape aggregation is going we are going to backport the code during in version.

Does this mean that the corresponding code for the main branch will not be released until support is started in the OSD?

@navneet1v
Copy link

Does this mean that the corresponding code for the main branch will not be released until support is started in the OSD?

@raintygao

Yes. So lets say we plan that geoshape aggregation is going to be launched in 2.8. I will go ahead and backport the changes in 2.x branch which will become 2.8. But if we say geoshape aggregation will be supported in 2.9 in OSD, then I will backport changes in 2.x after 2.8 launch. The reason we need to do this is because 2.x branch contains next minor release changes.

This is the reason I am saying you can work on main branch which is 3.0 version for OS, OSD and dashboards-maps. Once we develop on main and complete the PR for geoshape aggregation we will have a better understanding whether geoshape aggregation will be launched or not in next minor release.

@navneet1v
Copy link

OK, I think the support of geo shape in OSD depends on the support in OS. So do you mean I can directly use main branch to do integration test?

Yes you can use main branch for development and testing. Just make sure all the components have same version. Which is OS, OSD and dashboards-maps on main branch which is 3.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request maps Issues or PRs related to the Maps Service
Projects
None yet
Development

No branches or pull requests

5 participants