-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Handle single sharded keyspaces for analysis #16068
Handle single sharded keyspaces for analysis #16068
Conversation
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
|
@@ -1050,7 +1060,7 @@ func TestSchemaDiff(t *testing.T) { | |||
return | |||
} | |||
if tc.conflictingDiffs > 0 { | |||
assert.Error(t, err) | |||
require.Error(t, err) |
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.
Changed this, since without this we'd segfault on checks after this if we have no error at all.
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.
LGTM
This fixes the problem where we would throw an error for a query on a single unsharded keyspace, even though the limitation only exists for sharded queries. The issue here was that the previous `singleUnshardedKeyspace` field was really the field indicating if we could short circuit analysis. `schemadiff` doesn't want to short cut full analysis though, since it does want to know table & column dependencies etc. which would not be computed if analysis can be shortcutted. Since the `singleUnshardedKeyspace` field was overloaded and really was "can we shortcut" (either because of being in a single sharded keyspace or because of non-strict analysis being requested), we couldn't use that field as a fix to guard the error thrown. Instead, we now have a proper explicit field `canShortcut` that we use to handle existing shortcut logic. `singleUnshardedKeyspace` still exists, but now means what it actually implies, which is if this query runs against a single unsharded keyspace, irrespective of whether we can shortcut or not and is correctly set when doing full analysis. With those changes, we can use `singleUnshardedKeyspace` to guard the error throw for unsupported sharded queries and `schemadiff` works properly as well. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
3926bec
to
fc286e9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16068 +/- ##
=======================================
Coverage 68.21% 68.21%
=======================================
Files 1541 1541
Lines 197330 197332 +2
=======================================
+ Hits 134599 134604 +5
+ Misses 62731 62728 -3 ☔ View full report in Codecov by Sentry. |
This fixes the problem where we would throw an error for a query on a single unsharded keyspace, even though the limitation only exists for sharded queries.
The issue here was that the previous
singleUnshardedKeyspace
field was really the field indicating if we could short circuit analysis.schemadiff
doesn't want to short cut full analysis though, since it does want to know table & column dependencies etc. which would not be computed if analysis can be shortcutted.Since the
singleUnshardedKeyspace
field was overloaded and really was "can we shortcut" (either because of being in a single sharded keyspace or because of non-strict analysis being requested), we couldn't use that field as a fix to guard the error thrown.Instead, we now have a proper explicit field
canShortcut
that we use to handle existing shortcut logic.singleUnshardedKeyspace
still exists, but now means what it actually implies, which is if this query runs against a single unsharded keyspace, irrespective of whether we can shortcut or not and is correctly set when doing full analysis.With those changes, we can use
singleUnshardedKeyspace
to guard the error throw for unsupported sharded queries andschemadiff
works properly as well.Related Issue(s)
Fixes #16067
Checklist