Simplify the grammar to drop "system". There are already too many "system" in our vocabulary and here used for two different meanings ("system tenant" and "system target"); there's no difference between tenant-id==1 and system, and the test cases render them differently. It would help simplify your parser below. I'd be ok exporting it now too (ParseSystemTarget) like we do for everything else -- we are going to use something like it in spanconfig.store tests shortly anyway.

Could panic instead of this confusing return.

Typo: traget. Could panic below instead of the empty string return.

(Applies to earlier commits as well I think) Should we be using InternalValue? The name and surrounding comment suggests we shouldn't:

Could also make our helper take the TenantID type instead. If we got rid of type casting errors like suggested in the other review, some of this code could be simplified/inlined given there'd be no error handling needed.

How exactly are we expecting the reconciler to get all system targets down the line? The API here suggests that each key is to be retrieved by naming it one-by-one. Will that continue to be the case?

I'd recommend getting rid of this part of the grammar too. It's not adding anything that isn't represented by {source=1}, something our grammar can already represent. I also have misgivings about spanconfig.MakeClusterTarget. For one, despite the same name, it's wholly different conceptually to ptpb.MakeClusterTarget. We chatted a bit offline about this with @adityamaru, regardless of where we land, simplifying the testdata grammar itself is a good thing IMO.

[nit] "expected to find %q or %q prefix" would avoid stuttering in your error message.

For future PRs: worth reconsidering IMO these constructors, in addition to MakeClusterTarget. My preference:

MakeClusterTarget(roachpb.TenantID) SystemTarget // takes in what you call today "source"
MakeTenantsTarget([]roachpb.TenantID) []SystemTarget // doesn't take in the source, has to be system tenant, but takes in dests

Pick whatever names you want for these.

What we ended up with for the time being is:

MakeClusterTarget() SystemTarget // returns a system target with only the sourceTarget set to the system tenant ID 
MakeTenantTarget(source, target roachpb.TenantID)

Then the SQLTranslator will:

	// Aggregate cluster target protections for the tenant.
	clusterProtections := ptsStateReader.getProtectionPoliciesForCluster()
	if len(clusterProtections) != 0 {
		var systemTarget spanconfig.SystemTarget
		var err error
		if sourceTenantID == roachpb.SystemTenantID {
			systemTarget = spanconfig.MakeClusterTarget()
		} else {
			systemTarget, err = spanconfig.MakeTargetTenant(sourceTenantID, sourceTenantID)
			if err != nil {
				return nil, err
			}
		}
		clusterSystemRecord := spanconfig.Record{
			Target: spanconfig.MakeTargetFromSystemTarget(systemTarget),
			Config: roachpb.SpanConfig{GCPolicy: roachpb.GCPolicy{ProtectionPolicies: clusterProtections}}}
		records = append(records, clusterSystemRecord)
	}

	// Aggregate tenant target protections.
	tenantProtections := ptsStateReader.getProtectionPoliciesForTenants()
	for _, protection := range tenantProtections {
		tenantProtection := protection
		tenantSystemRecord := spanconfig.Record{
			Target: spanconfig.MakeTargetFromSystemTarget(spanconfig.MakeTenantTarget(
				sourceTenantID, tenantProtection.tenantID)),
			Config: roachpb.SpanConfig{GCPolicy: roachpb.GCPolicy{
				ProtectionPolicies: tenantProtection.protections}},
		}
		records = append(records, tenantSystemRecord)
	}

But we could make it simpler without complicating the client side:

	// Aggregate cluster target protections for the tenant.
	clusterProtections := ptsStateReader.getProtectionPoliciesForCluster()
	if len(clusterProtections) != 0 {
		clusterSystemRecord := spanconfig.Record{
			Target: spanconfig.MakeTargetFromSystemTarget(spanconfig.MakeClusterTarget(sourceTenantID)),
			Config: roachpb.SpanConfig{GCPolicy: roachpb.GCPolicy{ProtectionPolicies: clusterProtections}}}
		records = append(records, clusterSystemRecord)
	}

	// Aggregate tenant target protections.
	tenantProtections := ptsStateReader.getProtectionPoliciesForTenants()
	for _, protection := range tenantProtections {
		tenantProtection := protection
		tenantSystemRecord := spanconfig.Record{
			Target: spanconfig.MakeTargetFromSystemTarget(spanconfig.MakeTenantTarget(tenantProtection.tenantID)),
			Config: roachpb.SpanConfig{GCPolicy: roachpb.GCPolicy{
				ProtectionPolicies: tenantProtection.protections}},
		}
		records = append(records, tenantSystemRecord)
	}

If we want to simplify the client side logic, it might be worth considering changing the ptsStateReader to return ptpb.Targets instead of protection policies like it does today. Then, we could write conversions to/from ptpb.Targets to spanconfig.Targets.

It's not about simplifying client code, it's about concepts. We have a cluster concept in backup/pts, and we have another cluster concept here that means a wholly different thing. They both have the same MakeClusterTarget signature too. It's a bit difficult to see why.