Skip to content

Commit

Permalink
[BUG] Fix projection pushdowns not working with limits (#2635)
Browse files Browse the repository at this point in the history
Fixes #2616

From Slack:
> Essentially the project and the limit are being pushed down in the
same rule batch, at the same time. However, this means that if a project
and a limit are in the same tree, the project gets pushed down, but then
the limit gets immediately pushed down beneath it, resulting in no
change, and the project never gets to merge with the scan.

This PR moves limit pushdowns into a different rule batch than
projection pushdowns, meaning they no longer conflict with each other.
  • Loading branch information
Vince7778 authored Aug 9, 2024
1 parent 2139c5f commit 8cf6974
Showing 1 changed file with 23 additions and 13 deletions.
36 changes: 23 additions & 13 deletions src/daft-plan/src/logical_optimization/optimizer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,19 +129,29 @@ pub struct Optimizer {
impl Optimizer {
pub fn new(config: OptimizerConfig) -> Self {
// Default rule batches.
let rule_batches: Vec<RuleBatch> = vec![RuleBatch::new(
vec![
Box::new(DropRepartition::new()),
Box::new(PushDownFilter::new()),
Box::new(PushDownProjection::new()),
Box::new(PushDownLimit::new()),
],
// Use a fixed-point policy for the pushdown rules: PushDownProjection can produce a Filter node
// at the current node, which would require another batch application in order to have a chance to push
// that Filter node through upstream nodes.
// TODO(Clark): Refine this fixed-point policy.
RuleExecutionStrategy::FixedPoint(Some(3)),
)];
let rule_batches: Vec<RuleBatch> = vec![
RuleBatch::new(
vec![
Box::new(DropRepartition::new()),
Box::new(PushDownFilter::new()),
Box::new(PushDownProjection::new()),
],
// Use a fixed-point policy for the pushdown rules: PushDownProjection can produce a Filter node
// at the current node, which would require another batch application in order to have a chance to push
// that Filter node through upstream nodes.
// TODO(Clark): Refine this fixed-point policy.
RuleExecutionStrategy::FixedPoint(Some(3)),
),
RuleBatch::new(
vec![
// This needs to be separate from PushDownProjection because otherwise the limit and
// projection just keep swapping places, preventing optimization
// (see https://github.com/Eventual-Inc/Daft/issues/2616)
Box::new(PushDownLimit::new()),
],
RuleExecutionStrategy::FixedPoint(Some(3)),
),
];
Self::with_rule_batches(rule_batches, config)
}

Expand Down

0 comments on commit 8cf6974

Please sign in to comment.