Skip to content

feat(value): introduce Set storage abstraction - #60

Open
anakrish wants to merge 3 commits into
mainfrom
storage-abstraction-set-foundation
Open

feat(value): introduce Set storage abstraction#60
anakrish wants to merge 3 commits into
mainfrom
storage-abstraction-set-foundation

Conversation

@anakrish

@anakrish anakrish commented Jun 2, 2026

Copy link
Copy Markdown
Owner

Adds an opaque Set newtype paralleling Object, living under src/value/set/ with the same module structure as src/value/object/ (mod.rs / iter.rs / serde.rs).

Set wraps BTreeSet<Value> today but exposes only a curated surface: contains, insert, remove, iter / iter_sorted, cursor (resumable), is_subset, intersection, difference, serde, and a hand-written Ord. The cursor type is re-exported behind the rvm feature so the follow-up IterationState::Set swap can land additively.

To free the Set name for the new public type, the crate-internal BTreeSet as Set / HashSet as Set alias in lib.rs is renamed to MapSet. Only one in-tree consumer existed (compiled_policy::CompiledPolicy::rule_paths) and is updated in lockstep.

Value::Set is unchanged in this PR (still Rc<BTreeSet<Value>>); the payload swap and call-site migration ship in the next PR.

Tests live in src/value/tests.rs alongside the Object tests.

@anakrish

anakrish commented Jun 2, 2026

Copy link
Copy Markdown
Owner Author

@copilot Review this PR using the review skills in the repo. Use a separate agent for each skill.

Copilot AI commented Jun 2, 2026

Copy link
Copy Markdown

@copilot Review this PR using the review skills in the repo. Use a separate agent for each skill.

Completed both requested reviews using separate agents per skill:

  • code-review skill run via dedicated review flow.
  • deep-review skill run with 3 discovery agents + micro-passes + adversarial verifier.

Key surviving findings:

  1. High: unchecked accumulation in Set::intersection/union/difference (src/value/set/mod.rs:131-150).
  2. High: unchecked accumulation in Set::append, Extend, FromIterator (src/value/set/mod.rs:125,241-252).
  3. Medium (likely): public Set abstraction still coupled to BTreeSet through Value::Set and related APIs.

Reports were written to:

  • /tmp/code-review-report.md
  • /tmp/deep-review-report.md

@anakrish

anakrish commented Jun 2, 2026

Copy link
Copy Markdown
Owner Author

Thanks. Quick triage:

Findings 1 & 2 (unchecked accumulation in intersection/union/difference, append, Extend, FromIterator) — not a regression introduced by this PR. The exact same shapes already exist on Object (src/value/object/mod.rs:142, 240, 246) — Object::append, impl Extend<(Value, Value)> for Object, impl FromIterator<…> for Object — none of which call enforce_limit. By design, Set here mirrors Object so the surfaces evolve together.

A note on the intersection/union/difference bound: the output cardinality is strictly ≤ self.len() + other.len(), and both inputs are already bounded by upstream enforce_limit calls at the points they were constructed. So there's no unbounded growth path on these — they're bounded by data that already passed the limit check.

If we want fallible variants returning Result, that's a cross-cutting change that has to touch Object and Set in lockstep (and Extend/FromIterator traits don't return Result, so those would need new fallible methods). Better as a follow-up that addresses both abstractions, not as a one-sided fix on Set.

