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

feat(mysql): Implement cast function parser #2473

Merged
merged 3 commits into from
Jul 30, 2023

Conversation

RadhiFadlillah
Copy link
Contributor

This is duplicate of #2020, which become closed because I accidentally deleted its source repository. Since I'm unable to recover the deleted repo, I decided to just recreate the PR.

What is this

As the title said, this PR wants to add support for CAST function in MySQL.

This PR is based from PR by @ryanpbrewster here (which unfortunately he didn't send here, and only exist in his repository).

Why is this PR created

Currently sqlc unable to infer the correct type from SQL function like MAX, MIN, SUM, etc. For those function, sqlc will return its value as interface{}. This behavior can be seen in this playground.

As workaround, it advised to use CAST function to explicitly tell what is the type for that column, as mentioned in #1574.

Unfortunately, currently sqlc only support CAST function in PostgreSQL and not in MySQL. Thanks to this, right now MySQL users have to parse the interface{} manually, which is not really desirable.

What does this PR do?

  • Implement convertFuncCast function for MySQL.
  • Add better nil pointer check in some functions that related to convertFuncCast.

I haven't write any test because I'm not sure how and where to put it. However, as far as I know the code that handle ast.TypeCast for PostgreSQL also don't have any test, so I guess it's fine 🤷‍♂️

Related issues

@andrewmbenton
Copy link
Collaborator

andrewmbenton commented Jul 18, 2023

Hi and thanks! I haven't looked at this closely yet but regarding the testing question I think the best thing would be to create a new endtoend test in either internal/endtoend/testdata/func_call_cast or internal/endtoend/testdata/select_column_cast. You can put the current contents of either directory into a postgresql subdirectory, and then create a mysql subdirectory for your test. The names of the directories don't matter, it's just our convention.

To generate the expected test output (from your patch) you'll naturally need a dev build of sqlc with your patch in it. You can build using make sqlc-dev for convenience.

You can run all the endtoend tests with go test ./internal/endtoend/... or just your test with go test -run TestReplay/testdata/func_call_cast/mysql -v ./internal/endtoend/.

@RadhiFadlillah
Copy link
Contributor Author

I've added the test, however I'm not really sure if I did it right:

  • I haven't added the postgresql directory because there is already pgx directory. Should I add it as well?
  • For func_call_cast in MySQL, I don't use UUID like in pgx directory because MySQL doesn't have UUID type like Postgres, and sqlc already return MySQL's UUID as string. So, I just use GREATEST function in MySQL which currently generated as interface{} by sqlc.

@andrewmbenton
Copy link
Collaborator

The tests look great, thanks! You don't need to add the postgresql directory, I can do it later. When we have multi-engine tests we would typically have both the stdlib and pgx directories within a postgresql directory alongside the mysql directory since there are two access methods supported for postgres. But all the tests will run in any case.

Regarding the actual code, I think there is a subtle change in behavior introduced by removing the switch statement in internal/compiler/compat.go. The default error (unexpected node type) will get returned in cases now where the node type is actually expected but the node pointer happens to be nil. I'm not sure exactly what the right thing to do is, but my suggestion for now is to leave the switch statement in place and return a new error in each case where the pointer can be nil, before operating on the value.

I think we can get this merged for the next release though, so I'm adding it to the 1.20 milestone.

@andrewmbenton andrewmbenton added this to the v1.20.0 milestone Jul 27, 2023
@RadhiFadlillah
Copy link
Contributor Author

Done, I've reverted the if with type assertion back to switch. I also added the nil check in each switch case.

@andrewmbenton
Copy link
Collaborator

I think this looks good. @kyleconroy if you agree feel free to merge or comment if not.

@kyleconroy kyleconroy changed the title Implement cast function parser for MySQL feat(mysql): Implement cast function parser Jul 30, 2023
@kyleconroy kyleconroy merged commit 39f16cc into sqlc-dev:main Jul 30, 2023
7 checks passed
andrewmbenton added a commit that referenced this pull request Jul 31, 2023
kyleconroy pushed a commit that referenced this pull request Jul 31, 2023
@RadhiFadlillah RadhiFadlillah deleted the mysql-cast branch August 17, 2023 12:19
This pull request was closed.
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.

3 participants