-
Notifications
You must be signed in to change notification settings - Fork 7
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
Query shape for agg & sort #44
Conversation
Thanks! @dzane17 do you have some examples where we have the entire query shape including Query, Aggregations, Sort and Pipeline aggregations all in one? Something like this:
Also, will the field related information for query operations will be as a followup? |
Was hoping we could have a |
+1 on this, we should create a lib to extract shape from a |
cc8f17e
to
2f995db
Compare
@deshsidd @ansjcy I have added a large search request example to the description. QueryShapeService now contains logic to generate shape given a source. We can call it when and where ever needed. Currently all methods are static. I could not envision why we'd want to create instances of QueryShapeService unless we are calling @jainankitk Regarding query field data, there is not an easy way to extract field names from a The options I came up with are
This also gets more complicated if we want to expand beyond field names... For example, we have
|
Thanks @dzane17. Looks good overall, but need to figure out the best way to get the field details for the query type. Based on the above proposed approaches I like approach 2 For properties such as width, we would have to do custom handling for the specific query type. Eg: if we encounter a range we can use the source to get the width. |
The options I came up with are
This also gets more complicated if we want to expand beyond field names... For example, we have width param which is unique to range type. @dzane17 - Thank you for explaining the issue here, and some of the possible options. While option 2 looks decent, I am wondering about a fourth option to have a map containing QueryBuilder class type and list of things you want to collect for that specific class. For example: <RangeQueryBuilder, List.of(getFieldName, getWidth)> If a specific class is not present in the map, we will collect default data without any field names. Thoughts? |
@dzane17 - Thank you for explaining the issue here, and some of the possible options. While option 2 looks decent, I am wondering about a fourth option to have a map containing QueryBuilder class type and list of things you want to collect for that specific class. For example: <RangeQueryBuilder, List.of(getFieldName, getWidth)> If a specific class is not present in the map, we will collect default data without any field names. Thoughts? |
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeService.java
Outdated
Show resolved
Hide resolved
@deshsidd
|
@jainankitk This 4th option will also work and solves additional field data beyond field names. The main disadvantage is that we need to maintain the Map. If we intend to collect field data unique to each QueryBuilder, option 4 seems good. If we only need fieldNames, then option 2 seems more natural. |
We can build the map gradually over time. We should write code in a manner that not having entry in the map still allows us to collect the basic information. As and when more stuff is added to the map, shape gets decorated with more stuff |
@jainankitk @deshsidd I have added query field data using option 4 |
...t/java/org/opensearch/plugin/insights/core/service/categorizor/QueryShapeGeneratorTests.java
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Outdated
Show resolved
Hide resolved
...ain/java/org/opensearch/plugin/insights/core/service/categorizer/SearchQueryCategorizer.java
Outdated
Show resolved
Hide resolved
...t/java/org/opensearch/plugin/insights/core/service/categorizor/QueryShapeGeneratorTests.java
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Show resolved
Hide resolved
@jainankitk Let us know your thoughts |
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Outdated
Show resolved
Hide resolved
8ff59f9
to
a40fad5
Compare
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Outdated
Show resolved
Hide resolved
@jainankitk @deshsidd Currently we have no field data for pipeline aggregations. Should we use |
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.
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/plugin/insights/core/service/categorizer/QueryShapeGenerator.java
Outdated
Show resolved
Hide resolved
Signed-off-by: David Zane <[email protected]>
Not sure if this will be helpful. Can leave it without any additional data for now? |
Thanks @dzane17 this is looking good overall! Lets iterate and keep improving this. |
Signed-off-by: David Zane <[email protected]> (cherry picked from commit b55d760) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit b55d760) Signed-off-by: David Zane <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
Add ability to generate query shape for
aggregation
andsort
portions of search body.Testing
Large Search
Aggregation
Nested Aggregation
Pipeline Aggregation
Sort