fix(solparq): match S3 restore columns by name, not position - #68
Open
picard8472 wants to merge 1 commit into
Open
fix(solparq): match S3 restore columns by name, not position#68picard8472 wants to merge 1 commit into
picard8472 wants to merge 1 commit into
Conversation
The S3 restore used `INSERT INTO dest SELECT * FROM s3(...)`, which maps columns by position, while the local restore (`FORMAT Parquet`) maps by name. If a destination table's column order ever drifts from the archived file's, the local path restores fine but the S3 path silently writes into the wrong columns (or errors on a type mismatch). For an archive format meant to outlive schema changes, that drift will happen eventually. Read the archived file's own column order via `DESCRIBE TABLE s3(...)` and emit an explicit, backtick-quoted column list on both sides — `INSERT INTO dest (cols) SELECT cols FROM s3(...)` — so the S3 path matches by name, mirroring the local path. Fixes #62 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Problem
Fixes #62.
The solparq S3 restore path used
INSERT INTO dest SELECT * FROM s3(...), which maps columns by position. The local restore path (INSERT INTO dest FORMAT Parquet) maps by name. So if a destination table's column order ever differs from the archived file's, local restores fine but S3 silently writes into the wrong columns (or errors on a type mismatch).For an archive format meant to outlive schema changes, that drift will happen eventually. (Original review comment.)
Fix
Before the restore INSERT, read the archived file's own column order via
DESCRIBE TABLE s3(...)(ClickHouse only reads the Parquet footer for this — cheap), then emit an explicit, backtick-quoted column list on both sides:This makes the S3 path match columns by name, mirroring the local path. A file column absent from the destination now surfaces as an explicit error rather than a silent mis-column; a destination column absent from the file takes its default, exactly as the local
FORMAT Parquetreader already does.Changes
describe_s3_columnson the ClickHouse HTTP client + aquery_texthelper that keeps the response body.build_s3_restore_sqlnow takes the archived column list and emits the explicit(cols) SELECT colsform.S3Objectstruct groups the object URL + credentials (shared by describe and restore; also keepsbuild_s3_restore_sqlunder clippy's arg limit).quote_identifier/parse_describe_columnshelpers.crates/superbank/README.md.Testing
cargo test -p superbank --locked— all pass (new unit tests forparse_describe_columns,quote_identifier, and the name-matched SQL shape).cargo fmt --all -- --checkandcargo clippy -p superbank --all-targets --locked -- -D warnings— clean.🤖 Generated with Claude Code