feat(planner): logical plan for rcte #16680
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
| _predicate: Condition, | ||
| _ctx: &mut PredicatePushdownContext, | ||
| ) -> PlanRef { | ||
| self.clone().into() |
There was a problem hiding this comment.
We should handle predicate pushdown as well.
There was a problem hiding this comment.
yep this should be handled once the optimization is enabled - since there is no actual implementation for predicate pushdown at present, it's alright.
There was a problem hiding this comment.
I think we need to change it to unimplemented!(), if we don't handle it right now, because the current implementation doesn't handle the push down predicate and it will generate a wrong plan.
There was a problem hiding this comment.
it will generate a wrong plan
I don't quite get this, why directly cloning the current plan without predicate pushdown will lead to a wrong plan? 👀
There was a problem hiding this comment.
If you don't want to push down the predicate you should implement it like this, because the predicate in this function couldn't be thrown away, at least we need to generate a filter for it.
fn predicate_pushdown(
&self,
predicate: Condition,
ctx: &mut PredicatePushdownContext,
) -> PlanRef {
// No pushdown.
gen_filter_and_pushdown(self, predicate, Condition::true_cond(), ctx)
}
There was a problem hiding this comment.
update the implementation for predicate_pushdown, I think it's reasonable to push predicate down to the base and recursive plan.
| fn prune_col(&self, required_cols: &[usize], ctx: &mut ColumnPruningContext) -> PlanRef { | ||
| let new_inputs = self | ||
| .inputs() | ||
| .iter() | ||
| .map(|input| input.prune_col(required_cols, ctx)) | ||
| .collect_vec(); | ||
| let new_plan = self.clone_with_inputs(&new_inputs); | ||
| self.ctx() | ||
| .insert_rcte_cache_plan(self.core.id, new_plan.clone()); | ||
| new_plan | ||
| } |
There was a problem hiding this comment.
Here we reuse the ShareId but insert a new_plan which might have a different schema from the old_plan, but cte_ref has a base field which represents a plan at the beginning of planning, so it would be inconsistent with the rcte_cache_plan updated here. That's why I think we should never use base of cte_ref after the initial planning.
There was a problem hiding this comment.
but cte_ref has a base field which represents a plan at the beginning of planning, so it would be inconsistent with the rcte_cache_plan updated here.
agree. the only essential use case for base is to get the current OptimizerContext, for GenericPlanNode's impl methods I think we could just stick with the current stream_key's logic - in this case everything is consistent with the on-the-fly plan cache stored in the context.
There was a problem hiding this comment.
implementation of the current stream_key is here (the consistent one we should stick to): https://github.com/risingwavelabs/risingwave/pull/16680/files#diff-1d6ad1cdae116ec20167aafcc080d20606c2fb3da492cea9c6a398412456d96bR63.
|
@chenzl25 could you help me merge this pr, thanks! |
Sure |
a63fa8c
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
refer #16483.
Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.