[Derived Fields] Add aggregation support for derived fields

[mch2]

Description

Adds aggregation support for derived fields.

Related Issues

Resolves #14590

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.

[github-actions]

❌ Gradle check result for 5c150d5: 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 92c4724: 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 6336b13: 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 78cd442: 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?

[jed326]

@mch2 @rishabhmaurya for concurrent search we realized that whenever an aggregation script is used on the _source field we would hit a race condition where the leafLookup objects are actually shared across threads. To mitigate this we disabled concurrent search for composite aggregations that use scripts, however just at a glance it looks like that same race condition might be possible here with derived fields.
See:

• [Concurrent Segment Search] Supporting scripting for composite aggs #12947
• [BUG][Concurrent Search] Search request for top anomalies from AD plugin is failing #12331 (comment)

Could we add some ITs here an run them parameterized with concurrent search to double check?
Or if more testing/research is still needed we could also disable concurrent search when derived fields are used for now as well.

fyi @sohami

[github-actions]

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

[sohami]

@mch2 @rishabhmaurya for concurrent search we realized that whenever an aggregation script is used on the _source field we would hit a race condition where the leafLookup objects are actually shared across threads. To mitigate this we disabled concurrent search for composite aggregations that use scripts, however just at a glance it looks like that same race condition might be possible here with derived fields. See:

• [Concurrent Segment Search] Supporting scripting for composite aggs #12947
• [BUG][Concurrent Search] Search request for top anomalies from AD plugin is failing #12331 (comment)

Could we add some ITs here an run them parameterized with concurrent search to double check? Or if more testing/research is still needed we could also disable concurrent search when derived fields are used for now as well.

fyi @sohami

I see that it is documented in the limitation section but we should have the logic to handle this in the service side as @jed326 pointed out. Also we should add tests with concurrent search enabled as well for any new search feature to ensure that it works well with that as well.

[rishabhmaurya]

@jed326 could you help me understand why/how the LeafSearchLookup is shared across multiple threads in concurrent segment search codepath?
LeafSearchLookup isn't threadsafe and that's the invariant which should be maintained.

[jed326]

@rishabhmaurya shared some more details in #12331 (comment)

[github-actions]

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

[mch2]

Would prefer we tackle the issue with concurrent search separately from this. Will make a separate issue and see what we can do. @jed326 @rishabhmaurya wdyt?

Also we should add tests with concurrent search enabled as well for any new search feature to ensure that it works well with that as well.

@sohami I think it would be nice to randomize here with yaml tests vs requiring a separate parameterized case every time.

[jed326]

@mch2 The biggest concern I have right now is that we document that derived fields does not support concurrent segment search but there's no service side enforcement of that. I'm fine with taking that as a fast follow-up but want to make sure we don't lose track of that task. It should be a simple addition to this method to do so but will leave that to you to decide how to handle it.

[jed326]

@sohami I think it would be nice to randomize here with yaml tests vs requiring a separate parameterized case every time.

At least for concurrent search part of the problem is that in the yaml tests it's hard to ensure there are multiple segments so that we actually go down the concurrent search path. In the Java ITs this method

OpenSearch/test/framework/src/main/java/org/opensearch/test/ParameterizedOpenSearchIntegTestCase.java

Lines 36 to 41 in f5b0eba

	// This method shouldn't be called in setupSuiteScopeCluster(). Only call this method inside single test.
	public void indexRandomForConcurrentSearch(String... indices) throws InterruptedException {
	if (CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.get(settings)) {
	indexRandomForMultipleSlices(indices);
	}
	}

is used as a best effort to add (deleted) dummy docs and ensure we actually go down the concurrent search path.

[mch2]

added a docs issue - opensearch-project/documentation-website#7850

[mch2]

@mch2 The biggest concern I have right now is that we document that derived fields does not support concurrent segment search but there's no service side enforcement of that. I'm fine with taking that as a fast follow-up but want to make sure we don't lose track of that task. It should be a simple addition to this method to do so but will leave that to you to decide how to handle it.

Agreed - cut #15007

@sohami I think it would be nice to randomize here with yaml tests vs requiring a separate parameterized case every time.

At least for concurrent search part of the problem is that in the yaml tests it's hard to ensure there are multiple segments so that we actually go down the concurrent search path. In the Java ITs this method

OpenSearch/test/framework/src/main/java/org/opensearch/test/ParameterizedOpenSearchIntegTestCase.java

Lines 36 to 41 in f5b0eba

	// This method shouldn't be called in setupSuiteScopeCluster(). Only call this method inside single test.
	public void indexRandomForConcurrentSearch(String... indices) throws InterruptedException {
	if (CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.get(settings)) {
	indexRandomForMultipleSlices(indices);
	}
	}

is used as a best effort to add (deleted) dummy docs and ensure we actually go down the concurrent search path.

hmm true, maybe we can do the same here with yml tests re dummy docs.

[github-actions]

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

[mch2]

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

#15008

[github-actions]

✅ Gradle check result for 7fcab64: SUCCESS