I am concerned about the performance of doing this on dotcom. A batch of matches can contain thousands of results and I know querying the repo table is already an issue.

But these thousand repos are read at once for each batch. Or is your concern memory usage related?

I'm not worried so much about memory usage more so about long running queries on the repo table. This would issue a single query with a potentially thousands of repo ids. We've done similar things in insights for filtering repos out of an insight and had performance issues that came from passing a long list of parameters. I don't know this repo query well enough to know how it performs in that situation about you should make sure it handles it will.

Is the need for this DB call to load the metadata keys / values?

I thought yes, but I just looked and I see the metadata in the search result event stream, so it shouldn't be necessary.

I'm seeing matches comeback as

[
    {
        "type": "repo",
        "repositoryID": 399,
        "repository": "github.com/sourcegraph/sourcegraph",
        "repoStars": 7857,
        "repoLastFetched": "2023-05-16T15:41:12.441465Z",
        "description": "Code Intelligence Platform",
        "metadata": {
            "code-intel": null,
            "code-search": null,
            "license": "multiple",
            "open-source": "",
            "team": "sourcegraph"
        }
    }
]

if that's the case, I don't fully understand why this lookup is happening. Could we just use the search results directly here @erzhtor?

The metadata is populated lazily right at the search API boundary, so it's not on the search results that are seen by the aggregation code

@chwarwick, can you please point out where did you check the stream? I'm getting following when checking here (where the aggregation is happening):

{
  "Results": [
    {
      "Name": "github.com/sourcegraph/sourcegraph",
      "ID": 7,
      "Rev": "",
      "DescriptionMatches": null,
      "RepoNameMatches": [{ "start": [0, 0, 0], "end": [34, 0, 34] }]
    }
  ],
  "Stats": {
    "IsLimitHit": false,
    "Repos": null,
    "Status": {},
    "BackendsMissing": 0,
    "ExcludedForks": 0,
    "ExcludedArchived": 0
  }
}

I see, you've mentioned search result stream. I think that part similarly does additional batch db query for repo metadata and appends to the eventMatch, https://sourcegraph.com/github.com/sourcegraph/sourcegraph@f3b65acc3e189c7b54586e7bd1d7eaa13dc77104/-/blob/cmd/frontend/internal/search/search.go?L697-713 (pointed out by @camdencheek). So I used the same approach here for aggregation event stream.

I would like to avoid the AggregationMode from leaking into this aggregator. My initial goal here was that over time we would be able to reuse this for Code Insights that are persisted on the dashboard as it was previously a goal to be able to persist these aggregations to an Insights dashboard.

I think an alternative approach that I think is worth trying is to use the repoCount function and then swap out the Aggregator when you are in metadata mode. My understanding is that since it's select:repo the result is going to be a long list of repoID with a 1.

I think you could compose your own aggregator on top of the existing limitedAggregator that builds up the repo slice and then converts it to metadata, maybe flushing the list every N Add calls or when the user requests it to be sorted.

type LimitedAggregator interface {
	Add(label string, count int32)
	SortAggregate() []*Aggregate
	OtherCounts() OtherCount
}

Thank you for the suggestion, will take a look!