fix(itg): only propagate string-valued attributes through the optimiz… - #316
Merged
Merged
Conversation
…ed graph OptimizedGraph.AddTarget and upsertTarget previously recorded every target attribute via GetName()/GetStringValue(), regardless of its underlying type. For non-string attributes (bool, int, label lists, etc.) this silently coerced values through the zero-value string getters, corrupting the stored attribute map. Only record attributes whose Type is buildpb.Attribute_STRING and whose Name/StringValue are non-nil. OptimizedTargetToTarget now also sets Type explicitly to buildpb.Attribute_STRING when reconstructing attributes, since every attribute stored in the optimized graph is now guaranteed to be string-valued and downstream consumers that check GetType() should not see an ambiguous zero-value default. Co-Authored-By: Claude Sonnet 5 <ANONYMIZED_EMAIL_ADDRESS_0_5>
yushan8
marked this pull request as draft
September 22, 2026 21:51
yushan8
marked this pull request as ready for review
September 22, 2026 21:58
xytan0056
reviewed
Sep 22, 2026
xytan0056
reviewed
Sep 22, 2026
| continue | ||
| } | ||
| attrNameID := getOrGenerateRecordReverse(*attr.Name, g.AttrNameToID, g.AttrNameIDToString) | ||
| attrValueID := getOrGenerateRecordReverse(*attr.StringValue, g.AttrValueToID, g.AttrValueIDToString) |
Contributor
There was a problem hiding this comment.
also btw does stringList order matter? such as tags = ["a", "b"] vs tags = ["b", "a"]
Contributor
Author
There was a problem hiding this comment.
good q, it does. Added a test to verify.
xytan0056
approved these changes
Sep 22, 2026
Make the STRING-type attribute filter in AddTarget/upsertTarget explicitly skip a nil *buildpb.Attribute element instead of relying on GetType()'s nil-safe default (Attribute_INTEGER) short-circuiting the `||` before the nil-unsafe Name/StringValue field reads. Behavior is unchanged, but the safety is now explicit rather than incidental. Co-Authored-By: Claude Sonnet 5 <ANONYMIZED_EMAIL_ADDRESS_0_6>
Directly answers the PR review question on this branch about whether StringListValue order (e.g. tags = ["a","b"] vs ["b","a"]) affects the target hash. It doesn't: HashRuleCommon's canonical encoder sorts string-list attribute values before hashing, so itg's computeAvailableHashes produces an identical HashWithoutDeps for both orderings. Verified this assertion actually exercises the sort by temporarily removing it and confirming the new test fails. Co-Authored-By: Claude Sonnet 5 <ANONYMIZED_EMAIL_ADDRESS_0_7>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OptimizedGraph.AddTarget and upsertTarget previously recorded every target attribute via GetName()/GetStringValue(), regardless of its underlying type. For non-string attributes (bool, int, label lists, etc.) this silently coerced values through the zero-value string getters, corrupting the stored attribute map.
Only record attributes whose Type is buildpb.Attribute_STRING and whose Name/StringValue are non-nil. OptimizedTargetToTarget now also sets Type explicitly to buildpb.Attribute_STRING when reconstructing attributes, since every attribute stored in the optimized graph is now guaranteed to be string-valued and downstream consumers that check GetType() should not see an ambiguous zero-value default. This matches the OptimizedTarget graph format with tgb as well.
TODO: support non-string based attribute values #318
Test Plan
unit tests and tested internally