backup: add BEFORE/AFTER syntax to SHOW BACKUPS - #160085
Conversation
|
This change is |
There was a problem hiding this comment.
Pull request overview
This PR adds time range filtering syntax to the SHOW BACKUPS command, allowing users to filter backups using BEFORE <timestamp> and/or AFTER <timestamp> clauses.
Key changes:
- Introduces
ShowBeforeAfterstruct to represent time range filtering with BEFORE/AFTER expressions - Updates parser grammar to accept BEFORE/AFTER clauses in any order (normalized to BEFORE-AFTER)
- Adds comprehensive test coverage for all syntax variations
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/sql/sem/tree/show.go | Adds ShowBeforeAfter struct with formatting and default-checking methods; integrates time range into ShowBackup AST node |
| pkg/sql/parser/sql.y | Extends grammar to support opt_show_before_after_clause with BEFORE/AFTER in any order; adds helper function for type conversion |
| pkg/sql/parser/testdata/backup_restore | Adds test cases covering BEFORE-only, AFTER-only, both clauses, and order normalization scenarios |
| docs/generated/sql/bnf/stmt_block.bnf | Updates BNF grammar to include opt_show_before_after_clause in the SHOW BACKUPS syntax |
| docs/generated/sql/bnf/show_backup.bnf | Updates BNF grammar for SHOW BACKUP statement to include the new time range clause |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| func (s *ShowBeforeAfter) Format(ctx *FmtCtx) { | ||
| if s.Before != nil { | ||
| ctx.WriteString("BEFORE ") |
There was a problem hiding this comment.
i find it more intuitive for "after" to be returned before "before". as after indicates the lower bound. But maybe i'm weird. i don't have strong opinions.
e.g.: AFTER 10/1 BEFORE 10/3 reads easier to me than BEFORE 10/3 AFTER 10/1.
There was a problem hiding this comment.
also, i imagine BEFORE will be used less frequently
There was a problem hiding this comment.
Oh yea I guess semantically it makes more sense for AFTER to show up first. I was thinking linguistically. I think your interpretation makes more sense.
Potential Bug(s) DetectedThe three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation. Next Steps: Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary. After you review the findings, please tag the issue as follows:
|
github-actions
Bot
added
the
o-AI-Review-Potential-Issue-Detected
Potential Bug(s) DetectedThe three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation. Next Steps: Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary. After you review the findings, please tag the issue as follows:
|
This commit adds the `AFTER <ts> BEFORE <ts>` to the `SHOW BACKUPS` syntax. Epic: CRDB-57536 Informs: #159647 Release note: None
kev-cao
removed
the
o-AI-Review-Potential-Issue-Detected
|
TFTR! bors r=msbutler |
celeste-cockroachdb
Bot
added
v26.2.0-prerelease
and removed
target-release-26.2.0
labels
This commit adds the
BEFORE <ts> AFTER <ts>to theSHOW BACKUPSsyntax.Epic: CRDB-57536
Informs: #159647
Release note: None