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

Add fieldType to AbstractQueryBuilder and SortBuilder #15328

Merged
merged 3 commits into from
Aug 30, 2024

Conversation

dzane17
Copy link
Contributor

@dzane17 dzane17 commented Aug 21, 2024

Description

  • Added fieldType class variable and getter methods in AbstractQueryBuilder and FieldSortBuilder.
  • Added fieldName() as an abstract method in AbstractQueryBuilder. This requires all child classes to implement fieldName(). Child classes which do not already have a fieldName() method defined should call getDefaultFieldName() which returns null.

Related Issues

opensearch-project/query-insights#69

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.

Copy link
Contributor

❌ Gradle check result for 095565f: 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?

Copy link
Contributor

❌ Gradle check result for 14ad54f: 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?

Copy link
Contributor

❌ Gradle check result for 3aaced4: 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?

Copy link
Contributor

❌ Gradle check result for 795c633: 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?

Copy link
Contributor

❌ Gradle check result for ab96823: 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?

Copy link
Contributor

❕ Gradle check result for a4235ca: 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.

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

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

Project coverage is 71.89%. Comparing base (1d5082e) to head (1b598e0).
Report is 3 commits behind head on main.

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              @@
##               main   #15328      +/-   ##
============================================
- Coverage     72.01%   71.89%   -0.12%     
- Complexity    63603    63609       +6     
============================================
  Files          5247     5247              
  Lines        297186   297355     +169     
  Branches      42939    42979      +40     
============================================
- Hits         214023   213790     -233     
- Misses        65610    65944     +334     
- Partials      17553    17621      +68     

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

@jainankitk
Copy link
Collaborator

jainankitk commented Aug 30, 2024

This is also not a good way to solve the original problem.

Revert this change and go back to the drawing board with this.

We have had some discussion on this here - opensearch-project/query-insights#69. Let us know if you have any suggestions on improving this

The idea that a query type acts on a single field doesn't even make sense. What about MultiMatchQueryBuilder? What about a QueryStringQueryBuilder?

And that is the reason we have provided default implementation of fieldName method, so that such QueryBuilders can delegate to that. Maybe having default implementation instead of abstract is better, although new implementations of AbstractQueryBuilder don't necessarily need to implement in that case?

@msfroh
Copy link
Collaborator

msfroh commented Aug 31, 2024

This way we are not assuming anything about query builders. Naming the abstract method getSingleFieldNameIfExists() may be more accurate but I think we can brainstorm about that.

We are not adding an abstract method to AbstractQueryBuilder.

It breaks any plugin that implements a new query type.

sachinpkale pushed a commit to sachinpkale/OpenSearch that referenced this pull request Sep 1, 2024
jainankitk added a commit to jainankitk/OpenSearch that referenced this pull request Sep 3, 2024
jainankitk added a commit that referenced this pull request Sep 4, 2024
akolarkunnu pushed a commit to akolarkunnu/OpenSearch that referenced this pull request Sep 10, 2024
…ject#15328)

* Add fieldType to AbstractQueryBuilder and FieldSortBuilder

Signed-off-by: David Zane <[email protected]>

* Add fieldType to AbstractQueryBuilder and SortBuilder

Signed-off-by: David Zane <[email protected]>

---------

Signed-off-by: David Zane <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
Co-authored-by: Ankit Jain <[email protected]>
akolarkunnu pushed a commit to akolarkunnu/OpenSearch that referenced this pull request Sep 10, 2024
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 16, 2024
…ject#15328)

* Add fieldType to AbstractQueryBuilder and FieldSortBuilder

Signed-off-by: David Zane <[email protected]>

* Add fieldType to AbstractQueryBuilder and SortBuilder

Signed-off-by: David Zane <[email protected]>

---------

Signed-off-by: David Zane <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
Co-authored-by: Ankit Jain <[email protected]>
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 16, 2024
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 17, 2024
…ject#15328)

* Add fieldType to AbstractQueryBuilder and FieldSortBuilder

Signed-off-by: David Zane <[email protected]>

* Add fieldType to AbstractQueryBuilder and SortBuilder

Signed-off-by: David Zane <[email protected]>

---------

Signed-off-by: David Zane <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
Co-authored-by: Ankit Jain <[email protected]>
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 17, 2024
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 21, 2024
…ject#15328)

* Add fieldType to AbstractQueryBuilder and FieldSortBuilder

Signed-off-by: David Zane <[email protected]>

* Add fieldType to AbstractQueryBuilder and SortBuilder

Signed-off-by: David Zane <[email protected]>

---------

Signed-off-by: David Zane <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
Co-authored-by: Ankit Jain <[email protected]>
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch backport-failed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants