[Backport 2.x] Add fieldType to AbstractQueryBuilder and SortBuilder (#15328)

[dzane17]

(cherry picked from commit 839ba0b)
Signed-off-by: David Zane davizane@amazon.com

Description

[Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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.

[msfroh]

This is a breaking change on the 2.x line.

There are many plugins that implement custom QueryBuilders. We can't go and add a new abstract method to AbstractQueryBuilder without breaking all of those plugins.

[github-actions]

❌ Gradle check result for 584a55a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[msfroh]

Adding "Request changes" to block this from getting backported to 2.x, so that we don't break all the plugins.

[jainankitk]

This is a breaking change on the 2.x line.

There are many plugins that implement custom QueryBuilders. We can't go and add a new abstract method to AbstractQueryBuilder without breaking all of those plugins.

That's a good point @msfroh. The reason for making it abstract is to ensure future implementations automatically provide fieldName.

Any idea as to how many plugins we are talking about? And, what is the right channel for communicating this change?

[jainankitk]

While did not see anything in SQL plugin, saw 1 usage in k-NN - https://github.com/opensearch-project/k-NN/blob/2b303d90d7e47169f5bb1a216c4b9d7207794fba/src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java#L69

[msfroh]

Any idea as to how many plugins we are talking about?

Within opensearch-project, there are these 66 files: https://github.com/search?q=org%3Aopensearch-project+%22extends+AbstractQueryBuilder%22+NOT+repo%3Aopensearch-project%2FOpenSearch&type=code

Outside of opensearch-project, who knows? I tried searching all of GitHub and found a few hits (e.g. Querqy) that aren't forks from opensearch-project: https://github.com/search?q=org.opensearch.index.query.AbstractQueryBuilder+NOT+repo%3Aopensearch-project%2FOpenSearch+NOT+path%3A%2F%5Eserver%5C%2Fsrc%5C%2Fmain%5C%2Fjava%5C%2Forg%5C%2Fopensearch%5C%2F%2F+NOT+owner%3Aopensearch-project++NOT+path%3A%2F%5Emodules%5C%2F%2F+NOT+path%3A%2F%5Esrc%5C%2Fmain%5C%2Fkotlin%5C%2Forg%5C%2Fopensearch%5C%2Findexmanagement%5C%2F%2F+NOT+path%3A%2F%5Esrc%5C%2Fmain%5C%2Fjava%5C%2Forg%5C%2Fopensearch%5C%2Fknn%5C%2Findex%5C%2F%2F+NOT+path%3A%2F%5Esrc%5C%2Fmain%5C%2Fjava%5C%2Forg%5C%2Fopensearch%5C%2Fsecurity%5C%2F%2F+NOT+path%3A%2F%5Eserver%5C%2Fsrc%5C%2FinternalClusterTest%5C%2Fjava%5C%2Forg%5C%2Fopensearch%5C%2Fsearch%5C%2Ffetch%5C%2Fsubphase%5C%2Fhighlight%5C%2F%2F&type=code

See also my comment on #15328, though. Adding a field name to all QueryBuilders doesn't make sense and is a bad idea. There are many query types that act on multiple fields. The assumption that a QueryBuilders in general "have" a field name is a fundamental misunderstanding of OpenSearch queries.

[github-actions]

❕ Gradle check result for 6bad1a9: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

Attention: Patch coverage is 93.75000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 71.66%. Comparing base (a2b6f8c) to head (6bad1a9).
Report is 30 commits behind head on 2.x.

Files with missing lines	Patch %	Lines
...g/opensearch/index/query/AbstractQueryBuilder.java	66.66%	1 Missing ⚠️
...rg/opensearch/index/query/WrapperQueryBuilder.java	0.00%	1 Missing ⚠️
.../org/opensearch/search/sort/ScriptSortBuilder.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x   #15538      +/-   ##
============================================
- Coverage     71.73%   71.66%   -0.07%     
- Complexity    63721    63774      +53     
============================================
  Files          5235     5235              
  Lines        297971   298039      +68     
  Branches      43363    43371       +8     
============================================
- Hits         213741   213589     -152     
- Misses        66369    66646     +277     
+ Partials      17861    17804      -57     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jainankitk]

Any idea as to how many plugins we are talking about?

Within opensearch-project, there are these 66 files: https://github.com/search?q=org%3Aopensearch-project+%22extends+AbstractQueryBuilder%22+NOT+repo%3Aopensearch-project%2FOpenSearch&type=code

Outside of opensearch-project, who knows? I tried searching all of GitHub and found a few hits (e.g. Querqy) that aren't forks from opensearch-project: https://github.com/search?q=org.opensearch.index.query.AbstractQueryBuilder+NOT+repo%3Aopensearch-project%2FOpenSearch+NOT+path%3A%2F%5Eserver%5C%2Fsrc%5C%2Fmain%5C%2Fjava%5C%2Forg%5C%2Fopensearch%5C%2F%2F+NOT+owner%3Aopensearch-project++NOT+path%3A%2F%5Emodules%5C%2F%2F+NOT+path%3A%2F%5Esrc%5C%2Fmain%5C%2Fkotlin%5C%2Forg%5C%2Fopensearch%5C%2Findexmanagement%5C%2F%2F+NOT+path%3A%2F%5Esrc%5C%2Fmain%5C%2Fjava%5C%2Forg%5C%2Fopensearch%5C%2Fknn%5C%2Findex%5C%2F%2F+NOT+path%3A%2F%5Esrc%5C%2Fmain%5C%2Fjava%5C%2Forg%5C%2Fopensearch%5C%2Fsecurity%5C%2F%2F+NOT+path%3A%2F%5Eserver%5C%2Fsrc%5C%2FinternalClusterTest%5C%2Fjava%5C%2Forg%5C%2Fopensearch%5C%2Fsearch%5C%2Ffetch%5C%2Fsubphase%5C%2Fhighlight%5C%2F%2F&type=code

See also my comment on #15328, though. Adding a field name to all QueryBuilders doesn't make sense and is a bad idea. There are many query types that act on multiple fields. The assumption that a QueryBuilders in general "have" a field name is a fundamental misunderstanding of OpenSearch queries.

I think most of these references are coming from documentation, which should auto update from the code. Excluding that we have about:

• 1 - k-NN
• 1 - alerting
• 1 - security-analytics
• 3 - neural-search
• 5 - opensearch-learning-to-rank-base

TBH, updating these plugins doesn't look too bad to me. But, I am open to suggestions on improving the original code change itself. Some of the discussion on different options is captured over here - opensearch-project/query-insights#69

[jainankitk]

Closing PR while we are working on different approach for solving this - opensearch-project/query-insights#69