Add Hybrid Cardinality collector to prioritize Ordinals Collector

[anandpatel9998]

Description

Current cardinality aggregator logic selects DirectCollector over OrdinalsCollector when relative memory overhead due to OrdinalsCollector (compared to DirectCollector) is higher. Because of this relative memory consumption logic, DirectCollector is selected for high cardinality aggregation queries. DirectCollector is slower compared to OrdinalsCollector. This default selection leads to higher search latency even when Opensearch process have available memory to use ordinals collector for faster query performance.

There is no way to figure out memory requirement for nested aggregation because number of buckets are dynamically created as we traverse through all the matching document ids. To overcome this limitation, this change create a hybrid collector which will first use Ordinals Collector and will switch to DirectCollector if memory usage for Ordinals Collector Increase beyond certain threshold. When Hybrid collector switch from Ordinals Collector to Direct Collector, it will utilize already computed aggregation data from Ordinals Collector so that we do not have to rebuild aggregation result using Direct Collector.

Signed-off-by: Anand Pravinbhai Patel anapat@amazon.com

Related Issues

Resolves #19260

Check List

• [ Done ] Functionality includes testing.
• [ Not Applicable ] API changes companion pull request created, if applicable.
• [ Is it required ? ] 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.

Summary by CodeRabbit

• New Features
  • Hybrid Cardinality collector that can switch collection strategy at runtime and two dynamic cluster settings to control it.
• Observability
  • Profiler output updated to report hybrid_collectors_used and adjusted profiler expectations.
• Public API
  • New cardinality aggregation context exposed to configure hybrid behavior (enabled + memory threshold).
• Tests
  • Expanded tests covering hybrid collector scenarios.
• Chores
  • CHANGELOG updated with the addition.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[github-actions]

❌ Gradle check result for a2f5dd7: null

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 41a9e69: 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 c142ac4: null

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 88989f3: 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 c142ac4: 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 06ce5c3: null

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 fc328a2: 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?

[jainankitk]

@sandeshkr419 - Can you help get this change merged and backported for 3.4? This is important performance optimizations that many customers should be able to benefit from.

[github-actions]

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

server/src/test/java/org/opensearch/search/aggregations/metrics/CardinalityAggregatorTests.java (1)

750-778: Consider adding assertion for active collector type.

These tests verify the overall functionality but only check that the collector is a HybridCollector without verifying whether it stayed with OrdinalsCollector or switched. For better coverage, consider adding an assertion similar to the other tests:

-    }, collector -> { assertTrue(collector instanceof HybridCollector); }, fieldType, true, 1024L * 1024L);
+    }, collector -> {
+        assertTrue(collector instanceof HybridCollector);
+        assertTrue(((HybridCollector) collector).getActiveCollector() instanceof OrdinalsCollector);
+    }, fieldType, true, 1024L * 1024L);

server/src/main/java/org/opensearch/search/aggregations/metrics/CardinalityAggregator.java (1)

944-947: Verify close() handles the ordinalsCollector reference after switch.

After switching to DirectCollector, ordinalsCollector holds a reference to an already-closed collector. The current close() implementation only closes activeCollector, which is correct. However, if the class is refactored in the future, someone might accidentally try to close ordinalsCollector again.

Consider nullifying the reference or adding a comment for clarity:

 private void switchToDirectCollector() throws IOException {
     // Post collect all the already computed data from OrdinalsCollector
     ordinalsCollector.postCollect();
     ordinalsCollector.close();
     logger.debug("Hybrid collector switching to DirectCollector due to memory threshold exceeded");
+    // Note: ordinalsCollector is now closed; only activeCollector should be used hereafter

     // Create DirectCollector with computed data
     activeCollector = new DirectCollector(counts, hashValues);
 }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aabab15 and d2730aa.

📒 Files selected for processing (6)

