Improving the performance of date histogram aggregation (without any sub-aggregation)

[jainankitk]

Description

This change preemptively creates point range query filter on each date histogram bucket for quick collection instead of iterating over all the documents. Currently, the optimization is limited to search requests getting rewritten into matchall query or filtering the documents using point range query on same field as date histogram aggregation. This PR will be followed up with below changes:

• Expand the optimization to match all query by using min/maxPackedValue at segment/leaf level for creating bucket
• Apply the same optimization to AutoDateHistogram
• Apply the same optimization to Composite aggregation with source as DateHistogram
• Tune the number of buckets for which query filter optimization can be applied
• Apply the optimization to DocValuesFieldExistsQuery over the field (now deprecated in favor of FieldExistsQuery)
• Apply the optimization if the individual segment is match all query even though the whole shard is not

Related Issues

Resolves #9310

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
• Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)
• [ ] Public documentation issue/PR created

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]

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

[github-actions]

✅ Gradle check result for be20dde: SUCCESS

[codecov]

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (8c6eb7d) 71.33% compared to head (e225781) 71.47%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
...egations/bucket/histogram/FilterRewriteHelper.java	79.38%	10 Missing and 10 partials ⚠️
...ions/bucket/histogram/DateHistogramAggregator.java	82.60%	3 Missing and 1 partial ⚠️
.../bucket/histogram/AutoDateHistogramAggregator.java	94.44%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #11083      +/-   ##
============================================
+ Coverage     71.33%   71.47%   +0.14%     
- Complexity    58982    59136     +154     
============================================
  Files          4890     4891       +1     
  Lines        277468   277629     +161     
  Branches      40313    40347      +34     
============================================
+ Hits         197919   198442     +523     
+ Misses        63127    62723     -404     
- Partials      16422    16464      +42     

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

[msfroh]

The test org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=search.aggregation/10_histogram/date_histogram profiler} is failing due to the optimization not being present in the prior versions of OpenSearch. I am wondering if the collect_count validation should be removed from the test or the test itself should be skipped for prior versions. I am in favor of removing the collect_count validation as it does not really matter as long as the result is correct. @msfroh - Thoughts?

I agree -- while I appreciate the value of testing the profiler, I don't think it was ever reasonable to assume backward compatibility in profiler output meaning we must provide the exact same profile shape across different versions.

[github-actions]

✅ Gradle check result for e225781: SUCCESS

[reta]

@jainankitk What is the reason to change the visibility level? this is internal API and should stay as such

[jainankitk]

The offset value is needed for creating the correct range buckets in FilterRewriteHelper::createFilterForAggregations. Some of the calendar intervals like month/quarter/year can have varying number of days, which is stored in extraLocalOffsetLookup. Do you see any harm in making just getter public? If yes, do you have a workaround in mind?

[reta]

@jainankitk please take a loot at #11392

[jainankitk]

That's a good one, thanks!

[jainankitk]

This is targeted for v2.12, hence adding backport 2.x label

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-11083-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0ddbd96291d4ff05499ec53c5a04a5dda32d36ad
# Push it to GitHub
git push --set-upstream origin backport/backport-11083-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-11083-to-2.x.