Skip to content

Add support for limit pushdown through union#14895

Merged
rongrong merged 1 commit intoprestodb:masterfrom
fgwang7w:14894
Aug 5, 2020
Merged

Add support for limit pushdown through union#14895
rongrong merged 1 commit intoprestodb:masterfrom
fgwang7w:14894

Conversation

@fgwang7w
Copy link
Member

@fgwang7w fgwang7w commented Jul 25, 2020

== RELEASE NOTES ==
General Changes
* Add support for limit pushdown through union

resolve #14894

@fgwang7w fgwang7w requested review from tdcmeehan and wenleix July 25, 2020 17:13
Copy link
Contributor

@wenleix wenleix left a comment

Choose a reason for hiding this comment

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

LGTM.

@kaikalur
Copy link
Contributor

Will be nice if this applies to ORDER BY LIMIT as well.

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

So, theoretically, this won't fire if the limit here is > 1 either right? Maybe also test that?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure I add another test to cover limit2 case

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer a separate check before building the list. Something like

if (unionNode.getSources().stream().allMatch(source -> isAtMost(source, context.getLookup(), parent.getCount()))) {
    return Result.empty();
}
// build new plan node

It's slightly more efficient when the rule should not fire, which should happen more often.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes this check makes sense, will incorporate it.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to check shouldApply anymore after the check. It should always apply if the code gets here.

@fgwang7w fgwang7w force-pushed the 14894 branch 2 times, most recently from fbaf69c to fa40064 Compare July 29, 2020 22:07
Cherry-pick of trinodb/trino@f244457

Co-authored-by: praveenkrishna <praveenkrishna@tutanota.com>
@rongrong rongrong merged commit e9f49df into prestodb:master Aug 5, 2020
@caithagoras caithagoras mentioned this pull request Aug 14, 2020
7 tasks
@fgwang7w fgwang7w deleted the 14894 branch September 4, 2020 03:17
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.

limit pushdown through union

4 participants