-
-
Notifications
You must be signed in to change notification settings - Fork 457
Typecast bugfix #4961
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Typecast bugfix #4961
Conversation
mathemancer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work.
|
@zackkrida I'll leave it to you to figure out where/how to merge this. Specifically, I suppose you should make a branch off of master for 0.7.1 (since we don't want to keep any post-0.7.0 changes in develop right now) and rebase this onto that, then merge back into it? |
caae932 to
09ceebd
Compare
|
I cherry-picked @Anish9901's commits and merged to a 0.7.1 branch based off of master. |
#4821 introduced a regression in which the appropriate
cast_optionsparameter wasn't passed for__msar.build_cast_exprfrom withinmsar.set_old_col_defaultthis led to a bug where retyping a column with non-dynamic defaults would result in the following error:We already had tests to ensure type casting a column with a non-dynamic default value would preserve the default and doesn't error but
msar.is_default_possibly_dynamicwrongly classified the non-dynamic default as dynamic which is why we didn't catch the aformentioned bug earlier. I've fixed the test setup so thatmsar.is_default_possibly_dynamiccorrectly classifies the defaults.Another bug that was introduced was related to typecasting
numericormathesar_moneyinferred columns to other types during the import preview.numeric(inferred) -->mathesar_moneywasn't possible as thecast_optionsdidn't containcurr_pref&curr_suffas the inferred column wasnumeric.mathesar_money(inferred) -->numericis still not possible as this is a lossy cast if there are currency symbols in the data.The added tests shows both of these cases.
Checklist
Update index.md).developbranch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin