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

Extract all used fields from QueryStringQueryBuilder #6020

Closed
wants to merge 4 commits into from

Conversation

petardz
Copy link
Contributor

@petardz petardz commented Jan 26, 2023

Signed-off-by: Petar Dzepina [email protected]

Description

We recently implemented rewriting of fields used in QueryStringQueryBuilder in ISM and we had to copy over substantial amount of code from core in ISM. To avoid this code duplication and future maintenance, we could extend core's QueryStringQueryBuilder to provide method for extracting all fields which are used in final query.

Also in Alerting we would have use-case for this, where if we could grab all fields used in queries, we could just copy mappings to percolate index only for those fields.

Issues Resolved

#6019

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Jan 26, 2023

Codecov Report

Attention: Patch coverage is 78.26087% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 70.84%. Comparing base (e265355) to head (8738485).
Report is 426 commits behind head on main.

❗ Current head 8738485 differs from pull request most recent head 2616db2. Consider uploading reports for the commit 2616db2 to get more accurate results

Files Patch % Lines
...pensearch/index/query/QueryStringQueryBuilder.java 56.25% 4 Missing and 3 partials ⚠️
...pensearch/index/search/QueryStringQueryParser.java 90.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6020      +/-   ##
============================================
- Coverage     71.41%   70.84%   -0.57%     
+ Complexity    59397    58760     -637     
============================================
  Files          4923     4771     -152     
  Lines        279212   280863    +1651     
  Branches      40595    40571      -24     
============================================
- Hits         199408   198989     -419     
- Misses        63223    65557    +2334     
+ Partials      16581    16317     -264     

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

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@kotwanikunal
Copy link
Member

Apologies. This PR was auto closed without reaching a resolution from the maintainers.
Re-opening to move it forward.
Thanks for your contributions to OpenSearch!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 14, 2023

Compatibility status:

Checks if related components are compatible with change 2616db2

Incompatible components

Incompatible components: [https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/performance-analyzer-rca.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/k-nn.git]

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@ashking94
Copy link
Member

@petardz Is this being worked upon? Pls free to reach out to maintainers for further reviews.

@ticheng-aws
Copy link
Contributor

Hi @petardz, do we have any updates?

@petardz
Copy link
Contributor Author

petardz commented Jan 6, 2024

This was gonna be useful for ISM plugin for Rollup feature where field names are getting rewritten. Currently, ISM is copying over bunch of code from QSQ implementation from core.

Also, this could be useful for Alerting plugin too. Currently, Alerting is copying over mappings from source index to query index for ALL fields, but it could be copying only ones used in percolate queries, if parsing query fields would be trivial.

@bowenlan-amzn @sbcd90 @praveensameneni @getsaurabh02 if you still find this useful I could rebase this PR.

return queryParser;
}

public Set<String> extractAllUsedFields(QueryShardContext context) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need comment for this method. What it's used for and how it can be used.

@bowenlan-amzn
Copy link
Member

@sbcd90 @praveensameneni I belive Subho worked on this before, can do a review.
@petardz please rebase and let's get this in, don't want the effort here to be wasted.

@ticheng-aws ticheng-aws added the enhancement Enhancement or improvement to existing feature or request label Jan 6, 2024
@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Jan 15, 2024
Signed-off-by: Petar Dzepina <[email protected]>
Signed-off-by: Petar Dzepina <[email protected]>
Signed-off-by: Petar Dzepina <[email protected]>
Signed-off-by: Petar Dzepina <[email protected]>
@petardz petardz force-pushed the qsq-extract-fields branch from 8738485 to 2616db2 Compare January 22, 2024 01:49
@petardz petardz requested a review from abbashus as a code owner January 22, 2024 01:49
Copy link
Contributor

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

@stephen-crawford
Copy link
Contributor

It seems like this contribution is stalled. I would recommend we close it for now. @peternied could you close this for us? Thanks

@peternied
Copy link
Member

Thanks for the mention @scrawfor99 but this is a little outside my experience.

@eirsep @sbcd90 @praveensamenen @bowenlan-amzn Maybe one of you could weight in on what you think should happen next for this PR? Is this something one of you would want to drive to completion?

@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added stalled Issues that have stalled and removed stalled Issues that have stalled labels May 10, 2024
@dblock
Copy link
Member

dblock commented Jul 15, 2024

Closing, please feel free to finish and reopen. [Catch All Triage - 1, 2, 3, 4]

@dblock dblock closed this Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or improvement to existing feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.