VDiff: Use byte compare if weight_string() returns null for either source or target #7696
Conversation
go/vt/wrangler/vdiff.go
Outdated
| // This detects if we are using weight_string() to compare this (text) column. | ||
| // If either source or target weight_string is null we fallback to a byte compare | ||
| if sourceRow[col].IsNull() && sourceRow[i].IsText() && col > i { | ||
| col = i | ||
| } | ||
| if targetRow[col].IsNull() && targetRow[i].IsText() && col > i { | ||
| col = i | ||
| } |
There was a problem hiding this comment.
I confess I'm completely lost in this logic. What is the logical connection between col (column) and i (array index)? What does it mean where col > i and what does it mean to assign col = i?
There was a problem hiding this comment.
VDiff adds additional pseudo-columns to the rows which contain the weight_string() for text fields and points to that column for comparison purposes. When those are null (for reasons mentioned in #7296) the PR falls back to the original column)
There was a problem hiding this comment.
Thank you! I still don't understand the connection between col and i. They seem to be in different spaces. i is an array index. col is, ... col. Why would you test col > i or assign col = i`?
There was a problem hiding this comment.
col is initialized to be the index i and then overwritten for text fields by pointing to the weight_string(): https://github.com/vitessio/vitess/blob/98e4e0e9bfae1e1483e2f7fed952df455fe528af/go/vt/wrangler/vdiff.go#L436
There was a problem hiding this comment.
I see that compare is being called with td.comparePKs in one case and td.cmpareCols in the other. I feel like this logic may not work correctly in the case of comparePKs.
Also, I think it will become more readable if we changed cols to be a struct that contains a compareCol and an originalCol. If compareCol fails, then we can fall back to originalCol.
There was a problem hiding this comment.
I see that
compareis being called withtd.comparePKsin one case andtd.cmpareColsin the other. I feel like this logic may not work correctly in the case of comparePKs.Also, I think it will become more readable if we changed
colsto be a struct that contains acompareColand anoriginalCol. IfcompareColfails, then we can fall back tooriginalCol.
Refactored based on your suggestion. @sougou, please review both this change and the updated tests.
There was a problem hiding this comment.
Thank you! I still don't understand the connection between
colandi. They seem to be in different spaces.iis an array index.colis, ... col. Why would you testcol > ior assign col = i`?
@shlomi-noach, when you get a chance please review the changes and see if it easier to understand now
rohit-nayak-ps
force-pushed
the
rn-vdiff-null-weight-string
branch
from
98e4e0e to
a506a80
Compare
… target Signed-off-by: Rohit Nayak <[email protected]>
…f tests needed changes. Signed-off-by: Rohit Nayak <[email protected]>
rohit-nayak-ps
force-pushed
the
rn-vdiff-null-weight-string
branch
from
a506a80 to
921c225
Compare
Description
VDiff compares text columns using the weight_string() of the value. However there are times when weight_string() can return a null value. Per the mysql reference
It is possible that just one of the weight_string(), for source or target is null or both are null, depending on the configuration of the source and target servers. Currently if one is null vdiff detects a mismatch and if both are null then it is considered a match. This is incorrect since, in such cases, vdiff needs to compare the actual data.
This PR detects such a condition and uses a byte compare instead.
See related issue for more details and repro.
Signed-off-by: Rohit Nayak [email protected]
Related Issue(s)
Checklist
Impacted Areas in Vitess
Components that this PR will affect: