-
-
Notifications
You must be signed in to change notification settings - Fork 213
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
don't push Filter
below Limit
#1692
Conversation
sql/analyzer/pushdown.go
Outdated
@@ -672,6 +685,19 @@ func pushdownFiltersUnderSubqueryAlias(ctx *sql.Context, a *Analyzer, sa *plan.S | |||
// pushdownIndexesToTable attempts to convert filter predicates to indexes on tables that implement | |||
// sql.IndexAddressableTable | |||
func pushdownIndexesToTable(a *Analyzer, tableNode sql.NameableNode, indexes map[string]*indexLookup) (sql.Node, transform.TreeIdentity, error) { | |||
hasLimit := false |
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 one confuses me a bit. This function is called on *plan.ResolvedTable
and *plan.TableAlias
. Can either of those have a limit as a child?
sql/analyzer/pushdown.go
Outdated
@@ -413,6 +422,10 @@ func convertFiltersToIndexedAccess( | |||
// pushdown, it will get picked up in the isolated pass | |||
// run by the filters pushdown transform. | |||
return false | |||
case *plan.Filter: | |||
if childIsLimit { |
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.
might be easier to just put the limit check here, if it's only used once
@@ -2723,6 +2723,38 @@ var QueryTests = []QueryTest{ | |||
Query: "SELECT i FROM mytable ORDER BY i LIMIT 1 OFFSET 1;", | |||
Expected: []sql.Row{{int64(2)}}, | |||
}, | |||
{ |
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.
seems like good subquery and single relation coverage! might be worth adding join tests, root and subscopes, might behave differently
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 🚢
When there's a query with a filter over a subquery with a limit, we incorrectly push filters down to the subquery.
Example:
This
is not equivalent to
Fix for: dolthub/dolt#5568