Improve inline verifier mismatch errors #361
Conversation
driv3r
force-pushed
the
improve-inline-verifier-mismatch-errors
branch
from
154e234 to
2dd8c90
Compare
inline_verifier.go
Outdated
| MismatchRowMissingOnSource mismatchType = "row missing on source" | ||
| MismatchRowMissingOnTarget mismatchType = "row missing on target" | ||
| MismatchContentDifference mismatchType = "content difference" | ||
| MismatchChecksumDifference mismatchType = "rows checksum difference" |
There was a problem hiding this comment.
added checksum difference next to content one, to differentiate between whole row checksum comparison and column data one
There was a problem hiding this comment.
isn't content & checksum difference the same thing? especially now that we are reporting missing row & column separately. If all the columns are there, and content is different checksum will be different?
There was a problem hiding this comment.
there's a difference, because we check both checksum of a whole row + compare each rows columns separately, so the checksum difference is for when the SQL query generating checksum for whole row doesn't match one in source, while the content difference is when we compare each single column of a row and encounter and issue there
There was a problem hiding this comment.
🤔 If i understand this right, then maybe we should rename these to MismatchFieldDifference and MismatchRowChecksumDifference?
| type InlineVerifierMismatches struct { | ||
| Pk uint64 | ||
| SourceChecksum string | ||
| TargetChecksum string | ||
| MismatchColumn string | ||
| MismatchType mismatchType |
There was a problem hiding this comment.
flatten the structure a bit, as we always want to have the type in
| if mismatch.TargetChecksum != "" { | ||
| messageBuf.WriteString(", target: ") | ||
| messageBuf.WriteString(mismatch.TargetChecksum) | ||
| } |
There was a problem hiding this comment.
source and target checksum are optional, as we don't include them in case rows are missing (source or target)
inline_verifier.go
Outdated
| mismatchSet[paginationKey] = InlineVerifierMismatches{ | ||
| Pk: paginationKey, | ||
| MismatchType: MismatchChecksumDifference, |
There was a problem hiding this comment.
- I couldn't find any reason why
!existwas after the bytes comparison, it's counter intuitive as we should first check if rows are there - That's why I've split up check for
!existsfrom!bytes.Equal(sourceHash, targetHash)so we could properly define the mismatch type
| if !bytes.Equal(sourceHash, targetHash) || !exists { | ||
| for paginationKey, _ := range source { | ||
| _, exists := target[paginationKey] | ||
| if !exists { |
There was a problem hiding this comment.
if rows exist in both source & target, then they are already compared above, no need to check it again
| MismatchColumn: colName, | ||
| } | ||
| break // no need to compare other columns | ||
| } else if !bytes.Equal(sourceData, targetData) { |
There was a problem hiding this comment.
- again, split
!existscheck from bytes comparison - for bytes comparison, generate checksums for columns data (avoid storing data which might be PII)
| break | ||
| for colName := range sourceDecompressedColumns { | ||
| _, exists := targetDecompressedColumns[colName] | ||
| if !exists { |
There was a problem hiding this comment.
again, there's no point of comparing the data for the second time, it should be the same as in the loop above, check for column existence only
| } | ||
| compressedMismatch := compareDecompressedData(sourceData, targetData) | ||
| for paginationKey, mismatch := range compressedMismatch { | ||
| mismatches[paginationKey] = mismatch |
There was a problem hiding this comment.
with this, even if there's a hash comparison already there, we will get more details on the column involved
| for tableName, _ := range mismatches[schemaName] { | ||
| sortedTables = append(sortedTables, tableName) | ||
| } | ||
| sort.Strings(sortedTables) |
There was a problem hiding this comment.
sort tables for predictability
driv3r
force-pushed
the
improve-inline-verifier-mismatch-errors
branch
from
0593a70 to
53c5c66
Compare
inline_verifier_test.go
Outdated
There was a problem hiding this comment.
Proper unit tests! 🎉
No description provided.