PRDCT-356: time travel restore works on BigQuery too, not Snowflake only - #1083
Draft
Iamfle4ka wants to merge 3 commits into
Draft
PRDCT-356: time travel restore works on BigQuery too, not Snowflake only#1083Iamfle4ka wants to merge 3 commits into
Iamfle4ka wants to merge 3 commits into
Conversation
The June storage audit flagged "Time travel restore -- available only for the Snowflake backend" as wrong. PRDCT-356 was then closed as Done and PR #979 was merged -- but #979 does not touch backups.md at all, so this finding was silently dropped while the issue closed. The sentence has been live ever since. Settled from the platform code rather than the audit note: keboola/php-storage-driver-bigquery ships src/Handler/Table/Create/CreateTableFromTimeTravelHandler.php, which runs CREATE TABLE <dest> AS SELECT * FROM <src> FOR SYSTEM_TIME AS OF <timestamp>; and it is covered by two active (not skipped) functional tests in tests/functional/UseCase/Table/CreateTableFromTimeTravelTest.php that assert real row counts -- 8 rows in the source, 4 in the table restored from the earlier timestamp. So the audit was right and the docs are wrong. - Both the method list and the section body now name Snowflake and BigQuery. - Added the Pay As You Go exclusion, which /management/payg-project/ already states but this page never mentioned. (Note that page's slug is payg-project, not pay-as-you-go -- the obvious guess is a broken link.) - Left a VERIFY(owner) on the retention ceiling instead of inventing one: on Snowflake it follows the project data-retention setting, but on BigQuery the limit is the dataset's own time-travel window, which BigQuery caps at 7 days. The page's single "limit set for the project" phrasing may not hold for both. Verified: build clean (306 pages); audit-phase2 back to the main baseline of 45 broken links / 0 missing images after correcting the payg slug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The second code-accuracy finding from the June storage audit, and the one #979 deliberately left out as needs_human. The audit reports that /storage/tables/ is wrong for COLUMN names specifically: that dashes are rejected by ColumnNameValidator.php, and that a column name is capped at 64 characters while a table name allows 96 -- neither limit documented. That validator lives in the private keboola/connection repo. I could not confirm any of it from public sources (searched the public keboola org, php-table-backend-utils, storage-api-php-client), so the user-facing sentence is left EXACTLY as it was rather than reversed on the strength of an audit note. Added a VERIFY(owner) explaining the contested claim and why it matters: if dashes really are invalid for columns, that sentence actively misleads anyone naming one. Someone with connection access can settle it in a minute. Verified: build clean (306 pages); audit-phase2 unchanged at 45 broken links / 0 missing images. No user-visible text changed in this commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
keboola-pr-reviewer-bot
left a comment
There was a problem hiding this comment.
Verdict: needs_human (risk 3/5) · profile connection-docs
Escalating: the diff adds two unresolved VERIFY(owner) markers, failing content-bucket auto-approve gate 3.
Concerns:
src/content/docs/storage/tables/backups.md: Unresolved VERIFY(owner) added on BigQuery time-travel retention ceilingsrc/content/docs/storage/tables/index.md: Unresolved VERIFY(owner) added; column-name rule unconfirmed from public codesrc/content/docs/storage/tables/backups.md: New BigQuery time-travel availability claim needs owner/code confirmation
Suggested reviewers: @keboola/docs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The June storage audit flagged "Time travel restore – available only for the Snowflake backend" as wrong. PRDCT-356 was then closed as Done and PR #979 merged — but #979 does not touch
backups.mdat all. The finding was silently dropped while the issue closed, and the sentence has been live ever since.This is the clearest example I found of a pattern worth naming: a closed Linear issue is not evidence that a defect was fixed.
Settled from the platform code, not the audit note
keboola/php-storage-driver-bigqueryshipssrc/Handler/Table/Create/CreateTableFromTimeTravelHandler.php, which runs:It is covered by two active, non-skipped functional tests in
tests/functional/UseCase/Table/CreateTableFromTimeTravelTest.phpthat assert real row counts — 8 rows in the source, 4 in the table restored from the earlier timestamp.So the audit was right: time travel restore is a working, tested BigQuery feature and the docs deny it exists.
Changes
/management/payg-project/already states time travel is unsupported there; this page never mentioned it.VERIFY(owner)on the retention ceiling instead of inventing a number: on Snowflake it follows the project data-retention setting, but on BigQuery the limit is the dataset's own time-travel window, which BigQuery caps at 7 days (2–7 configurable). The page's single "limit set for the project" phrasing may not hold for both backends, and I could not confirm what a BigQuery project is actually allowed.A side observation for the docs
The Pay As You Go page's slug is
management/payg-project, notmanagement/pay-as-you-go— the path-shaped guess is a broken link, and I hit it myself before checking./management/project/limits/links it correctly. Worth knowing when writing new links to it.Verification
Build clean, 306 pages.
audit-phase2back to themainbaseline of 45 broken links / 0 missing images after correcting that slug.Fourth PR from the reconciliation of the June audits against the live sites — companions #1080, #1081, #1082.
Second commit — the column-naming rule, flagged rather than guessed
This is PRDCT-356's other code-accuracy finding, and the one #979 deliberately left out as
needs_human.The audit reports that
/storage/tables/is wrong for column names specifically: that dashes are rejected byColumnNameValidator.php, and that a column name is capped at 64 characters while a table name allows 96 — neither limit documented.I could not confirm any of it. That validator lives in the private
keboola/connectionrepo; I searched the public keboola org,php-table-backend-utils, andstorage-api-php-clientand found no public validation logic. So the user-facing sentence is left exactly as it was — I am not reversing a documented rule on the strength of an audit note.What this commit adds is a
VERIFY(owner)recording the contested claim and why it matters: the page currently tells readers "Dash and underscores are allowed", and if dashes really are invalid for columns, that sentence actively misleads anyone naming one. Someone withconnectionaccess can settle it in a minute — and it is worth settling, because this is a rule users hit while typing.No user-visible text changed in this commit; build clean,
audit-phase2unchanged.