-
Notifications
You must be signed in to change notification settings - Fork 2.3k
VDiff: Do not intentionally timeout query, and display diff sample binary columns as hex #19044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Matt Lord <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
7aefe71 to
8d1d085
Compare
Signed-off-by: Matt Lord <[email protected]>
8d1d085 to
a95b1de
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19044 +/- ##
========================================
Coverage 69.88% 69.88%
========================================
Files 1610 1610
Lines 215431 215679 +248
========================================
+ Hits 150551 150732 +181
- Misses 64880 64947 +67 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Matt Lord <[email protected]>
|
📝 Documentation updates detected! New suggestion: Add changelog entry for VDiff improvements |
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
nickvanw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me other than a test that I think needs updating. Other questions are around changes that look unrelated.
| return false | ||
| } | ||
| return route.RoutingParameters.Opcode.IsSingleShard() | ||
| return route.Opcode.IsSingleShard() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended to be part of the change - if so, I'm not sure how it's part of VDiff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the linter complaining about it after I merged in origin/main.
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
42a1c66 to
2f7e3d9
Compare
Description
This PR makes 2 meaningful quality of life improvements for VDiff:
You can see that the query hint is no longer added using the local examples:
The log messages that still DO have the
MAX_EXECUTION_TIMEquery hint are the ones for the copy phase of the workflow that we are diffing (as they should).Versus on
main:Related Issue(s)
Checklist