Skip to content

Improvements to CAST and CONVERT functions #1843

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jun 23, 2023
Merged

Improvements to CAST and CONVERT functions #1843

merged 10 commits into from
Jun 23, 2023

Conversation

fulghum
Copy link
Contributor

@fulghum fulghum commented Jun 21, 2023 •
edited
Loading

This PR adds support for casting/converting to FLOAT and DOUBLE types with the CAST and CONVERT functions. It also adds support for length (aka precision) and scale type constraints (e.g. CAST(1.2345 AS DECIMAL(3,2))).

Parser support for DOUBLE and FLOAT with CAST and CONVERT: dolthub/vitess#249

Fixes: dolthub/dolt#5835

@timsehn
Copy link
Contributor

timsehn commented Jun 21, 2023

Does this partially address this one?

dolthub/dolt#5835

I can't tell.

@fulghum
Copy link
Contributor Author

fulghum commented Jun 21, 2023

Does this partially address this one?

dolthub/dolt#5835

It doesn't directly address the issue with CHAR and DECIMAL in that issue, but while I'm in this function's code, I'll take another pass and see if those are easy fixes. Thanks for suggesting!

@fulghum fulghum changed the title Adding support for FLOAT and DOUBLE with CAST and CONVERT Improvements to CAST and CONVERT functions Jun 21, 2023
@fulghum fulghum requested a review from jycor June 22, 2023 16:36
jycor
jycor reviewed Jun 22, 2023
View reviewed changes
jycor
jycor reviewed Jun 22, 2023
View reviewed changes
jycor
jycor reviewed Jun 22, 2023
View reviewed changes
jycor
jycor approved these changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@jycor jycor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM.

Need bounds checking to prevent some panics.
Suggestion for erroring on invalid decimal types.

@fulghum fulghum merged commit 334b28e into main Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jycor jycor jycor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CAST not fully supported
3 participants
@fulghum @timsehn @jycor