Uh oh!
There was an error while loading. Please reload this page.
Add libs/structdiff - #2928
Conversation
shreyas-goenka
left a comment
There was a problem hiding this comment.
Thanks, looks good to me, the comments are minor.
| v1 := reflect.ValueOf(a) | ||
| v2 := reflect.ValueOf(b) | ||
| if !v1.IsValid() && !v2.IsValid() { |
There was a problem hiding this comment.
Just curious, Do we encounter invalid values in practice? Or are we just being defensive here?
There was a problem hiding this comment.
Yes, if both a and b are nils.
| // If Index >= 0, the node specifies a slice/array index in Index. | ||
| // If Index == -1, the node specifies a struct attribute in Key | ||
| // If Index == -2, the node specifies a map key in Key | ||
| Index int |
There was a problem hiding this comment.
(not blocking) This can be confusing in isolation. Maybe a separate enum type? For array vs struct vs map?
There was a problem hiding this comment.
Separate enum type would increase size for no good reason.
We could add methods that hide this details -- that would be good if this was an exported struct. But currently it's a local helper.
| // diffValues appends changes between v1 and v2 to the slice. path is the current | ||
| // JSON-style path (dot + brackets). At the root path is "". | ||
| func diffValues(path *pathNode, v1, v2 reflect.Value, changes *[]Change) { |
There was a problem hiding this comment.
(not blocking) Have you also consider having this function return changes? Slightly easier to reason about. Same for other functions.
| funcdiffValues(path*pathNode, v1, v2 reflect.Value, changes*[]Change) { | |
| funcdiffValues(path*pathNode, v1, v2 reflect.Value) []Change { |
The callsite then can be changes = append(changes, diffValues(...))
There was a problem hiding this comment.
Not sure what's the benefit? It's more verbose on the caller side.
There was a problem hiding this comment.
The benefits are, it mirrors how append works in Go for slices. Pointer to []change allows for arbitrary changes to the slice while it seems we only need append semantics here. Having slice as a return type would encode the append semantics in the type system.
Not blocking from my POV, but seems marginally better.
| deepEqualValues(path, v1, v2, changes) | ||
| } | ||
| default: | ||
| deepEqualValues(path, v1, v2, changes) |
There was a problem hiding this comment.
A struct with a function callback as a field will always never be equal to itself due to deepequal semantics. Can that be a problem? If so we might want to ignore functional fields in diffStruct.
There was a problem hiding this comment.
I guess you're right. I don't have a use case involving structs with callback fields though.
| for i := range t.NumField() { | ||
| sf := t.Field(i) | ||
| if !sf.IsExported() || sf.Name == "ForceSendFields" { |
There was a problem hiding this comment.
You can specify json:"-" to have the marshaller ignore those fields when serializing. Should we also ignore those for diff?
There was a problem hiding this comment.
Not sure, let's see a few specific use cases first. This can also be done by the caller btw.
It's possible that we'd have to change this function to look at json tags more (for example, report json names instead of golang names). In that case, we'd probably want to ignore these.
+ exec go test -bench=. -benchmem -run '^x' goos: darwin goarch: arm64 pkg: github.com/databricks/cli/libs/structdiff cpu: Apple M3 Max BenchmarkEqual-16 31399 37034 ns/op 13368 B/op 540 allocs/op --- BENCH: BenchmarkEqual-16 bench_test.go:30: Total: 0 / 1 bench_test.go:30: Total: 0 / 100 bench_test.go:30: Total: 0 / 10000 bench_test.go:30: Total: 0 / 31399 BenchmarkChanges-16 30908 38932 ns/op 16008 B/op 598 allocs/op --- BENCH: BenchmarkChanges-16 bench_test.go:30: Total: 10 / 1 bench_test.go:30: Total: 1000 / 100 bench_test.go:30: Total: 100000 / 10000 bench_test.go:30: Total: 309080 / 30908 BenchmarkZero-16 26421 45517 ns/op 26012 B/op 843 allocs/op --- BENCH: BenchmarkZero-16 bench_test.go:30: Total: 75 / 1 bench_test.go:30: Total: 7500 / 100 bench_test.go:30: Total: 750000 / 10000 bench_test.go:30: Total: 1981575 / 26421 BenchmarkNils-16 36157 33067 ns/op 20542 B/op 572 allocs/op --- BENCH: BenchmarkNils-16 bench_test.go:30: Total: 77 / 1 bench_test.go:30: Total: 7700 / 100 bench_test.go:30: Total: 770000 / 10000 bench_test.go:30: Total: 2784089 / 36157 PASS ok github.com/databricks/cli/libs/structdiff 7.112s
denik
commented
May 27, 2025
It works on types without it as well, but respects if it's present and of the right type. |
+ exec go test -bench=. -benchmem -run '^x' goos: darwin goarch: arm64 pkg: github.com/databricks/cli/libs/structdiff cpu: Apple M3 Max BenchmarkEqual-16 27452 42422 ns/op 24632 B/op 628 allocs/op --- BENCH: BenchmarkEqual-16 bench_test.go:30: Total: 0 / 1 bench_test.go:30: Total: 0 / 100 bench_test.go:30: Total: 0 / 10000 bench_test.go:30: Total: 0 / 27452 BenchmarkChanges-16 25983 45990 ns/op 27272 B/op 686 allocs/op --- BENCH: BenchmarkChanges-16 bench_test.go:30: Total: 10 / 1 bench_test.go:30: Total: 1000 / 100 bench_test.go:30: Total: 100000 / 10000 bench_test.go:30: Total: 259830 / 25983 BenchmarkZero-16 23082 54397 ns/op 35751 B/op 919 allocs/op --- BENCH: BenchmarkZero-16 bench_test.go:30: Total: 75 / 1 bench_test.go:30: Total: 7500 / 100 bench_test.go:30: Total: 750000 / 10000 bench_test.go:30: Total: 1731150 / 23082 BenchmarkNils-16 32385 36278 ns/op 24194 B/op 608 allocs/op --- BENCH: BenchmarkNils-16 bench_test.go:30: Total: 77 / 1 bench_test.go:30: Total: 7700 / 100 bench_test.go:30: Total: 770000 / 10000 bench_test.go:30: Total: 2493645 / 32385 PASS ok github.com/databricks/cli/libs/structdiff 6.853s
…arks worse" This reverts commit 6dd5285.
## Changes Switch structdiff to (lazily) compute jsontag-based path instead of Golang field names. Follow up to #2928 ## Why This is closer to what user is going to want to see in "bundle diff". This is also useful if we want to look up the path in dyn.Value world. ## Benchmark This PR: ``` ~/work/cli-main/libs/structdiff % go test -bench=. -benchmem -run ^x -benchtime=10s goos: darwin goarch: arm64 pkg: github.com/databricks/cli/libs/structdiff cpu: Apple M3 Max BenchmarkEqual-16 315444 36290 ns/op 18040 B/op 540 allocs/op BenchmarkChanges-16 307059 38747 ns/op 20704 B/op 598 allocs/op BenchmarkZero-16 260900 45907 ns/op 30456 B/op 843 allocs/op BenchmarkNils-16 365086 33155 ns/op 24034 B/op 572 allocs/op ``` main: ``` ~/work/cli-main/libs/structdiff % go test -bench=. -benchmem -run ^x -benchtime=10s goos: darwin goarch: arm64 pkg: github.com/databricks/cli/libs/structdiff cpu: Apple M3 Max BenchmarkEqual-16 311192 36944 ns/op 13368 B/op 540 allocs/op BenchmarkChanges-16 310232 39032 ns/op 16008 B/op 598 allocs/op BenchmarkZero-16 264555 45187 ns/op 26011 B/op 843 allocs/op BenchmarkNils-16 363981 32819 ns/op 20542 B/op 572 allocs/op ```
## Changes New library to compare two Go structs and give field-by-field difference. Respects ForceSendFields. ## Why Using it trigger deployments in #2926 Will also be using it for "bundle diff" command, however, it would need to be extended to calculate proper minimal diff on slices. ## Tests New unit tests. ## Benchmark ``` + exec go test -bench=. -benchmem -run '^x' goos: darwin goarch: arm64 pkg: github.com/databricks/cli/libs/structdiff cpu: Apple M3 Max BenchmarkEqual-16 32433 35876 ns/op 13368 B/op 540 allocs/op BenchmarkChanges-16 31153 38006 ns/op 16008 B/op 598 allocs/op BenchmarkZero-16 27346 44260 ns/op 26011 B/op 843 allocs/op BenchmarkNils-16 37209 32295 ns/op 20541 B/op 572 allocs/op ```
## Changes Switch structdiff to (lazily) compute jsontag-based path instead of Golang field names. Follow up to #2928 ## Why This is closer to what user is going to want to see in "bundle diff". This is also useful if we want to look up the path in dyn.Value world. ## Benchmark This PR: ``` ~/work/cli-main/libs/structdiff % go test -bench=. -benchmem -run ^x -benchtime=10s goos: darwin goarch: arm64 pkg: github.com/databricks/cli/libs/structdiff cpu: Apple M3 Max BenchmarkEqual-16 315444 36290 ns/op 18040 B/op 540 allocs/op BenchmarkChanges-16 307059 38747 ns/op 20704 B/op 598 allocs/op BenchmarkZero-16 260900 45907 ns/op 30456 B/op 843 allocs/op BenchmarkNils-16 365086 33155 ns/op 24034 B/op 572 allocs/op ``` main: ``` ~/work/cli-main/libs/structdiff % go test -bench=. -benchmem -run ^x -benchtime=10s goos: darwin goarch: arm64 pkg: github.com/databricks/cli/libs/structdiff cpu: Apple M3 Max BenchmarkEqual-16 311192 36944 ns/op 13368 B/op 540 allocs/op BenchmarkChanges-16 310232 39032 ns/op 16008 B/op 598 allocs/op BenchmarkZero-16 264555 45187 ns/op 26011 B/op 843 allocs/op BenchmarkNils-16 363981 32819 ns/op 20542 B/op 572 allocs/op ```
Changes
New library to compare two Go structs and give field-by-field difference. Respects ForceSendFields.
Why
Using it trigger deployments in #2926
Will also be using it for "bundle diff" command, however, it would need to be extended to calculate proper minimal diff on slices.
Tests
New unit tests.
Benchmark