give plan nodes bounds #142
Conversation
planner/bounds.go
Outdated
| } | ||
| } | ||
|
|
||
| s, bdd := node.ProcedureSpec().(BoundedProcedureSpec) |
There was a problem hiding this comment.
I'm wondering what the behavior for the flux function shift should be here. It's not bounded in the same way that range is, it just translates the time columns in its input by a fixed amount.
Maybe instead of BoundedProcedureSpec as an interface we need something like this
type BoundsAwareProcedureSpec interface {
TimeBounds(predBounds *plan.Bounds) plan.Bounds
}Both range and shift could implement this, but range would intersect, and shift would shift.
The behavior you have here is also the behavior of the old planner, so if you don't want to do this now, maybe that's okay.
There was a problem hiding this comment.
I completely overlooked shift. You are right, procedures should implement their own TimeBounds method. This is an easy fix. I'll add it.
planner/logical.go
Outdated
| @@ -134,6 +146,10 @@ func createLogicalPlan(spec *flux.Spec, a Administration) (*PlanSpec, error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| if err := v.plan.BottomUpWalk(ComputeBounds); err != nil { | |||
There was a problem hiding this comment.
I would have expected this to be in the physical planner, computed just before returning the final plan. The reason is, there may be rules we apply that refine the time bounds on the plan nodes, for example, if we can push a range down through a window.
There was a problem hiding this comment.
Yes you're right, I'll move this to end of planning.
|
Looks great. I like the tests. |
91cc5a4 to
3c6d310
Compare
This PR gives a
PlanNodetime bounds. Originally I had tried to make bounds plan node statistics. However, because the execution engine uses the bounds associated with each plan node, the execution engine would also have to have access to input statistics for each node. To go that direction would require more design, so instead I made bounds a property of the node itself. Thoughts?