Fix failure when Unowned lookup IS NULL #10346
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This test case shows a regression in the query planner for queries that use `IS NULL` condition on an unowned lookup vindex column. Signed-off-by: Arthur Schreiber <arthurschreiber@github.com> Co-authored-by: Elisa Vaccaro <30807261+evaccaro@users.noreply.github.com> Co-authored-by: Jakob Class <mikrobi@github.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
systay
requested review from
harshit-gangal,
frouioui and
GuptaManan100
as code owners
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
3 tasks
frouioui
changed the title
|
Updating the title of this PR to facilitate the release notes generation. |
frouioui
approved these changes
May 24, 2022
| utils.Exec(t, conn, "select * from t8 WHERE t9_id IS NULL") | ||
|
|
||
| // Cleanup | ||
| utils.Exec(t, conn, "delete from t9 WHERE parent_id = 1") |
There was a problem hiding this comment.
nit: we could put this in a defer in case line 88 fails. (I don't remember if utils.Exec terminate the test early if there is a failure, if not, then there is no need for a defer)
Comment on lines
+1832
to
+1834
| if !ContainsAggregation(k) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
this wouldn't support select exprs like count(a.id)+count(b.id) since ContainsAggregation checks for FuncExpr, is that a problem?
There was a problem hiding this comment.
you're thinking of IsAggregation. ContainsAggregation checks in the whole expression tree for aggregation
This was referenced May 30, 2022
harshit-gangal
pushed a commit
to planetscale/vitess
that referenced
this pull request
Aug 16, 2022
* Add a failing test case showing Gen4 planner failure. This test case shows a regression in the query planner for queries that use `IS NULL` condition on an unowned lookup vindex column. Signed-off-by: Arthur Schreiber <arthurschreiber@github.com> Co-authored-by: Elisa Vaccaro <30807261+evaccaro@users.noreply.github.com> Co-authored-by: Jakob Class <mikrobi@github.com> * bugfix: Limit the use of NoRoutesSpecialHandling to when it's needed Signed-off-by: Andres Taylor <andres@planetscale.com> Co-authored-by: Arthur Schreiber <arthurschreiber@github.com> Co-authored-by: Elisa Vaccaro <30807261+evaccaro@users.noreply.github.com> Co-authored-by: Jakob Class <mikrobi@github.com> Signed-off-by: Harshit Gangal <harshit@planetscale.com>
deepthi
added a commit
that referenced
this pull request
Aug 16, 2022
…eeded (#11020) * fix V3: only use the special NoRoute handling mode when needed (#10422) Signed-off-by: Andres Taylor <andres@planetscale.com> * fix: build Signed-off-by: Harshit Gangal <harshit@planetscale.com> * Fix failure when Unowned lookup IS NULL (#10346) * Add a failing test case showing Gen4 planner failure. This test case shows a regression in the query planner for queries that use `IS NULL` condition on an unowned lookup vindex column. Signed-off-by: Arthur Schreiber <arthurschreiber@github.com> Co-authored-by: Elisa Vaccaro <30807261+evaccaro@users.noreply.github.com> Co-authored-by: Jakob Class <mikrobi@github.com> * bugfix: Limit the use of NoRoutesSpecialHandling to when it's needed Signed-off-by: Andres Taylor <andres@planetscale.com> Co-authored-by: Arthur Schreiber <arthurschreiber@github.com> Co-authored-by: Elisa Vaccaro <30807261+evaccaro@users.noreply.github.com> Co-authored-by: Jakob Class <mikrobi@github.com> Signed-off-by: Harshit Gangal <harshit@planetscale.com> Signed-off-by: Andres Taylor <andres@planetscale.com> Signed-off-by: Harshit Gangal <harshit@planetscale.com> Signed-off-by: deepthi <deepthi@planetscale.com> Co-authored-by: Andres Taylor <andres@planetscale.com> Co-authored-by: Arthur Schreiber <arthurschreiber@github.com> Co-authored-by: Elisa Vaccaro <30807261+evaccaro@users.noreply.github.com> Co-authored-by: Jakob Class <mikrobi@github.com> Co-authored-by: deepthi <deepthi@planetscale.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
We should not use the
NoRoutesSpecialHandlingmode for vindex lookup queries.This PR changes when we apply this special handling mode. Before, we did it for all plans that were single routes, with this change we are more conservative and only use it in one of two situations:
select count(*) from tbl.dualquery. If we have been able to merge adualquery with a subquery, we need to run the query and return something, even if no routes can be found to match the subquery predicates. This is because thedualtable always returns a single row. Example of this would beselect exists(select 1 from tbl where foo = 43)Related Issue(s)
Checklist
Deployment Notes