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

sql/mysql: Attempt a fix for RawExpr cases #797

Merged
merged 5 commits into from May 15, 2022

Conversation

ofpiyush
Copy link
Contributor

@ofpiyush ofpiyush commented May 14, 2022

This is an optimistic attempt at a fix for expressions/functions in schema. Feel free to correct and guide me towards a better solve. :)

The wrapping is somewhat redundant when we already have sql() but going by the comments in code, backwards compatibility has made the situation a little messy.

Fixes #795

Note: Initially, MySQL looked like it was working fine without this change.
But if we tried to inspect -> drop database -> create. We ended up with string default instead of an expression.

@rotemtam rotemtam requested a review from a8m May 14, 2022 20:44
@rotemtam
Copy link
Member

hey @ofpiyush
thanks for working on this! notice that CI is red, do you mind taking a look at why?
🙏

@ofpiyush
Copy link
Contributor Author

Yes, I was playing with atlas and the bug just sucked me in. That commit was a lesson in why you shouldn't push code at 3 AM 😅

CURRENT_TIMESTAMP cases were failing. For now, I've added an exception for them.

A better solve for this family of problems will be fairly involved. Comparing the exact query can be brittle but attempting to compare the intent(via AST) behind the query might still result in wrong query and/or lot of edge cases to handle on a case to case basis.

Another such example is a field with default CONCAT(my_field,' anystr'). This generates an escaped query (tried with MySQL 8), which again breaks in the inspect -> drop database -> apply scenario.

@a8m
Copy link
Member

a8m commented May 15, 2022

Thanks for the contribution @ofpiyush 🙏

Code looks great, but the tests in integration/mysql_test.go need to be updated with the new inspection format. Let me know if you need any help with this.

Another such example is a field with default CONCAT(my_field,' anystr'). This generates an escaped query (tried with MySQL 8), which again breaks in the inspect -> drop database -> apply scenario.

Can you elaborate with an example? Did you address it in this PR? If not, that's fine, I can take this as well.

@ofpiyush
Copy link
Contributor Author

@a8m Would you believe that those tests pass in go 1.17? Would be interesting to know why!

I've updated the test cases.

Can you elaborate with an example? Did you address it in this PR? If not, that's fine, I can take this as well.

I've opened #803 to describe the bug. I haven't attempted it mostly because I am not sure if we're missing an unescape or is it because unescape itself isn't doing what it is supposed to.

sql/mysql/inspect.go Outdated Show resolved Hide resolved
Co-authored-by: Ariel Mashraki <7413593+a8m@users.noreply.github.com>
@a8m a8m merged commit 7c40300 into ariga:master May 15, 2022
@a8m
Copy link
Member

a8m commented May 15, 2022

Thanks for the contribution @ofpiyush 🙏

@ofpiyush ofpiyush deleted the cli-attempt-mysql-rawexpr-fix branch May 16, 2022 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inspect of default uuid() generates wrong schema.
3 participants