compare anyOf based on handmade diff score #32

6293 · 2023-05-16T18:07:43Z

Closes #26

untitaker · 2023-05-16T19:19:10Z

Cargo.toml

@@ -25,6 +25,7 @@ schemars = { version = "0.8.12", default_features = false }
 serde = "1.0.158"
 serde_json = "1.0.94"
 thiserror = "1.0.40"
+hungarian = "1.1.1"


istm hungarian is dead: https://github.com/nwtnni/hungarian

untitaker · 2023-05-16T19:37:48Z

src/diff_walker.rs

+            .diff_score(&mut rhs.clone().into_object())
+    }
+}
+impl DiffScore for SchemaObject {


actually! why not invoke the main diff function as a way to compute the score?

6293 · 2023-05-17T08:39:13Z

src/diff_walker.rs

+            for (i, l) in lhs_any_of.iter_mut().enumerate() {
+                for (j, r) in rhs_any_of.iter_mut().enumerate() {
+                    let mut walker = DiffWalker {
+                        changes: vec![],
+                        lhs_root: self.lhs_root.clone(),
+                        rhs_root: self.rhs_root.clone(),
+                    };
+                    walker.diff(
+                        "",
+                        &mut l.clone().into_object(),
+                        &mut r.clone().into_object(),
+                    )?;
+                    mat[(i, j)] = i32::try_from(walker.changes.len()).expect("too many changes");
+                }
+            }


It would be more convenient for Diffwalker::diff to return Vec<Change> instead of mutating internal changes. By doing so we can reuse Diffwalker and avoid cloning RootSchema here

there is generally way too much cloning going on. I haven't wrapped my head around it yet, but I already have a problem with the fact that we cannot just pass an immutable reference when we recurse into schemas

I would not change it for now, I want to figure out how we can eliminate as many clones as possible. changing DiffWalker now might make it harder to remove clone() in a hotter codepath.

@6293 actually, I think we should change the design of diff walker in the following way:

instead of a vector of changes, it is generic over the collection that holds the changes. this can be implemented by making DiffWalker take a callback for appending a change

at this point I think DiffWalker should have a proper new constructor as well, because we instantiate it in two places

in the public API, we can pass myvector.push as callback. but here, we can increment an integer and discard the change. that's a lot less memory used

if there are too many changes, we clip them at i32::MAX

untitaker · 2023-05-17T13:31:16Z

src/diff_walker.rs

-            rhs_any_of.sort_by_cached_key(|x| format!("{x:?}"));
+            match (lhs_any_of.len(), rhs_any_of.len()) {
+                (l, r) if l <= r => {
+                    lhs_any_of.append(&mut vec![Schema::Bool(false); r - l]);


it seems you're trying to get the vectors to be the same length by padding the shorter one with false.

if that is the case I think you can do this:

let max_len = cmp::max(lhs_any_of.len(), rhs_any_of.len()); lhs_any_of.resize(max_len, Schema::Bool(false)); rhs_any_of.resize(max_len, Schema::Bool(false));

untitaker · 2023-05-17T13:35:59Z

src/diff_walker.rs

+            for (i, l) in lhs_any_of.iter_mut().enumerate() {
+                for (j, r) in rhs_any_of.iter_mut().enumerate() {
+                    let mut walker = DiffWalker {
+                        changes: vec![],
+                        lhs_root: self.lhs_root.clone(),
+                        rhs_root: self.rhs_root.clone(),
+                    };
+                    walker.diff(
+                        "",
+                        &mut l.clone().into_object(),
+                        &mut r.clone().into_object(),
+                    )?;
+                    mat[(i, j)] = i32::try_from(walker.changes.len()).expect("too many changes");
+                }
+            }


@6293 actually, I think we should change the design of diff walker in the following way:

instead of a vector of changes, it is generic over the collection that holds the changes. this can be implemented by making DiffWalker take a callback for appending a change

at this point I think DiffWalker should have a proper new constructor as well, because we instantiate it in two places

in the public API, we can pass myvector.push as callback. but here, we can increment an integer and discard the change. that's a lot less memory used

if there are too many changes, we clip them at i32::MAX

untitaker · 2023-05-17T15:24:31Z

src/diff_walker.rs

-impl DiffWalker {
+impl<'cb> DiffWalker<'cb> {
+    pub fn new(
+        cb: Box<dyn FnMut(Change) + 'cb>,


can we instead do:

struct DiffWalker<F: FnMut(Change)> { pub cb: F }

I don't think we need a nameable DiffWalker type

the benefit of this is that the compiler can inline cb into the parent function, then it enables further optimization

we would get an reached the recursion limit while instantiating ... error if we try to specify the type of callback as a generic parameter. This is because generic instantiation occurs infinitely in diff_any_of

Can we just have private DiffWalker::count method for saving memory? Although it is not as efficient as "callback" approach, the memory for Vec<Change> would be freed by the time DiffWalker::count is dropped and I think that is fair enough

I didn't understand this comment at first because we are not instantiatign DiffWalker recursively in DiffWalker::new. But you are right, there is a problem with generics. The reason is apparently that the closure contains the parent type in its signature, so the expansion of the type name recurses infinitely.

I have never seen that compiler error before.

but we can still get rid of mandatory boxing, so I pushed a change that does that. Instead we can box only for the counter, and allow for inlining of the other callback

the super-optimized solution would define a new Counter struct that holds an integer and implements FnMut (or rather, a custom trait), but that is too annoying, even for me

we can still get rid of mandatory boxing

that was beyond my imagination. thanks

Can we just have private DiffWalker::count method for saving memory?

the problem is having a vector at all. if we have any vector and keep pushing stuff into it, it will use memory

the vector in its entirety is short-lived, so it is not like json-schema-diff will consume a lot of memory overall. but it will be slow because allocating, resizing, deallocating is slow.

relatively slow, overall it will still be super fast. arguably this is premature optimization, but we have a O(n^2) loop here already to build the cross-product of all anyOf items between LHS and RHS, so I guess for some pathological input it could be a problem

6293 · 2023-05-17T15:26:51Z

Had to box the callback because we recursively instantiate Diffwalker

untitaker

awesome

6293 added 2 commits May 17, 2023 01:35

fill subschemas on length mismatch

6c59eec

compare anyOf based on handmade diff score

47ed7cd

6293 force-pushed the compare-any-of branch from 958224e to 47ed7cd Compare May 16, 2023 18:08

add test on multiple number schemas within anyOf

e3d2f8e

untitaker reviewed May 16, 2023

View reviewed changes

use diff to compute the score

b0ff30c

6293 marked this pull request as ready for review May 17, 2023 04:33

6293 requested a review from a team as a code owner May 17, 2023 04:33

6293 requested a review from untitaker May 17, 2023 04:47

hungarian is dead; use pathfinding instead

deb50d9

6293 force-pushed the compare-any-of branch from 3695de8 to deb50d9 Compare May 17, 2023 08:35

6293 commented May 17, 2023

View reviewed changes

untitaker requested changes May 17, 2023

View reviewed changes

6293 added 2 commits May 17, 2023 22:47

make use of Vec::resize

0ebbfe3

make Diffwalker take a callback

7b4cc56

untitaker reviewed May 17, 2023

View reviewed changes

untitaker added 2 commits May 17, 2023 18:27

make generic

35fa7c1

fmt

6b315ac

untitaker approved these changes May 17, 2023

View reviewed changes

untitaker merged commit 0276ef2 into getsentry:main May 17, 2023
2 checks passed

6293 mentioned this pull request May 19, 2023

do not clone Schema, clone only when the inner struct is Bool #34

Merged

untitaker mentioned this pull request May 19, 2023

false-negative report on anyOf if two arrays have different length #30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compare anyOf based on handmade diff score #32

compare anyOf based on handmade diff score #32

6293 commented May 16, 2023

untitaker May 16, 2023

untitaker May 16, 2023

6293 May 17, 2023

6293 May 17, 2023

untitaker May 17, 2023

untitaker May 17, 2023

untitaker May 17, 2023 •

edited

Loading

untitaker May 17, 2023

untitaker May 17, 2023 •

edited

Loading

untitaker May 17, 2023

6293 May 17, 2023

6293 May 17, 2023 •

edited

Loading

untitaker May 17, 2023 •

edited

Loading

6293 May 17, 2023

untitaker May 17, 2023

6293 commented May 17, 2023

untitaker left a comment

compare anyOf based on handmade diff score #32

compare anyOf based on handmade diff score #32

Conversation

6293 commented May 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

6293 May 17, 2023 • edited Loading

Choose a reason for hiding this comment

untitaker May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

6293 commented May 17, 2023

untitaker left a comment

Choose a reason for hiding this comment

untitaker May 17, 2023 •

edited

Loading

untitaker May 17, 2023 •

edited

Loading

6293 May 17, 2023 •

edited

Loading

untitaker May 17, 2023 •

edited

Loading