sql: refactor tree.RoutineExpr #95885
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball)
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:
pkg/sql/sem/tree/routine.go line 32 at r1 (raw file):
// by a RoutinePlanGenerator. isFinalPlan is true if no more plans will be // generated. type RoutinePlanEnumerator func(plan RoutinePlan, isFinalPlan bool) error
[super nit] I'm not sure RoutinePlanEnumerator makes sense to me here, since it isn't doing the enumerating. Maybe something like RoutinePlanExecutor?
mgartner
force-pushed
the
routine-generator
branch
from
3bf17bb to
44b3a54
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/routine.go line 32 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
[super nit] I'm not sure
RoutinePlanEnumeratormakes sense to me here, since it isn't doing the enumerating. Maybe something likeRoutinePlanExecutor?
Good point. How about RoutinePlanGeneratedFunc? I'm steering away from Executor because it assumes that they plan will be executed - but it doesn't have to be - the caller using the generator can do what they will with the plans.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/routine.go line 32 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Good point. How about
RoutinePlanGeneratedFunc? I'm steering away fromExecutorbecause it assumes that they plan will be executed - but it doesn't have to be - the caller using the generator can do what they will with the plans.
I like that better. Another option could be Handler, but I'm ok with either of those.
mgartner
force-pushed
the
routine-generator
branch
2 times, most recently
from
982cb7e to
69825d9
Compare
`tree.RoutineExpr` no longer tracks the number of statements in the routine. Instead, it has a `tree.RoutinePlanGenerator` that generates a plan for each statement in the routine, given a list of arguments, and invokes a given callback on the plan. Release note: None
mgartner
force-pushed
the
routine-generator
branch
from
69825d9 to
08ac8fd
Compare
|
TFTR! bors r+ |
|
Build succeeded: |
tree.RoutineExprno longer tracks the number of statements in theroutine. Instead, it has a
tree.RoutinePlanGeneratorthat generates aplan for each statement in the routine, given a list of arguments, and
invokes a given callback on the plan.
Epic: None
Release note: None