server/src/main/java/org/opensearch/search/aggregations/bucket/histogram/DateHistogramAggregator.java (1)

236-239: Consider adding debug logging for skiplist collector selection.

Given the performance-sensitive nature of this optimization and the benchmark debugging discussed in PR comments, consider adding trace-level logging when the skiplist path is selected. This would help with future performance debugging.

         if (canUseSkiplist(skipper, singleton)) {
             skipListCollectorsUsed++;
+            logger.trace("Using skiplist collector for field: {}", fieldName);
             return new HistogramSkiplistLeafCollector(singleton, skipper, preparedRounding, bucketOrds, sub, this);
         }

server/src/main/java/org/opensearch/search/aggregations/bucket/HistogramSkiplistLeafCollector.java (3)

110-129: Consider caching and validating owningBucketOrd for correctness.

Based on prior discussion, this collector only works correctly when owningBucketOrd remains constant across calls. Consider caching the first owningBucketOrd and either asserting or falling back to non-skiplist behavior if a different value is received:

+    private long cachedOwningBucketOrd = -1;
+
     @Override
     public void collect(int doc, long owningBucketOrd) throws IOException {
+        if (cachedOwningBucketOrd == -1) {
+            cachedOwningBucketOrd = owningBucketOrd;
+        } else {
+            assert cachedOwningBucketOrd == owningBucketOrd : "HistogramSkiplistLeafCollector requires constant owningBucketOrd";
+        }
+
         if (doc > upToInclusive) {
             advanceSkipper(doc, owningBucketOrd);
         }

Based on learnings from past review comments discussing this exact scenario.

---

146-158: Performance optimization opportunity when sub-collector is NO_OP.

The fast path using stream.count(upToExclusive) is a good optimization. However, consider extracting the sub-collector check outside the loop to avoid repeated NO_OP_COLLECTOR comparisons:

     @Override
     public void collect(DocIdStream stream, long owningBucketOrd) throws IOException {
         // This will only be called if its the sub aggregation
+        final boolean isNoOpSub = (sub == NO_OP_COLLECTOR);
         for (;;) {
             int upToExclusive = upToInclusive + 1;
             if (upToExclusive < 0) { // overflow
                 upToExclusive = Integer.MAX_VALUE;
             }

             if (upToSameBucket) {
-                if (sub == NO_OP_COLLECTOR) {
+                if (isNoOpSub) {
                     // stream.count maybe faster when we don't need to handle sub-aggs
                     long count = stream.count(upToExclusive);

---

21-25: Add more detailed class-level Javadoc.

The current documentation is minimal. Consider expanding to describe the skip-list optimization strategy, when this collector should be used, and any invariants (e.g., constant owningBucketOrd requirement).

server/src/test/java/org/opensearch/search/aggregations/bucket/filterrewrite/FilterRewriteSubAggTests.java (2)

292-356: Consider adding verification that the skiplist path is actually exercised.

The test correctly verifies functional correctness of the range + date histogram aggregation with skiplist-enabled index configuration. However, it doesn't confirm that the skiplist optimization path was actually taken during execution. Consider adding assertions or instrumentation to verify the optimization triggered (e.g., checking profiling output, using a test hook, or verifying collection mode).

---

767-836: Consider extracting a helper to reduce duplication.

The method contains three similar blocks for indexing document batches. Each block follows the same pattern of creating documents with date and metric fields, then committing. While the explicit structure aids readability, extracting a helper method like addDocumentBatch(IndexWriter writer, int count, long timestamp, long metric) would reduce repetition and improve maintainability.

Example refactor:

private void addDocumentBatch(IndexWriter writer, int count, long timestamp, long metricValue) throws IOException {
    for (int i = 0; i < count; i++) {
        org.apache.lucene.document.Document doc = new org.apache.lucene.document.Document();
        doc.add(SortedNumericDocValuesField.indexedField(dateFieldName, timestamp));
        doc.add(new LongPoint(dateFieldName, timestamp));
        doc.add(new NumericDocValuesField(longFieldName, metricValue));
        doc.add(new LongPoint(longFieldName, metricValue));
        writer.addDocument(doc);
    }
}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro