Sort columns in st_column_changed before automigrating.
#3203
Conversation
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
Shubham8287
reviewed
Aug 27, 2025
Shubham8287
approved these changes
Aug 27, 2025
Centril
approved these changes
Aug 27, 2025
Contributor
|
@gefjon Can you drop the base-branch and re-target this at master? I think this can go before the error-reporting PR. |
gefjon
force-pushed
the
phoebe/st_column_changed-sort
branch
from
a132f44 to
d8ace00
Compare
gefjon
changed the base branch from
phoebe/change-type-better-errors
to
master
During bootstrapping, indexes don't exist, so `iter_by_col_eq` returns rows in physical storage order. In simple cases (without unrelated deletes) this will be the same as insertion order, and therefore also the same as sorted order, but in cases with multiple column-changing automigrations the physical storage order of the rows diverges from the correct sorted order. Because this isn't a hot path, we can just call `.sort()` and not worry about it.
Co-authored-by: Shubham Mishra <shubham@clockworklabs.io> Signed-off-by: Mazdak Farrokhzad <twingoow@gmail.com>
gefjon
force-pushed
the
phoebe/st_column_changed-sort
branch
from
612a0c8 to
f7bc21a
Compare
Centril
approved these changes
Aug 27, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 29, 2025
…ns (#3202) # Description of Changes Title. When validating column type changes in `Table::change_columns_to` and comparing layouts in `RowTypeLayout::is_compatible_with`, return structured error objects which describe the mismatch. I made this change while debugging the issue that led to #3203 , where `change_columns_to` returned an error during replay of an automigration which had succeeded the first time. The original error contained enough information to debug, but it was presented in a noisy and unhelpful way, so I wrote this patch to get better diagnostics. Per @Centril 's comments, these errors are for internal assertions, not user-facing error reporting, so we value fail-fast rather than error hygiene and choose not to use the `ErrorStream` combinator. Also note that I sprinkled `Box`es around some large-ish error types to quiet https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err . # API and ABI breaking changes N/a # Expected complexity level and risk 1. # Testing Manual testing with BitCraft. As this is purely altering the error reporting for non-user-facing assertions, I don't believe any further testing is necessary.
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.
Description of Changes
In
st_column_changed,iter_st_column_for_tablereturns rows in physical storage order. In simple cases (without unrelated deletes) this will be the same as insertion order, and therefore also the same as sorted order, but in cases with multiple column-changing automigrations the physical storage order of the rows diverges from the correct sorted order. Because this isn't a hot path, we can just call.sort()and not worry about it.API and ABI breaking changes
N/a
Expected complexity level and risk
1
Testing
None yet. Needs manual testing with BitCraft update.