• CHANGELOG.md (1 hunks)
• rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/170_cardinality_metric.yml (2 hunks)
• rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/170_cardinality_metric_unsigned.yml (2 hunks)
• server/src/main/java/org/opensearch/search/aggregations/metrics/CardinalityAggregator.java (9 hunks)
• server/src/test/java/org/opensearch/search/aggregations/metrics/CardinalityAggregatorTests.java (2 hunks)
• test/framework/src/main/java/org/opensearch/search/aggregations/AggregatorTestCase.java (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (4)

• CHANGELOG.md
• rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/170_cardinality_metric.yml
• rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/170_cardinality_metric_unsigned.yml
• test/framework/src/main/java/org/opensearch/search/aggregations/AggregatorTestCase.java

🔇 Additional comments (10)

server/src/test/java/org/opensearch/search/aggregations/metrics/CardinalityAggregatorTests.java (5)

72-73: LGTM!

Imports for the new inner classes HybridCollector and OrdinalsCollector are correctly added to support the new test assertions.

---

639-701: LGTM! Well-structured test helper method.

The testAggregationHybridCollector helper correctly sets up a CardinalityAggregationContext with configurable hybridCollectorEnabled flag and memoryThreshold, allowing flexible testing of hybrid collector behavior under different configurations.

---

703-730: Good test coverage for enabled/disabled states.

These tests correctly verify that:

1. When enabled, a HybridCollector wrapping OrdinalsCollector is used
2. When disabled, OrdinalsCollector is used directly (without hybrid wrapper)

---

732-748: Test correctly verifies memory threshold switching.

Using an extremely low threshold of 1 byte ensures the switch from OrdinalsCollector to DirectCollector is triggered. The HLL++ tolerance of ±10 for 1000 values is appropriate.

---

854-862: Good verification of the singleton exception optimization.

This test properly validates the performance optimization: the singleton pattern and suppressed stack trace generation ensure minimal overhead when the exception is used for control flow.

server/src/main/java/org/opensearch/search/aggregations/metrics/CardinalityAggregator.java (5)

94-108: Well-designed control flow exception.

The singleton pattern with disabled stack trace generation (writableStackTrace: false) is the correct optimization for exceptions used as control flow signals. This avoids the ~100x overhead of stack trace capture in hot paths.

---

618-682: Memory monitoring design is efficient.

The implementation correctly:

1. Only accumulates memory when new buckets are created (not on every collect call)
2. Uses a flag (memoryMonitoringEnabled) to avoid overhead when monitoring is disabled
3. Throws the singleton exception immediately after threshold breach

One observation: the BitArray is allocated before the threshold check (lines 669-670). This is acceptable since the memory would be reclaimed during the subsequent close() call in switchToDirectCollector().

---

866-927: HybridCollector correctly handles the exception-based switching pattern.

The try-catch-retry pattern in each collect method is correct. The exception is only thrown from getBitArray() when creating new buckets, which occurs:

• Before stream.forEach() in collect(DocIdStream, long)
• Before the loop in collectRange(int, int)
• At the start of each doc in collect(int, long)

This ensures no partial processing issues when switching collectors.

---

929-947: Resource management in switchToDirectCollector is correct.

The method properly:

1. Calls postCollect() to flush accumulated ordinal data to the HyperLogLog++ counts
2. Closes the OrdinalsCollector to release memory
3. Creates the DirectCollector to continue collection

One note: after switching, ordinalsCollector still holds a reference to the closed collector, but this is fine since close() only releases activeCollector, and switchToDirectCollector can only be called once (DirectCollector doesn't throw MemoryLimitExceededException).

---

110-130: Settings are well-configured, but verify proper registration in ClusterSettings.

Using Setting.memorySizeSetting correctly supports both percentage-based (e.g., "1%") and absolute (e.g., "10mb") threshold values. The NodeScope and Dynamic properties allow runtime configuration changes. However, ensure that both CARDINALITY_AGGREGATION_HYBRID_COLLECTOR_ENABLED and CARDINALITY_AGGREGATION_HYBRID_COLLECTOR_MEMORY_THRESHOLD are properly registered in the ClusterSettings configuration.

[github-actions]

✅ Gradle check result for d2730aa: SUCCESS