Skip to content
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

Merged
merged 17 commits into from
Apr 6, 2023
Merged

don't push Filter below Limit #1692

merged 17 commits into from
Apr 6, 2023

Conversation

jycor
Copy link
Contributor

@jycor jycor commented Apr 3, 2023

When there's a query with a filter over a subquery with a limit, we incorrectly push filters down to the subquery.
Example:

This

select * from (select * from t limit 1) t where i > 1;

is not equivalent to

select * from (select * from t where i > 1) t limit 1;

Fix for: dolthub/dolt#5568

@@ -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
Copy link
Contributor

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?

@@ -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 {
Copy link
Contributor

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)}},
},
{
Copy link
Contributor

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

Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚢

@jycor jycor merged commit 4ee4a4c into main Apr 6, 2023
@jycor jycor deleted the james/limit branch July 21, 2023 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants