refactor(aliases): consolidate ObjMap and precompute alias transforms#648
refactor(aliases): consolidate ObjMap and precompute alias transforms#648anakrish wants to merge 2 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors Azure Policy alias normalization/denormalization to reduce per-call work by switching internal object maps to the Value::Object representation (BTreeMap<Value, Value>) and by precomputing alias operations/aggregates at registry-load time. This also removes the hashbrown dependency from the azure_policy feature.
Changes:
- Replace
ObjMap’s backing store withBTreeMap<Value, Value>and consolidate shared recursive array traversal helpers inobj_map.rs. - Precompute scalar alias ops, sub-resource rewrap ops, and aggregate lists per API version; add cached
casing_maptoResolvedAliases. - Update normalizer/denormalizer to walk precomputed operation lists; remove now-obsolete denormalizer helper module and drop
hashbrownfrom Cargo.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/azure_policy/normalization/mod.rs | Updates test helper to call rebuild_aggregates() after mutating ResolvedAliases test data. |
| src/languages/azure_policy/aliases/types.rs | Adds cached casing map and new precomputed op types; extends ResolvedEntry with normalized-path precomputations; adds select_aggregates(). |
| src/languages/azure_policy/aliases/obj_map.rs | Switches ObjMap to BTreeMap<Value, Value>; adds new path lookup helpers and shared recursive array traversal helpers; simplifies make_value(). |
| src/languages/azure_policy/aliases/normalizer/element_remap.rs | Replaces bespoke recursion with shared for_each_array_object_in_chain + set_nested. |
| src/languages/azure_policy/aliases/normalizer/alias_resolution.rs | Switches scalar alias normalization to precomputed scalar op list and shared segment-navigation helper. |
| src/languages/azure_policy/aliases/mod.rs | Builds cached casing_map, adds rebuild_aggregates(), and precomputes scalar/sub-resource ops into VersionedAggregates. |
| src/languages/azure_policy/aliases/denormalizer/sub_resource.rs | Moves sub-resource rewrap logic to precomputed ops and shared traversal helpers. |
| src/languages/azure_policy/aliases/denormalizer/mod.rs | Uses cached casing map + precomputed ops for scalar denorm/remaps/rewraps; removes helpers module usage. |
| src/languages/azure_policy/aliases/denormalizer/helpers.rs | Deleted (functionality moved into obj_map). |
| Cargo.toml | Drops hashbrown optional dependency from azure_policy feature. |
| Cargo.lock | Removes hashbrown from dependency graph. |
| bindings/ffi/Cargo.lock | Removes hashbrown from dependency graph for the FFI workspace lockfile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
anakrish
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Switch ObjMap from hashbrown::HashMap<Rc<str>, Value> to BTreeMap<Value, Value>, matching the Value::Object representation. make_value() becomes Rc::new() with no conversion overhead. Drops the hashbrown dependency from the azure_policy module. Consolidate duplicated recursive array traversal from element_remap and sub_resource into shared helpers in obj_map.rs (for_each_array_object_in_chain, for_each_array_object_in_path_ci). This eliminates the parallel ObjMap/BTreeMap recursion variants that existed to avoid conversion at each nesting level, now unnecessary since ObjMap is BTreeMap. Merge set_nested_lowercased and set_nested_verbatim into a single set_nested with a bool flag. Build precomputed operation lists (VersionedAggregates) at registry load time so normalize and denormalize walk flat vectors instead of iterating all entries and filtering per call: - PrecomputedScalarNormalize / PrecomputedScalarDenormalize - PrecomputedSubResourceRewrap - alias_owned_normalized_roots Cache the casing map and normalized key segments on ResolvedAliases and ResolvedEntry respectively. Store original ARM casing directly in PrecomputedReverseRemap.target_field to avoid per-call casing restoration. Delete denormalizer/helpers.rs (find_key_ci moved to obj_map.rs). Fix three clippy warnings (redundant field name, redundant pub(crate), missing const fn). Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
anakrish
force-pushed
the
normalizer
branch
2 times, most recently
from
52dfe89 to
635ab43
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…n denorm Phase 2a - In compute_aggregates_for_version, skip non-wildcard entries whose short name matches a detected sub-resource array. Without this filter, scalar alias normalization would overwrite the properly flattened sub-resource array with a non-flattened copy. - Populate alias_owned_normalized_roots for all non-wildcard scalar aliases (not just single-segment ones) so Phase 2a correctly skips nested alias roots like sku.name. - Change get_path_exact_or_ci to accept &[String] directly instead of &[&str], eliminating the per-op Vec<&str> allocation in the scalar denormalization loop. Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>
Switch ObjMap from hashbrown::HashMap<Rc, Value> to
BTreeMap<Value, Value>, matching the Value::Object representation.
make_value() becomes Rc::new() with no conversion overhead. Drops
the hashbrown dependency from the azure_policy module.
Consolidate duplicated recursive array traversal from element_remap
and sub_resource into shared helpers in obj_map.rs
(for_each_array_object_in_chain, for_each_array_object_in_path_ci).
This eliminates the parallel ObjMap/BTreeMap recursion variants that
existed to avoid conversion at each nesting level, now unnecessary
since ObjMap is BTreeMap. Merge set_nested_lowercased and
set_nested_verbatim into a single set_nested with a bool flag.
Build precomputed operation lists (VersionedAggregates) at registry
load time so normalize and denormalize walk flat vectors instead of
iterating all entries and filtering per call:
Cache the casing map and normalized key segments on ResolvedAliases
and ResolvedEntry respectively. Store original ARM casing directly in
PrecomputedReverseRemap.target_field to avoid per-call casing
restoration.
Delete denormalizer/helpers.rs (find_key_ci moved to obj_map.rs).
Fix three clippy warnings (redundant field name, redundant pub(crate),
missing const fn).