[backupccl] Use Expr for backup's Detached and Revision History options#85146
[backupccl] Use Expr for backup's Detached and Revision History options#85146craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
is the goal to do something like |
Not quite - We want to be able to take a BackupOptions struct, and distinguish between revision_history==false and revision_history==nil. In the former case we'll set revision_history to false, and in the latter case we'll make no changes (and assume that another field in BackupOptions is in fact set, representing the clause of the ALTER command). That make sense? |
38b870f to
7d90c69
Compare
pkg/sql/parser/sql.y
Outdated
| $$.val = &tree.BackupOptions{CaptureRevisionHistory: true} | ||
| $$.val = &tree.BackupOptions{CaptureRevisionHistory: tree.MakeDBool(true)} | ||
| } | ||
| | REVISION_HISTORY '=' TRUE |
There was a problem hiding this comment.
Should we just make this a general expr instead of the literals only? seems like placeholder support would be nice.
There was a problem hiding this comment.
done, this look good?
There was a problem hiding this comment.
yep, however now that it is an expr, you'll need to evaluate it rather than just compare it to DBoolTrue below, and I'm now looking at PlanHook and I'm not actually sure if we have that eval + check plumbed to here; might need to ask some sql folks how we should be eval'ing and typing as bool
There was a problem hiding this comment.
I think since you know that this expression must be of type bool, you can
type check it yourself.
typedExpr, err := expr.TypeCheck(ctx, semaCtx, types.Bool)
|
side nit: following the go project style, we use |
6676b1a to
71edb61
Compare
|
I made some changes to the planner to properly type-check bools, take another look? |
dt
left a comment
There was a problem hiding this comment.
Nice! one wrinkle with DETACHED which makes it special, but also maybe irrelevant to the ALTER work motivating this change
Release note (sql change): Can now specify explicit "true" and "false" values for "detached" and "revision_history" arguments in CREATE BACKUP and CREATE SCHEDULE FOR BACKUP.
|
Test failure appears on master, likely fixed by #85281 |
|
bors r+ |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
This will allow us to set them to null, which will be helpful for ALTER commands.
Release note: None