-
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
Fix querysharding labels analysis #5880
Conversation
a510f1e
to
9febb18
Compare
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.
Thanks, just one small nit.
da319c0
to
5ed0166
Compare
Signed-off-by: Ben Ye <[email protected]>
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.
Thanks, lgtm.
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
5ed0166
to
9c1b74d
Compare
Sorry I just fixed the merge conflict. @fpetkovski Could you approve again? Thanks! |
I will ignore the lint error and merge it if other tests pass. The lint error was introduced by another commit we just merged so let's fix it in another pr. |
* fix querysharding labels analysis Signed-off-by: Ben Ye <[email protected]> * update changelog Signed-off-by: Ben Ye <[email protected]> * fix lint Signed-off-by: Ben Ye <[email protected]> * update test case Signed-off-by: Ben Ye <[email protected]> Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye [email protected]
Fixes #5879
Changes
Remove the root expression analysis as it is not safe in some edge cases.
Change the sharding labels merge logic when we want to shard by and without at the same time.
Verification