Skip to content
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

Render enum/set variables as strings after they are set #1922

Merged
merged 4 commits into from Aug 3, 2023

Conversation

adjeiv
Copy link
Contributor

@adjeiv adjeiv commented Aug 3, 2023

Fixes dolthub/dolt#6370
Ensure that when getting all session variables, if any of them is of an enum/set type, we convert them back to their string value. The SET command would amend the value in the session variables map to its numerical representation, which we need to translate back.

@adjeiv
Copy link
Contributor Author

adjeiv commented Aug 3, 2023

Note - system_variables.go may have similar behaviour, as the code there also doesn't perform this conversion back to string

@timsehn
Copy link
Sponsor Contributor

timsehn commented Aug 3, 2023

Thanks for the PR. @fulghum will look at this today.

@fulghum fulghum self-assigned this Aug 3, 2023
@fulghum
Copy link
Contributor

fulghum commented Aug 3, 2023

Note - system_variables.go may have similar behaviour, as the code there also doesn't perform this conversion back to string

Ahh... good callout. Looks like SET @@GLOBAL.SQL_MODE='ONLY_FULL_GROUP_BY'; SHOW GLOBAL VARIABLES LIKE '%sql_mode%'; has the same issue, and a similar fix could be applied. I think it's fine to handle that fix separately from this session variable fix. No pressure of course, but if you're feeling up for it, that would be another really appreciated contribution! 😁

I also quickly tested with enums, but I wasn't able to reproduce this issue with them.

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @adjeiv! 🙏 Very cool to see you jump into the codebase and contribute this fix! ⚡️

Let us know if you need any help looking for other good issues to dig into!

@fulghum fulghum merged commit 2c287de into dolthub:main Aug 3, 2023
6 checks passed
@bpf120
Copy link
Contributor

bpf120 commented Aug 8, 2023

Hi @adjeiv, Thanks for contributing. Do you have a Dolt use case you are working on too? We'd love to hear about it. :)

Feel free to email me or swing by our Discord if you'd like to share.

@adjeiv adjeiv deleted the show_variables_string_render branch August 11, 2023 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SHOW VARIABLES should render enum/set values
5 participants