Skip to content

Conversation

@asimmahmood1
Copy link
Contributor

Description

Introduce execution hint for Cardinality aggregation - Revive PR #15764

Related Issues

Resolves #15269

Check List

  • [x ] Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created,

Testing

Tested using big5 dataset, see #16837 (comment)

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: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
Signed-off-by: Siddharth Rayabharam <[email protected]>
if (hasPrecisionThreshold) {
out.writeLong(precisionThreshold);
}
out.writeOptionalString(executionHint);
Copy link
Member

Choose a reason for hiding this comment

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

When making ser/de changes there can be issues in mixed clusters (fe during B/G) if this object is serialized to an older node that doesn't understand the value. In such cases, the serialization logic can check the target version (example) to see if the target node is of an older version and exclude it from serialization.

Similarly when deserializing if the request is coming from an older node then if you try to read the value from the StreamInput it will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point, will update.

* This PR is reviving opensearch-project#15764

Signed-off-by: Asim Mahmood <[email protected]>
* @opensearch.internal
*/
private static class DirectCollector extends Collector {
public static class DirectCollector extends Collector {
Copy link
Member

Choose a reason for hiding this comment

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

Can these be package-protected?

@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2025

❌ Gradle check result for c582cdc: 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
Copy link
Contributor

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

@asimmahmood1
Copy link
Contributor Author

Closing this PR, it has double commit message, going to recreate a new PR.

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 good first issue Good for newcomers Search:Aggregations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] Add a cluster setting for memory threshold to pick OrdinalsCollector in Cardinality aggregation

3 participants