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

[Star tree] Doc count field support in star tree #15282

Merged

Conversation

bharath-techie
Copy link
Contributor

@bharath-techie bharath-techie commented Aug 16, 2024

Description

In order to show doc count in search response as part of aggregation buckets, we are adding a _doc_count metric to star tree , so each star tree document will be associated with a _doc_count metric field.

By default _doc_count metric is added as part of StarTreeField.

So _doc_count field will be present as part of metricDocValueIterators as part of star tree doc values, which can be read during search aggregations flow.

Related Issues

Resolves #15288

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.

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
Copy link
Contributor

✅ Gradle check result for c647863: SUCCESS

Copy link

codecov bot commented Aug 16, 2024

Codecov Report

Attention: Patch coverage is 92.18750% with 5 lines in your changes missing coverage. Please review.

Project coverage is 71.85%. Comparing base (689cfca) to head (795fe81).
Report is 13 commits behind head on main.

Files Patch % Lines
...posite/composite99/Composite99DocValuesWriter.java 83.33% 1 Missing and 1 partial ⚠️
...datacube/startree/builder/BaseStarTreeBuilder.java 90.00% 0 Missing and 2 partials ⚠️
...arch/index/compositeindex/datacube/MetricStat.java 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #15282      +/-   ##
============================================
- Coverage     71.95%   71.85%   -0.11%     
+ Complexity    63266    63216      -50     
============================================
  Files          5224     5225       +1     
  Lines        296080   296128      +48     
  Branches      42764    42771       +7     
============================================
- Hits         213054   212770     -284     
- Misses        65560    65890     +330     
- Partials      17466    17468       +2     

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

Copy link
Contributor

@sarthakaggarwal97 sarthakaggarwal97 left a comment

Choose a reason for hiding this comment

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

Thanks @bharath-techie for the change. Gave some high level comments.
I am curious, is it different from _doc_count field that users can provide to ingest pre-aggregated data?

@bharath-techie
Copy link
Contributor Author

Thanks for early review.

PR is technically not in final state , need to add aggregator tests , merge flow tests.

Cleaning up the code also is in backlog, will change it to draft state for now till these are done.

@bharath-techie bharath-techie marked this pull request as draft August 17, 2024 04:14
@bharath-techie
Copy link
Contributor Author

I am curious, is it different from _doc_count field that users can provide to ingest pre-aggregated data?

Its the same. If user provides any value, we'll take that value otherwise we'll take '1' as the identity value.

Basically this will become the required metric field for all star tree fields and not value_count as _doc_count field in aggregations results will be based on this field.

#15152 (comment)

@bharath-techie bharath-techie self-assigned this Aug 18, 2024
@bharath-techie bharath-techie marked this pull request as ready for review August 18, 2024 13:45
@bharath-techie bharath-techie force-pushed the doc_count_mapper_change branch from d775c96 to 519c944 Compare August 18, 2024 13:51
Copy link
Contributor

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

@bharath-techie bharath-techie force-pushed the doc_count_mapper_change branch from 519c944 to 6194f05 Compare August 18, 2024 13:58
Copy link
Contributor

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

@bharath-techie bharath-techie force-pushed the doc_count_mapper_change branch from 6194f05 to b139000 Compare August 18, 2024 14:06
Copy link
Contributor

❌ Gradle check result for 6194f05: 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 eb9695d: 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
Contributor

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

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
@bharath-techie bharath-techie force-pushed the doc_count_mapper_change branch from b139000 to d5efcde Compare August 21, 2024 06:50
Copy link
Contributor

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

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
Copy link
Contributor

✅ Gradle check result for 795fe81: SUCCESS

@sachinpkale sachinpkale merged commit 091ab6f into opensearch-project:main Aug 27, 2024
34 of 37 checks passed
@bharath-techie bharath-techie added the backport 2.x Backport to 2.x branch label Aug 28, 2024
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 28, 2024
---------
Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
(cherry picked from commit 091ab6f)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ashking94 pushed a commit that referenced this pull request Aug 28, 2024
---------
Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
(cherry picked from commit 091ab6f)
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>
akolarkunnu pushed a commit to akolarkunnu/OpenSearch that referenced this pull request Sep 10, 2024
…15282)

---------
Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 16, 2024
…15282)

---------
Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 17, 2024
…15282)

---------
Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 21, 2024
…15282)

---------
Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
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 enhancement Enhancement or improvement to existing feature or request Indexing:Performance skip-changelog
Projects
None yet
4 participants