Avoid repeated prefix scans when projecting item policies
This commit is contained in:
@@ -1,7 +1,7 @@
|
|||||||
# Sequence Implementation Review Remediation
|
# Sequence Implementation Review Remediation
|
||||||
|
|
||||||
Status: remediation in progress. Completed findings are marked below; this
|
Status: all eleven findings complete. Each finding has a separate remediation
|
||||||
record introduces no new release gate.
|
commit and completion note below. This record introduces no new release gate.
|
||||||
|
|
||||||
Reviewed commit: `0be7cdb00eff576fda6b30c0600b2134d4e2e086`.
|
Reviewed commit: `0be7cdb00eff576fda6b30c0600b2134d4e2e086`.
|
||||||
Implementation range: `bc6b977..0be7cdb`.
|
Implementation range: `bc6b977..0be7cdb`.
|
||||||
@@ -25,6 +25,10 @@ Validation on the reviewed commit:
|
|||||||
- Finding 10 follows directly from the maintained text; finding 11 follows from
|
- Finding 10 follows directly from the maintained text; finding 11 follows from
|
||||||
the prefix-scan loop, not a timing comparison or performance threshold.
|
the prefix-scan loop, not a timing comparison or performance threshold.
|
||||||
|
|
||||||
|
Final validation after remediation: `./scripts/check.sh` and
|
||||||
|
`go test -race ./...` passed with Go 1.26.10 on Linux. The original public-API
|
||||||
|
probes for findings 1–9 now demonstrate the required corrected behavior.
|
||||||
|
|
||||||
P1 denotes the highest-priority correctness issue here; P2 denotes normal-priority
|
P1 denotes the highest-priority correctness issue here; P2 denotes normal-priority
|
||||||
work needed to complete the selected behavior. Preserve the agreed scope while
|
work needed to complete the selected behavior. Preserve the agreed scope while
|
||||||
fixing these issues. In particular, do not resolve failures by restricting valid
|
fixing these issues. In particular, do not resolve failures by restricting valid
|
||||||
@@ -376,7 +380,16 @@ Keep YAML-only enforcement distinct from registered final validation.
|
|||||||
|
|
||||||
## 11. Remove The Quadratic Prefix Scan In Item Policy Projection — P2
|
## 11. Remove The Quadratic Prefix Scan In Item Policy Projection — P2
|
||||||
|
|
||||||
Status: [ ] Open.
|
Status: [x] Complete.
|
||||||
|
|
||||||
|
Completion: assigned sequence ranges now supply item offsets directly. Handoffs
|
||||||
|
without a range lazily accumulate prefix lengths, visiting each preceding
|
||||||
|
contribution once rather than rescanning every prefix. Regression coverage keeps
|
||||||
|
inline and policy-less contributions in the correct positions with complete,
|
||||||
|
partial, or absent ranges. Existing ownership/placement tests remain intact.
|
||||||
|
Verified focused item-policy tests, `./scripts/check.sh`, `go test -race ./...`,
|
||||||
|
and inspection of the remaining error-only prefix-scan callers. No new benchmark
|
||||||
|
fixture or performance threshold was needed for this bounded change.
|
||||||
|
|
||||||
Owner: [`projectNestedCollectionPolicies`](../../internal/engine/imports.go),
|
Owner: [`projectNestedCollectionPolicies`](../../internal/engine/imports.go),
|
||||||
especially line 968; the scanned helper is `priorSequenceLength` in the same file.
|
especially line 968; the scanned helper is `priorSequenceLength` in the same file.
|
||||||
|
|||||||
@@ -961,17 +961,28 @@ func projectNestedCollectionPolicies(composed *composedDocument, scope acquisiti
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
baseOffsets := make(map[string]int)
|
baseOffsets := make(map[string]int)
|
||||||
|
var prefixLengths map[string]int
|
||||||
for sourceIndex, contribution := range scope.contributions {
|
for sourceIndex, contribution := range scope.contributions {
|
||||||
|
if prefixLengths != nil && sourceIndex > 0 {
|
||||||
|
addSequencePrefixLengths(scope.contributions[sourceIndex-1].document, Path{}, prefixLengths)
|
||||||
|
}
|
||||||
if contribution.nested == nil || len(contribution.nested.collectionPolicies.collections) == 0 {
|
if contribution.nested == nil || len(contribution.nested.collectionPolicies.collections) == 0 {
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
itemIndex := priorSequenceLength([]acquisitionScope{{contributions: scope.contributions[:sourceIndex]}}, contribution.destination)
|
|
||||||
if assigned, found := ranges[sequenceRangeKey{sourceIndex: sourceIndex, path: contribution.destination.String()}]; found {
|
|
||||||
if projected, ok := assigned.outputForInput(0); ok {
|
|
||||||
itemIndex = projected
|
|
||||||
}
|
|
||||||
}
|
|
||||||
key := contribution.destination.String()
|
key := contribution.destination.String()
|
||||||
|
assigned := ranges[sequenceRangeKey{sourceIndex: sourceIndex, path: key}]
|
||||||
|
itemIndex, found := assigned.outputForInput(0)
|
||||||
|
if !found {
|
||||||
|
// Normal composition supplies the assignment directly. For a
|
||||||
|
// handoff without ranges, scan each preceding tree only once.
|
||||||
|
if prefixLengths == nil {
|
||||||
|
prefixLengths = make(map[string]int)
|
||||||
|
for _, previous := range scope.contributions[:sourceIndex] {
|
||||||
|
addSequencePrefixLengths(previous.document, Path{}, prefixLengths)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
itemIndex = prefixLengths[key]
|
||||||
|
}
|
||||||
base, initialized := baseOffsets[key]
|
base, initialized := baseOffsets[key]
|
||||||
policy, _ := composed.collectionPolicies.at(contribution.destination)
|
policy, _ := composed.collectionPolicies.at(contribution.destination)
|
||||||
if !initialized {
|
if !initialized {
|
||||||
@@ -986,6 +997,20 @@ func projectNestedCollectionPolicies(composed *composedDocument, scope acquisiti
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func addSequencePrefixLengths(node *document.Node, path Path, lengths map[string]int) {
|
||||||
|
if node == nil {
|
||||||
|
return
|
||||||
|
}
|
||||||
|
if node.Kind == document.Sequence {
|
||||||
|
lengths[path.String()] += len(node.Sequence)
|
||||||
|
return
|
||||||
|
}
|
||||||
|
// Contribution destinations select mapping paths, not sequence members.
|
||||||
|
for _, entry := range node.Mapping {
|
||||||
|
addSequencePrefixLengths(entry.Value, path.Key(entry.Key), lengths)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func projectNestedOrigins(target map[string]Origin, root *document.Node, origins map[string]Origin, prefix Path) {
|
func projectNestedOrigins(target map[string]Origin, root *document.Node, origins map[string]Origin, prefix Path) {
|
||||||
var visit func(*document.Node, Path)
|
var visit func(*document.Node, Path)
|
||||||
visit = func(node *document.Node, path Path) {
|
visit = func(node *document.Node, path Path) {
|
||||||
|
|||||||
@@ -195,6 +195,40 @@ func TestNestedItemPolicyProjectionUsesStableOutputIndexes(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestItemPolicyProjectionCountsContributionsWithoutLocalPolicy(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
items := Keys("items")
|
||||||
|
for _, assignments := range []map[int]int{nil, {1: 2, 3: 4}, {1: 2}, {3: 4}} {
|
||||||
|
t.Run(fmt.Sprint(assignments), func(t *testing.T) {
|
||||||
|
local := collectionPolicyScope{}
|
||||||
|
local.set(Keys("steps"), collectionPolicy{identityFields: []string{"name"}, identityPresent: true})
|
||||||
|
scope := acquisitionScope{contributions: []acquiredContribution{
|
||||||
|
{document: parseItemPolicyDocument(t, "inline", "items: [{id: inline-a}, {id: inline-b}]\n")},
|
||||||
|
{document: parseItemPolicyDocument(t, "first", "items: [{id: first}]\n"), destination: items, nested: &composedDocument{collectionPolicies: local}},
|
||||||
|
{document: parseItemPolicyDocument(t, "gap", "items: [{id: gap}]\n"), destination: items},
|
||||||
|
{document: parseItemPolicyDocument(t, "second", "items: [{id: second}]\n"), destination: items, nested: &composedDocument{collectionPolicies: local}},
|
||||||
|
}}
|
||||||
|
ranges := make(map[sequenceRangeKey]sequenceRange)
|
||||||
|
for sourceIndex, output := range assignments {
|
||||||
|
ranges[sequenceRangeKey{sourceIndex: sourceIndex, path: items.String()}] = sequenceRange{start: output, end: output + 1}
|
||||||
|
}
|
||||||
|
composed := &composedDocument{}
|
||||||
|
projectNestedCollectionPolicies(composed, scope, ranges)
|
||||||
|
policy, _ := composed.collectionPolicies.at(items)
|
||||||
|
if len(policy.recordOrder) != 5 {
|
||||||
|
t.Fatalf("record policies = %d, want 5 positions", len(policy.recordOrder))
|
||||||
|
}
|
||||||
|
for index := range 5 {
|
||||||
|
record, _ := policy.recordPolicyAt(index)
|
||||||
|
_, found := record.scope.at(Keys("steps"))
|
||||||
|
if found != (index == 2 || index == 4) {
|
||||||
|
t.Errorf("local policy at item %d = %t", index, found)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func TestOverlaySequenceTransfersIncomingItemPolicyOwnership(t *testing.T) {
|
func TestOverlaySequenceTransfersIncomingItemPolicyOwnership(t *testing.T) {
|
||||||
path := Keys("records")
|
path := Keys("records")
|
||||||
registered := mustCompileSequenceIdentityPolicy(t, []sequenceIdentityRegistration{{selector: sequenceIdentitySelector{literalSequenceSelector("records")}, identityFields: []string{"id"}}})
|
registered := mustCompileSequenceIdentityPolicy(t, []sequenceIdentityRegistration{{selector: sequenceIdentitySelector{literalSequenceSelector("records")}, identityFields: []string{"id"}}})
|
||||||
|
|||||||
Reference in New Issue
Block a user