Finding 3 (Value::Set still coupled to BTreeSet) — yes, intentional and called out in the PR description: this PR only introduces the Set newtype; the Value::Set payload swap and call-site migration ship in the next PR (parallel to how PR #57 / #58 split the Object work).

No changes on this PR. Happy to file a tracking issue for "fallible accumulation across Set + Object" if you want, but I'd rather decouple that from the abstraction landing.

@anakrish
anakrish force-pushed the storage-abstraction-set-foundation branch 2 times, most recently from 70882a8 to 2dd9c43 Compare June 4, 2026 17:44
@anakrish

anakrish commented Jun 4, 2026

Copy link
Copy Markdown
Owner Author

@copilot Review this PR using the review skills in the repo. Use a separate agent for each skill.

Copilot AI commented Jun 4, 2026

Copy link
Copy Markdown

@copilot Review this PR using the review skills in the repo. Use a separate agent for each skill.

Completed both requested reviews using separate agents (code-review and deep-review).

Results: no new actionable regressions specific to this PR after adversarial verification. The main flagged items were either already triaged as intentional staging / non-regression or mirrored existing Object behavior.

Full reports were written to:

  • /tmp/code-review-report.md
  • /tmp/deep-review-report.md

@anakrish
anakrish force-pushed the storage-abstraction-set-foundation branch from 2dd9c43 to 39fff42 Compare June 4, 2026 19:37
@anakrish anakrish mentioned this pull request Jun 4, 2026
Open
kusha and others added 3 commits June 5, 2026 16:00
@kusha Mark Birger
tests/opa: normalize path separators in folder filter on Windows (mic…
bd90453 
…rosoft#742)

`run_opa_tests` builds `path_dir_str` from `path.strip_prefix(...).to_string_lossy()`,
which on Windows yields strings with backslash separators (e.g.
`v0\aggregates`). The folder filter then does an exact-string
comparison against the CLI arguments:

    let run_test = folders.is_empty()
        || folders.iter().any(|f| &path_dir_str == f);

CLI arguments use forward slashes (`v0/aggregates`), so on Windows
the comparison never matches, no tests are selected, and the function
bails with `"no matching tests found"`. This blocks the
`cargo xtask pre-push` hook for any Windows contributor.

Normalize `path_dir_str` to use forward slashes at construction
time. Reproduces before the fix as `cargo test ... --test opa --
v1/aggregates` exiting 1 with `no matching tests found`; after the
fix the same command runs 72 cases and the full hook command runs
2861 / 0 across 188 folders.

The duplicate platform check at the `is_rego_v0_test` site
(`path_dir_str.starts_with("v0/") || path_dir.starts_with("v0\\")`)
is left intact to keep the change minimal — the backslash branch
becomes redundant but is harmless.

Co-authored-by: Mark Birger <markbirger@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@anakrish
refactor(value): migrate Value::Object to Object storage abstraction (m…
ed6ae46 
…icrosoft#736)

Builds on #57. Swap Value::Object's payload from Rc<BTreeMap<Value, Value>>
to Rc<Object> and migrate all call sites to the Object API.

as_object / as_object_mut keep their names but return &Object / &mut Object.
The mutable accessor handles Rc::make_mut internally, so callers no longer
do it themselves. Object grows into_value() and From<Object> for Value.
Value's serializer now delegates to Object::serialize, dropping a duplicate
non-string-key stringification path.

RVM IterationState::Object is rewritten around ObjectCursor: O(log n)
steps over a shared Rc<Object>, no eager pair snapshot. Snapshot
independence is preserved by Rc copy-on-write; setup_next_iteration
advances the cursor inline and advance() becomes a no-op for this variant.
A new iteration_state_object_is_snapshot_independent_of_source test
covers CoW against a mutated alias.

Value::Set still wraps Rc<BTreeSet<Value>>; the matching Set abstraction
and its swap ship in follow-up PRs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@anakrish
feat(value): introduce Set storage abstraction
923da90 
Add an opaque `Set` newtype paralleling `Object`, living under
`src/value/set/` with the same module structure (`mod.rs` /
`iter.rs` / `serde.rs`). `Set` wraps `BTreeSet<Value>` today but
exposes only a curated surface: `contains`, `insert`, `remove`,
`iter`, `iter_sorted`, `cursor` (resumable), `is_subset`,
`intersection`, `difference`, serde, and a hand-written `Ord`.
The cursor types are re-exported behind the `rvm` feature so the
follow-up `IterationState::Set` swap can land additively.

To free the `Set` name for the new public type, the crate-internal
`BTreeSet as Set` / `HashSet as Set` aliases in `lib.rs` are
renamed to `MapSet`. All in-tree consumers of the old alias are
updated in lockstep.

`Value::Set` is unchanged in this commit (still wraps
`Rc<BTreeSet<Value>>`); the payload swap and call-site migration
ship in the next PR.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@anakrish
anakrish force-pushed the storage-abstraction-set-foundation branch from 39fff42 to 923da90 Compare June 6, 2026 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@anakrish @kusha