From 5b99f7116abd5856e7c8415fbe626ee5b540ce57 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Wed, 28 Jan 2026 15:33:33 -0800 Subject: [PATCH 1/3] Implement compareTargetGraphs Update Update test Update --- core/common/BUILD.bazel | 11 ++++- core/common/nameidmapper.go | 41 +++++++++++++++++ core/common/nameidmapper_test.go | 46 +++++++++++++++++++ core/config/BUILD.bazel | 2 +- core/targethasher/BUILD.bazel | 1 + core/targethasher/graph.go | 78 ++++++-------------------------- 6 files changed, 111 insertions(+), 68 deletions(-) create mode 100644 core/common/nameidmapper.go create mode 100644 core/common/nameidmapper_test.go diff --git a/core/common/BUILD.bazel b/core/common/BUILD.bazel index d18e3e92..80232631 100644 --- a/core/common/BUILD.bazel +++ b/core/common/BUILD.bazel @@ -2,7 +2,10 @@ load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "common", - srcs = ["utils.go"], + srcs = [ + "nameidmapper.go", + "utils.go", + ], importpath = "github.com/uber/tango/core/common", visibility = ["//visibility:public"], deps = ["//tangopb"], @@ -10,9 +13,13 @@ go_library( go_test( name = "common_test", - srcs = ["utils_test.go"], + srcs = [ + "nameidmapper_test.go", + "utils_test.go", + ], embed = [":common"], deps = [ "//tangopb", + "@com_github_stretchr_testify//assert", ], ) diff --git a/core/common/nameidmapper.go b/core/common/nameidmapper.go new file mode 100644 index 00000000..3ba0bb5c --- /dev/null +++ b/core/common/nameidmapper.go @@ -0,0 +1,41 @@ +package common + +// NameIDMapper assigns stable int32 IDs to string names on demand. +// IDs are assigned sequentially starting from 0. +type NameIDMapper struct { + nameToID map[string]int32 + nextID int32 +} + +// NewNameIdMapper creates a new NameIdMapper. +func NewNameIDMapper() *NameIDMapper { + return &NameIDMapper{ + nameToID: make(map[string]int32), + nextID: 0, + } +} + +// ID returns the existing ID for the provided name or assigns a new one. +func (a *NameIDMapper) ID(name string) int32 { + if id, ok := a.nameToID[name]; ok { + return id + } + id := a.nextID + a.nextID++ + a.nameToID[name] = id + return id +} + +// Mapping returns the underlying name->id map. +func (a *NameIDMapper) Mapping() map[string]int32 { + return a.nameToID +} + +// Invert returns an id->name map built from the current name->id map. +func (a *NameIDMapper) Invert() map[int32]string { + out := make(map[int32]string, len(a.nameToID)) + for name, id := range a.nameToID { + out[id] = name + } + return out +} diff --git a/core/common/nameidmapper_test.go b/core/common/nameidmapper_test.go new file mode 100644 index 00000000..a97cfa33 --- /dev/null +++ b/core/common/nameidmapper_test.go @@ -0,0 +1,46 @@ +package common + +import ( + "testing" + "github.com/stretchr/testify/assert" +) + +func TestNameIDMapper_AssignsSequentialAndStableIDs(t *testing.T) { + mapper := NewNameIDMapper() + + idA := mapper.ID("a") + assert.Equal(t, int32(0), idA) + idB := mapper.ID("b") + assert.Equal(t, int32(1), idB) + // Re-requesting 'a' should return the same id + idA2 := mapper.ID("a") + assert.Equal(t, idA, idA2) + // A new, third name should get the next sequential id + idC := mapper.ID("c") + assert.Equal(t, int32(2), idC) +} + +func TestNameIDMapper_MappingAndInvert(t *testing.T) { + mapper := NewNameIDMapper() + names := []string{"x", "y", "z"} + for _, n := range names { + mapper.ID(n) + } + + mapping := mapper.Mapping() + assert.Equal(t, 3, len(mapping)) + assert.Equal(t, int32(0), mapping["x"]) + assert.Equal(t, int32(1), mapping["y"]) + assert.Equal(t, int32(2), mapping["z"]) + + inv := mapper.Invert() + assert.Equal(t, 3, len(inv)) + assert.Equal(t, "x", inv[0]) + assert.Equal(t, "y", inv[1]) + assert.Equal(t, "z", inv[2]) + + // Mutating the inverted map should not affect the original mapping + inv[3] = "extra" + _, ok := mapping["extra"] + assert.False(t, ok) +} diff --git a/core/config/BUILD.bazel b/core/config/BUILD.bazel index 22d7ec34..0c5cf4bd 100644 --- a/core/config/BUILD.bazel +++ b/core/config/BUILD.bazel @@ -5,5 +5,5 @@ go_library( srcs = ["config.go"], importpath = "github.com/uber/tango/core/config", visibility = ["//visibility:public"], - deps = ["@com_github_goccy_go_yaml//:go_default_library"], + deps = ["@com_github_goccy_go_yaml//:go-yaml"], ) diff --git a/core/targethasher/BUILD.bazel b/core/targethasher/BUILD.bazel index e7ab20e6..cbfa146f 100644 --- a/core/targethasher/BUILD.bazel +++ b/core/targethasher/BUILD.bazel @@ -9,6 +9,7 @@ go_library( importpath = "github.com/uber/tango/core/targethasher", visibility = ["//visibility:public"], deps = [ + "//core/common", "//tangopb", "@com_github_bazelbuild_buildtools//build_proto", "@com_github_bazelbuild_buildtools//labels", diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index ae26f6ca..7292ad89 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -6,6 +6,7 @@ import ( "sort" "strings" + "github.com/uber/tango/core/common" "github.com/uber/tango/tangopb" buildpb "github.com/bazelbuild/buildtools/build_proto" ) @@ -515,62 +516,24 @@ func ResultToGetTargetGraphResponse(result Result) ([]*tangopb.GetTargetGraphRes targetNamesMapping[name] = int32(i) } - ruleTypeMapping := make(map[string]int32) - var ruleTypeCurr int32 - getRuleTypeID := func(key string) int32 { - if id, ok := ruleTypeMapping[key]; ok { - return id - } - id := ruleTypeCurr - ruleTypeCurr++ - ruleTypeMapping[key] = id - return id - } + ruleTypeMapper := common.NewNameIDMapper() + getRuleTypeID := func(key string) int32 { return ruleTypeMapper.ID(key) } - tagMapping := make(map[string]int32) - var tagCurr int32 - getTagID := func(key string) int32 { - if id, ok := tagMapping[key]; ok { - return id - } - id := tagCurr - tagCurr++ - tagMapping[key] = id - return id - } + tagMapper := common.NewNameIDMapper() + getTagID := func(key string) int32 { return tagMapper.ID(key) } - attrNameMapping := make(map[string]int32) - var attrNameCurr int32 - getAttrNameID := func(key string) int32 { - if id, ok := attrNameMapping[key]; ok { - return id - } - id := attrNameCurr - attrNameCurr++ - attrNameMapping[key] = id - return id - } + attrNameMapper := common.NewNameIDMapper() + getAttrNameID := func(key string) int32 { return attrNameMapper.ID(key) } - attrStrValMapping := make(map[string]int32) - var attrStrValCurr int32 - getAttrStrValID := func(key string) int32 { - if id, ok := attrStrValMapping[key]; ok { - return id - } - id := attrStrValCurr - attrStrValCurr++ - attrStrValMapping[key] = id - return id - } + attrStrValMapper := common.NewNameIDMapper() + getAttrStrValID := func(key string) int32 { return attrStrValMapper.ID(key) } // Build the optimized targets slice optimizedTargets := make([]*tangopb.OptimizedTarget, 0, len(result.Targets)) for _, t := range result.Targets { - // Intern name nameID := targetNamesMapping[t.Name] - // Intern dependencies that are also in targetNames depIDs := make([]int32, 0, len(t.Deps)) for _, depName := range t.Deps { if _, ok := targetNamesMapping[depName]; !ok { @@ -623,25 +586,10 @@ func ResultToGetTargetGraphResponse(result Result) ([]*tangopb.GetTargetGraphRes targetIDToName[id] = s } - ruleTypeIDToName := make(map[int32]string, len(ruleTypeMapping)) - for s, id := range ruleTypeMapping { - ruleTypeIDToName[id] = s - } - - tagIDToName := make(map[int32]string, len(tagMapping)) - for s, id := range tagMapping { - tagIDToName[id] = s - } - - attrNameIDToName := make(map[int32]string, len(attrNameMapping)) - for s, id := range attrNameMapping { - attrNameIDToName[id] = s - } - - attrStrValIDToVal := make(map[int32]string, len(attrStrValMapping)) - for s, id := range attrStrValMapping { - attrStrValIDToVal[id] = s - } + ruleTypeIDToName := ruleTypeMapper.Invert() + tagIDToName := tagMapper.Invert() + attrNameIDToName := attrNameMapper.Invert() + attrStrValIDToVal := attrStrValMapper.Invert() // Assemble final OptimizedTargets return []*tangopb.GetTargetGraphResponse{ From 02c9c0da8737f0fe017466199bc62aca07ac6906 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Thu, 29 Jan 2026 16:27:16 -0800 Subject: [PATCH 2/3] Update --- core/common/utils.go | 108 +++++++++++++++++++++++++++++++++++++ core/targethasher/graph.go | 106 ------------------------------------ 2 files changed, 108 insertions(+), 106 deletions(-) diff --git a/core/common/utils.go b/core/common/utils.go index 5948465c..0641f35c 100644 --- a/core/common/utils.go +++ b/core/common/utils.go @@ -33,3 +33,111 @@ func getReqsBase64(requestURLs []string) string { } return strings.Join(encodedURLs, "-") } + + + +// ResultToGetTargetGraphResponse converts a Result to a GetTargetGraphResponse +func ResultToGetTargetGraphResponse(result Result) ([]*tangopb.GetTargetGraphResponse, error) { + // Map target names to ids. This list is topologically sorted, so the ids are stable. + targetNamesMapping := make(map[string]int32, len(result.TargetNames)) + for i, name := range result.TargetNames { + targetNamesMapping[name] = int32(i) + } + + ruleTypeMapper := NewNameIDMapper() + getRuleTypeID := func(key string) int32 { return ruleTypeMapper.ID(key) } + + tagMapper := NewNameIDMapper() + getTagID := func(key string) int32 { return tagMapper.ID(key) } + + attrNameMapper := NewNameIDMapper() + getAttrNameID := func(key string) int32 { return attrNameMapper.ID(key) } + + attrStrValMapper := NewNameIDMapper() + getAttrStrValID := func(key string) int32 { return attrStrValMapper.ID(key) } + + // Build the optimized targets slice + optimizedTargets := make([]*tangopb.OptimizedTarget, 0, len(result.Targets)) + + for _, t := range result.Targets { + nameID := targetNamesMapping[t.Name] + + depIDs := make([]int32, 0, len(t.Deps)) + for _, depName := range t.Deps { + if _, ok := targetNamesMapping[depName]; !ok { + continue + } + depIDs = append(depIDs, targetNamesMapping[depName]) + } + + ot := &tangopb.OptimizedTarget{ + Id: nameID, + Hash: string(t.Hash), + DirectDependencies: depIDs, + } + + // RuleType + if t.RuleType != "" { + id := getRuleTypeID(t.RuleType) + ot.RuleType = id + } + + // Tags + if len(t.Tags) > 0 { + tagIDs := make([]int32, 0, len(t.Tags)) + for _, tag := range t.Tags { + tagIDs = append(tagIDs, getTagID(tag)) + } + ot.Tags = tagIDs + } + ot.Root = t.Root + ot.External = t.External + if len(t.Attributes) > 0 { + attrs := make(map[int32]int32, len(t.Attributes)) + for _, attr := range t.Attributes { + // Only include STRING attributes with non-nil name and value to avoid nil dereferences. + if attr.GetType() == buildpb.Attribute_STRING && attr.Name != nil && attr.StringValue != nil { + nameID := getAttrNameID(*attr.Name) + valID := getAttrStrValID(*attr.StringValue) + attrs[nameID] = valID + } + } + ot.Attributes = attrs + } + + optimizedTargets = append(optimizedTargets, ot) + } + + // Invert mappings: string -> id => id -> string + targetIDToName := make(map[int32]string, len(targetNamesMapping)) + for s, id := range targetNamesMapping { + targetIDToName[id] = s + } + + ruleTypeIDToName := ruleTypeMapper.Invert() + tagIDToName := tagMapper.Invert() + attrNameIDToName := attrNameMapper.Invert() + attrStrValIDToVal := attrStrValMapper.Invert() + + // Assemble final OptimizedTargets + return []*tangopb.GetTargetGraphResponse{ + { + Item: &tangopb.GetTargetGraphResponse_Targets{ + Targets: &tangopb.OptimizedTargets{ + Targets: optimizedTargets, + }, + }, + }, + { + Item: &tangopb.GetTargetGraphResponse_Metadata{ + Metadata: &tangopb.Metadata{ + TargetIdMapping: targetIDToName, + RuleTypeMapping: ruleTypeIDToName, + TagMapping: tagIDToName, + AttributeNameMapping: attrNameIDToName, + AttributeStringValueMapping: attrStrValIDToVal, + }, + }, + }, + }, nil +} diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index 7292ad89..a121a20a 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -507,109 +507,3 @@ func hasAnyPrefix(t *buildpb.Target, rootRulePrefixes []string) bool { } return false } - -// ResultToGetTargetGraphResponse converts a Result to a GetTargetGraphResponse -func ResultToGetTargetGraphResponse(result Result) ([]*tangopb.GetTargetGraphResponse, error) { - // Map target names to ids. This list is topologically sorted, so the ids are stable. - targetNamesMapping := make(map[string]int32, len(result.TargetNames)) - for i, name := range result.TargetNames { - targetNamesMapping[name] = int32(i) - } - - ruleTypeMapper := common.NewNameIDMapper() - getRuleTypeID := func(key string) int32 { return ruleTypeMapper.ID(key) } - - tagMapper := common.NewNameIDMapper() - getTagID := func(key string) int32 { return tagMapper.ID(key) } - - attrNameMapper := common.NewNameIDMapper() - getAttrNameID := func(key string) int32 { return attrNameMapper.ID(key) } - - attrStrValMapper := common.NewNameIDMapper() - getAttrStrValID := func(key string) int32 { return attrStrValMapper.ID(key) } - - // Build the optimized targets slice - optimizedTargets := make([]*tangopb.OptimizedTarget, 0, len(result.Targets)) - - for _, t := range result.Targets { - nameID := targetNamesMapping[t.Name] - - depIDs := make([]int32, 0, len(t.Deps)) - for _, depName := range t.Deps { - if _, ok := targetNamesMapping[depName]; !ok { - continue - } - depIDs = append(depIDs, targetNamesMapping[depName]) - } - - ot := &tangopb.OptimizedTarget{ - Id: nameID, - Hash: string(t.Hash), - DirectDependencies: depIDs, - } - - // RuleType - if t.RuleType != "" { - id := getRuleTypeID(t.RuleType) - ot.RuleType = id - } - - // Tags - if len(t.Tags) > 0 { - tagIDs := make([]int32, 0, len(t.Tags)) - for _, tag := range t.Tags { - tagIDs = append(tagIDs, getTagID(tag)) - } - ot.Tags = tagIDs - } - ot.Root = t.Root - ot.External = t.External - if len(t.Attributes) > 0 { - attrs := make(map[int32]int32, len(t.Attributes)) - for _, attr := range t.Attributes { - // Only include STRING attributes with non-nil name and value to avoid nil dereferences. - if attr.GetType() == buildpb.Attribute_STRING && attr.Name != nil && attr.StringValue != nil { - nameID := getAttrNameID(*attr.Name) - valID := getAttrStrValID(*attr.StringValue) - attrs[nameID] = valID - } - } - ot.Attributes = attrs - } - - optimizedTargets = append(optimizedTargets, ot) - } - - // Invert mappings: string -> id => id -> string - targetIDToName := make(map[int32]string, len(targetNamesMapping)) - for s, id := range targetNamesMapping { - targetIDToName[id] = s - } - - ruleTypeIDToName := ruleTypeMapper.Invert() - tagIDToName := tagMapper.Invert() - attrNameIDToName := attrNameMapper.Invert() - attrStrValIDToVal := attrStrValMapper.Invert() - - // Assemble final OptimizedTargets - return []*tangopb.GetTargetGraphResponse{ - { - Item: &tangopb.GetTargetGraphResponse_Targets{ - Targets: &tangopb.OptimizedTargets{ - Targets: optimizedTargets, - }, - }, - }, - { - Item: &tangopb.GetTargetGraphResponse_Metadata{ - Metadata: &tangopb.Metadata{ - TargetIdMapping: targetIDToName, - RuleTypeMapping: ruleTypeIDToName, - TagMapping: tagIDToName, - AttributeNameMapping: attrNameIDToName, - AttributeStringValueMapping: attrStrValIDToVal, - }, - }, - }, - }, nil -} From f9a77399a0128eba085f5128343a28b284754855 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Thu, 29 Jan 2026 16:39:11 -0800 Subject: [PATCH 3/3] Update --- core/common/BUILD.bazel | 6 +++++- core/common/utils.go | 7 +++++-- core/targethasher/BUILD.bazel | 1 - core/targethasher/graph.go | 2 -- orchestrator/BUILD.bazel | 1 - orchestrator/native_orchestrator.go | 3 +-- 6 files changed, 11 insertions(+), 9 deletions(-) diff --git a/core/common/BUILD.bazel b/core/common/BUILD.bazel index 80232631..4d28f1ca 100644 --- a/core/common/BUILD.bazel +++ b/core/common/BUILD.bazel @@ -8,7 +8,11 @@ go_library( ], importpath = "github.com/uber/tango/core/common", visibility = ["//visibility:public"], - deps = ["//tangopb"], + deps = [ + "//core/targethasher", + "//tangopb", + "@com_github_bazelbuild_buildtools//build_proto", + ], ) go_test( diff --git a/core/common/utils.go b/core/common/utils.go index 0641f35c..8f4127a5 100644 --- a/core/common/utils.go +++ b/core/common/utils.go @@ -1,10 +1,13 @@ package common import ( - "github.com/uber/tango/tangopb" "encoding/base64" "path/filepath" "strings" + + buildpb "github.com/bazelbuild/buildtools/build_proto" + "github.com/uber/tango/core/targethasher" + "github.com/uber/tango/tangopb" ) // ToShortRemote returns the short remote name given a git ssh remote string. @@ -37,7 +40,7 @@ func getReqsBase64(requestURLs []string) string { // ResultToGetTargetGraphResponse converts a Result to a GetTargetGraphResponse -func ResultToGetTargetGraphResponse(result Result) ([]*tangopb.GetTargetGraphResponse, error) { +func ResultToGetTargetGraphResponse(result targethasher.Result) ([]*tangopb.GetTargetGraphResponse, error) { // Map target names to ids. This list is topologically sorted, so the ids are stable. targetNamesMapping := make(map[string]int32, len(result.TargetNames)) for i, name := range result.TargetNames { diff --git a/core/targethasher/BUILD.bazel b/core/targethasher/BUILD.bazel index cbfa146f..e7ab20e6 100644 --- a/core/targethasher/BUILD.bazel +++ b/core/targethasher/BUILD.bazel @@ -9,7 +9,6 @@ go_library( importpath = "github.com/uber/tango/core/targethasher", visibility = ["//visibility:public"], deps = [ - "//core/common", "//tangopb", "@com_github_bazelbuild_buildtools//build_proto", "@com_github_bazelbuild_buildtools//labels", diff --git a/core/targethasher/graph.go b/core/targethasher/graph.go index a121a20a..5bb0edf7 100644 --- a/core/targethasher/graph.go +++ b/core/targethasher/graph.go @@ -6,8 +6,6 @@ import ( "sort" "strings" - "github.com/uber/tango/core/common" - "github.com/uber/tango/tangopb" buildpb "github.com/bazelbuild/buildtools/build_proto" ) diff --git a/orchestrator/BUILD.bazel b/orchestrator/BUILD.bazel index dbe696ab..039be691 100644 --- a/orchestrator/BUILD.bazel +++ b/orchestrator/BUILD.bazel @@ -16,7 +16,6 @@ go_library( "//core/git", "//core/repomanager", "//core/storage", - "//core/targethasher", "//core/workspace", "//tangopb", "@org_uber_go_zap//:zap", diff --git a/orchestrator/native_orchestrator.go b/orchestrator/native_orchestrator.go index 98755f53..394f92ab 100644 --- a/orchestrator/native_orchestrator.go +++ b/orchestrator/native_orchestrator.go @@ -12,7 +12,6 @@ import ( "github.com/uber/tango/core/git" "github.com/uber/tango/core/repomanager" "github.com/uber/tango/core/storage" - "github.com/uber/tango/core/targethasher" "github.com/uber/tango/core/workspace" "go.uber.org/zap" "time" @@ -136,7 +135,7 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, param GetTarget b.logger.Error("getGraph: Error computing target graph", zap.Any("request build description", param.Req.BuildDescription), zap.Error(err)) return nil, err } - graphs, err := targethasher.ResultToGetTargetGraphResponse(result) + graphs, err := common.ResultToGetTargetGraphResponse(result) if err != nil { b.logger.Error("getGraph: Error converting target graph to GetTargetGraphResponse", zap.Any("request build description", param.Req.BuildDescription), zap.Error(err)) return nil, err