sql: refactor PlanCDCExpression#143463
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 |
836109e to
400767c
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @asg0451)
pkg/sql/distsql_plan_changefeed.go line 136 at r1 (raw file):
// Walk the plan, perform sanity checks and extract information we need. var spans roachpb.Spans var presentation colinfo.ResultColumns
nit: presentation variable here should be removed in the first commit, not in the second, otherwise the first commit doesn't compile, right?
The column presentation of the top node is collected to determine the output columns for the CDC expression. There is no need to do this within the plan node walker, so it has been moved outside. Release note: None
The `Input` and `InputCount` methods of the `planNode` interface are now used to walk plan node trees of CDC expressions. This continues the effort to deprecate and remove the plan node walkers (see cockroachdb#137620 for more details on the motivation for this). Release note: None
The `return` statements for error cases now explicitly return an empty `CDCExpressionPlan` for clarity. Release note: None
400767c to
518b3fa
Compare
mgartner
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsql_plan_changefeed.go line 136 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit:
presentationvariable here should be removed in the first commit, not in the second, otherwise the first commit doesn't compile, right?
Great catch! Fixed.
|
TFTR! bors r+ |
sql: collect CDC presentation outside of plan walker
The column presentation of the top node is collected to determine the
output columns for the CDC expression. There is no need to do this
within the plan node walker, so it has been moved outside.
Release note: None
sql: use InputCount and Input planNode methods to walk CDC plans
The
InputandInputCountmethods of theplanNodeinterface are nowused to walk plan node trees of CDC expressions. This continues the
effort to deprecate and remove the plan node walkers (see #137620 for
more details on the motivation for this).
Epic: None
Release note: None
sql: refactor return statements in PlanCDCExpression
The
returnstatements for error cases now explicitly return an emptyCDCExpressionPlanfor clarity.Release note: None