scope uniqueness constraints to affected objects#10019
Conversation
…niquenessChecker Replace the per-node targeted uniqueness path with a single batched TargetedUniquenessValidationQuery per constraint group, paged over the changed nodes. Take branch and schema_branch from the validator request rather than the constructor, and open one read-only session per change instead of per query. Update the dependency builder and the m018 migration for the new signatures, and switch the node-scoped tests to exact violation-set assertions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Optimizes uniqueness validation by scoping checks to only the nodes affected by data diffs, reducing unnecessary full scans. All changes are implementation-level; cubic reviewed and found no issues.
Re-trigger cubic
Merging this PR will improve performance by 30.86%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_base_schema_duplicate_CoreProposedChange |
2.2 ms | 1.5 ms | +49.29% |
| ⚡ | test_schemabranch_duplicate |
7.7 ms | 6.7 ms | +14.7% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ifc-2796-node-scope-uniqueness-2 (7a09ce8) with stable (c993b19)1
Footnotes
There was a problem hiding this comment.
All reported issues were addressed
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| self, candidate_schema: SchemaBranch, schema_diff_constraints: list[SchemaUpdateConstraintInfo] | ||
| ) -> MergeConstraintValidationResult: | ||
| determiner = ConstraintValidatorDeterminer(schema_branch=candidate_schema) | ||
| determiner = build_constraint_validator_determiner( |
There was a problem hiding this comment.
this should be built as part of construction, but the dependencies require schema_branch which is not ready until runtime. I think the right solution is to move schema_branch from dependency to runtime argument for all of these, but I'm going to do that separately
The constraint result model gained a node_uuids field, so model_dump() now emits 'node_uuids': None. Update the two schema-validate tests whose hardcoded expected dicts predate the field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The targeted validation path emitted attribute values with their native graph type, so an integer attribute rendered as value=8 while the full-population path rendered '8'. Stringify the value when building the data path so error messages are identical across both paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The determiner re-initializes its node-diff index on each get_constraints call, but the uniqueness scoper's per-kind scope cache was never cleared, so a reused determiner could return scopes computed from a previous diff. Add a reset() on the scoper and call it alongside index initialization. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The batched targeted query cannot read an attribute of a related peer (a constraint element such as owner__name) and raises on one. Detect such constraints and validate the whole population for that schema instead of scoping to the changed nodes, since the full-population path supports them. Removes the xfail markers on the two peer-attribute tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The diff field summary collapsed all changed node uuids per kind, so changing one node's unique field validated every changed node of that kind. Track uuids per changed field through NodeDiffFieldSummary, the diff query, NodeDiffIndex, and the uniqueness scoper, so a uniqueness check is scoped to only the nodes whose changed field participates in it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 14 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Scopes uniqueness validation to affected nodes per diff, with fallback to full scan for unsupported paths; a bounded, clearly beneficial performance optimization. Implementation verified by Cubic.
Re-trigger cubic
Why
Uniqueness validation scanned the entire population of a kind on every data change. Merging, rebasing, or proposing a change that touched a handful of nodes of a large kind still ran a population-wide scan — slow on big kinds (hundreds of thousands of nodes) and it shipped large duplicate result sets back to Python.
Goal: scope a data-triggered uniqueness check to only the nodes the diff actually affects, so the work is proportional to the size of the change rather than the size of the kind.
Non-goals: this does not change node-save-time uniqueness (
grouped_uniqueness), does not touch other constraint validators, and does not change the uniqueness error surfaced to users.Note: uniqueness constraints do not support attribute of relationships (e.g.
"device__name"). some parts of the code support this kind of uniqueness constraint, but not all of it and it is still blocked when creating/updating a schema.Planned follow-ups
MergeConstraintValidatorconstruction.schema_branchshould probably be an argument for this class and its dependents, not a dependencyPart of IFC-2796 (epic IFC-2706).
What changed
Behavioral
owner__name) fall back to full-population validation, which supports them; the targeted query does not. Again, this type of uniqueness constraint cannot be added to schemas currently.Implementation notes
node_uuidscarrier is threaded end to end:Nonemeans full scan, a list means validate exactly those nodes. It rides from the diff summary through the determiner, the constraint info, and the validator request into the checker.NodeDiffFieldSummary/NodeDiffIndexnow keep the correlation between each changed field and the nodes that changed it (per-field UUID maps), so scoping targets a specific field's nodes rather than every changed node of the kind.build_constraint_validator_determiner):UniquenessConstraintScoperdecides, per kind, whether uniqueness must run and which nodes it affects (None→ full scan); theConstraintValidatorDeterminerupdate threads the affected set through; constraint dedup/merge helpers collapse a node's uniqueness check into its generic's where the generic already covers it.UniquenessCheckernow runs the targeted query for a scoped set, and falls back to the full-population scan when a constraint reads a peer attribute.What stayed the same
grouped_uniqueness) are untouched.Suggested review order
core/diff/model/path.py,core/diff/query/field_summary.py,core/validators/node_diff_index.py— how per-field changed-node UUIDs are captured.core/validators/uniqueness/scope.py+core/validators/determiner.py— how the affected-node set (orNone) is decided.core/validators/uniqueness/checker.py— running the targeted query for a scoped set vs the full-population fallback.proposed_change/tasks.py,merge/constraints.py,branch/tasks.py— the three call sites that thread the affected set through.How to review
Focus on the scope decision (
UniquenessConstraintScoper._compute_scope) and the checker's targeted-vs-full-population branch (UniquenessChecker.check/_supports_targeted) — those govern correctness. Mechanical: theattribute_node_uuidsconversions across the test files.Impact & rollout
Checklist
Summary by cubic
Scope node-level uniqueness validation to only the nodes that changed each unique field (and their dependents), using diff-derived IDs and batched, index-backed queries. Full scans now run only when a constraint is added/broadened or when peer-attribute elements are present, cutting worst-case merge and schema-update time (IFC-2796).
Bug Fixes
Refactors
TargetedUniquenessValidationQuery, paging over changed nodes with one read-only session per change.UniquenessCheckerreads branch/schema from the validator request; dependency builder and m018 migration updated.node_uuidsthroughSchemaUpdateConstraintInfoand validator requests across proposed-change, merge, rebase, and migrations; merge/dedup constraints and standardize onConstraintIdentifier.NODE_UNIQUENESS_CONSTRAINTS_UPDATE.Written for commit 7a09ce8. Summary will update on new commits.