-
Notifications
You must be signed in to change notification settings - Fork 323
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
Allow parse to pass through already matched type. #12016
Conversation
Make with warning public as wanted by user.
Closes #11991 |
|
||
c1 = t.at "X" . parse Value_Type.Integer | ||
setup.expect_integer_type c1 | ||
c1.to_vector . should_equal [42, 0, -1] | ||
c1.to_vector . should_equal v |
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.
Is there a way to make sure the column was returned directly?
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.
Perhaps we could do
c1 = t.at "X"
r1 = c1.parse ..Integer
Meta.is_same_object c1 r1 . should_be_true
Also PR comment.
column_value_type = column_to_parse.value_type | ||
already_parsed = Auto != type && column_value_type.is_same_type type | ||
if already_parsed then table else |
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.
I've just realised that this check is redundant to the check in Column.parse
.
We could discard this change altogether and all should still work - because we will just call parse
on all selected columns and for those of them that already fit the target type, they will be returned as-is and passed to set
- set
updating a column to the same column effectively does nothing.
I think it could be a worthwhile simplification to remove this custom logic from Table.parse
and only rely on Column.parse
ensuring the correct behaviour. That way the logic is only in 2 places (because of also DB_Column
) instead of 4.
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.
Looks good now.
I think we could further simplify a bit as the check inside of Table.parse
seems redundant with Column.parse
.
Pull Request Description
Table.parse
andColumn.parse
will allow the target type to be returned unchanged.Excel_Workbook.read_many
.DB_Table.sort
.Table.text_cleanse
widget by adding a default value.In_Any_Warn_On_Missing
forread_many
as wanted by user in review.Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.