cdctest: fix fingerprint validator primary key cols check #139958
Conversation
|
This change is |
|
The first commit comes from #139914. |
| After map[string]interface{} `json:"after"` | ||
| } | ||
| var value wrapper | ||
| if err := gojson.Unmarshal([]byte(row.value), &value); err != nil { |
There was a problem hiding this comment.
Trying to understand what's going on here.
Is this what's happening? Previously we had an issue with the declarative schema changer where JSON encoded data was failing these assertions. Then we introduced using gojson to fix that problem. This did not work so now we a) avoid using that schema changer in tests until we fix that more permanently and b) remove the previous fix that didn't work.
Ok, so using this style of JSON decoding fixes the issue instead of using gojson. We don't know why we were using gojson previously but it doesn't seem like it was for any particular reason. Sounds good
There was a problem hiding this comment.
i believe this section is to fix an error where gojson truncated large int64s in the key during decoding
Previously, fingerprint validator could fail with sql smith due to precision loss from JSON encoding & decoding of large numbers. We previously fixed the validator regarding its values check. This patch fixes the primary key columns check accordingly. Part of: cockroachdb#124146 Release note: none
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
TFTR! bors r=asg0451 |
Previously, fingerprint validator could fail with sql smith due to precision
loss from JSON encoding & decoding of large numbers. We previously fixed the
validator regarding its values check. This patch fixes the primary key columns
check accordingly.
Part of: #124146
Release note: none