So tryPrecomputeAggregationForLeaf runs per leaf segment and returns a CollectionTerminationException if that leaf can be computed without running an expensive collection. The getLeafCollector of GlobalOrdinalStringTermsAggregator only runs when we cannot pre-compute. So I think what the stack trace is pointing to is two leaves where one is pre-computed and the other is not. This is intended behavior, unless I'm mistaken.

@msfroh @sandeshkr419 am I completely off here?

@bruce-hong-glean specifically what regressions are you seeing? Are your queries slower?

@harshavamsi I think I understand the problem pointed out now.
Thanks @bruce-hong-glean for reporting it.

Basically when trying to pre-compute using tryPrecomputeAggregationForLeaf(...), we compute globalOrds once.

    @Override
    protected boolean tryPrecomputeAggregationForLeaf(LeafReaderContext ctx) throws IOException {
        SortedSetDocValues globalOrds = valuesSource.globalOrdinalsValues(ctx);
        .
        .
        .
        return false;
    }

Now, for cases when we are not successfully pre-computing (return false), we fallback to default implementation of getLeafCollector(...) where we compute globalOrds again:

    @Override
    public LeafBucketCollector getLeafCollector(LeafReaderContext ctx, LeafBucketCollector sub) throws IOException {
        SortedSetDocValues globalOrds = valuesSource.globalOrdinalsValues(ctx);
        collectionStrategy.globalOrdsReady(globalOrds);
        .
        .
        .

So I think re-computing globalOrds is what we can avoid since it is expensive. Before #16733 since everything resided in getLeafCollector() we were computing globalOrds only once.

I'm also curious about the regression, if we can reproduce, maybe we will find some issue for loading global ordinal. Theoretically I would expect it to be built only once for a shard, unless there are updates and refreshes coming in.

@bruce-hong-glean

I raised a fix for this.
For testing, a non match-all query with keyword terms aggregation should see slight improvements.

Something like https://github.com/opensearch-project/opensearch-benchmark-workloads/blob/main/noaa/operations/default.json#L253 Once the CI finishes, will try running benchmark comparisons on it.

@bruce-hong-glean

I was looking into benchmark results of 2.18 vs 2.19 since the refactoring was done in 2.19.

Orange/brown line is 2.18 and trailing green line is 2.19 - I didn't see any regression in the 2 queries where the sub-aggregation is present which should fail pre-compute and use regular LeafCollector inplementation:

    {
      "name": "keyword-terms-min",
      "operation-type": "search",
      "body": {
        "size": 0,
        "aggs": {
          "station": {
            "terms": {
              "field": "station.id",
              "size": 500
            },
            "aggs": {
              "tmin": {
                "min": {
                  "field": "TMIN"
                }
              }
            }
          }
        }
      }
    },
    {
      "name": "keyword-terms-low-cardinality-min",
      "operation-type": "search",
      "body": {
        "size": 0,
        "aggs": {
          "country": {
            "terms": {
              "field": "station.country",
              "size": 200
            },
            "aggs": {
              "tmin": {
                "min": {
                  "field": "TMIN"
                }
              }
            }
          }
        }
      }
    },

Let me know if you have a better way to verify the fix. Else will try to do some benchmarking setup locally.

Unfortunately I don't have a better way to verify immediately either. We've seen this happen on a subset of customer deployments, so I could build an image and post screenshots of the changes in profiles and latency stats here in ~2 weeks (following our release schedule) if that helps.