-
Notifications
You must be signed in to change notification settings - Fork 510
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
Prefer Kotlin type for some functions #4517
Prefer Kotlin type for some functions #4517
Conversation
...tion-postgresql/src/test/kotlin/app/cash/sqldelight/postgresql/integration/PostgreSqlTest.kt
Outdated
Show resolved
Hide resolved
@@ -93,16 +94,17 @@ class MySqlTypeResolver( | |||
} | |||
|
|||
private fun SqlFunctionExpr.mySqlFunctionType() = when (functionName.text.lowercase()) { | |||
"greatest" -> encapsulatingType(exprList, INTEGER, REAL, TEXT, BLOB) | |||
"greatest" -> encapsulatingTypePreferringKotlin(exprList, INTEGER, REAL, TEXT, BLOB) | |||
"least" -> encapsulatingTypePreferringKotlin(exprList, BLOB, TEXT, INTEGER, REAL) |
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.
Figured I'd throw this in here (and for postgres) while I'm at it. I'm taking an educated guess at the correct order here; my brain is still spinning from trying to follow SQLite's logic.
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.
Ideally we should use the ordering from the db/dialect. Postgres has this algorithm: https://www.postgresql.org/docs/current/typeconv-union-case.html while ms-sql (not support at the moment) provides this nice list: https://learn.microsoft.com/en-us/sql/t-sql/data-types/data-type-precedence-transact-sql?view=sql-server-ver16
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.
OK I'll take a look
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 was double checking the type order for the MAX/MIN functions when I realized that in essentially all dialects except SQLite, MAX/MIN only allow a single argument, and for multiple arguments you should use GREATEST, LEAST.
This would allow simply taking the type from the expression list, but SqlDelight allows passing multiple arguments to MAX/MIN. I have a pretty simple changeset to not allow that, but I'm not sure if SqlDelight is looking to validate function usage. Some tests would have to be updated to only include SQLite when using MAX/MIN, and new tests would have to be added to exclude SQLite and test GREATEST/LEAST.
I don't want to do too much more work on that without a clear direction, so @hfhbd what do you think?
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.
Put up the PoC here
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.
Well, on the one hand I like "hardening" compiling sql code, on the other hand having this check code in the grammar is annoying with the current function resolver code in the dialect. Limiting the parameters in the grammar is way complex. I would simple put this logic in the function resolver and use exprList.single()
or similar. The drawback is missing grammar support.
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.
Limiting the parameters in the grammar is way complex
My PoC seemed pretty simple, am I missing something? I'm even considering have a mixin per dialect that can handle function validation at the grammar level for all functions (maybe with an ANSI base so it doesn't have to be repeated everywhere), and it would be pretty straightforward. That way there wouldn't need to be a mixin per function.
The drawback is missing grammar support
Which I think would be cool to have, and I'm happy to work on it. The only thing stopping me is the question of whether this falls in SqlDelght's domain. Nothing would technically stop working in SqlDelight without it.
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 would put the mixin in sql-psi, if we want it at all (+ Function1Param, Function2Param, etc). But the annoying things is to create a grammar rule for each possible function and to add the function + its return type in function resolver (or in another mixin).
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.
My idea was to override function_expr
in each dialect using the same definition as sql-psi (function_expr ::= function_name LP [ [ DISTINCT ] expr ( ( COMMA | FROM | FOR | SEPARATOR ) expr ) * | MULTIPLY ] RP
) and add a mixin.
Then each dialect can define how to handle each function in the mixin itself, by keying off of the name of the function. It could delegate to a "common" class that validates functions following ANSI, and have custom handling for things that are different in the specific dialect.
I'll push a commit to my PoC showing this
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.
🙈 OK somehow I was missing what the existing FunctionExprMixin
was doing. I updated the PoC to use that.
b565c1a
to
2bbcf3c
Compare
sqldelight-compiler/dialect/src/main/kotlin/app/cash/sqldelight/dialect/api/TypeResolver.kt
Outdated
Show resolved
Hide resolved
Very nice implementation. I thought about something similar, and I am very curious if all new tests will pass. |
2bbcf3c
to
98d6c65
Compare
@hfhbd I added some tests. LMK if there should be more/less I didn't add any tests for greatest/least because there weren't existing ones, but I'm not opposed to adding those. |
4c47cf2
to
529e4f1
Compare
sqldelight-compiler/src/test/kotlin/app/cash/sqldelight/core/queries/ExpressionTest.kt
Show resolved
Hide resolved
sqldelight-compiler/src/test/kotlin/app/cash/sqldelight/core/queries/ExpressionTest.kt
Show resolved
Hide resolved
sqldelight-compiler/dialect/src/main/kotlin/app/cash/sqldelight/dialect/api/TypeResolver.kt
Outdated
Show resolved
Hide resolved
sqldelight-compiler/dialect/src/main/kotlin/app/cash/sqldelight/dialect/api/TypeResolver.kt
Show resolved
Hide resolved
sqldelight-compiler/dialect/src/main/kotlin/app/cash/sqldelight/dialect/api/TypeResolver.kt
Show resolved
Hide resolved
3f5d061
to
9f668d1
Compare
6b7fb74
to
9c4e7eb
Compare
@hfhbd I think this one is good to go. What do you think of all these tests going into ExpressionTest? Is there a better place for it? |
9c4e7eb
to
ee9e637
Compare
ee9e637
to
09392dd
Compare
...tion-postgresql/src/test/kotlin/app/cash/sqldelight/postgresql/integration/PostgreSqlTest.kt
Outdated
Show resolved
Hide resolved
...tion-postgresql/src/test/kotlin/app/cash/sqldelight/postgresql/integration/PostgreSqlTest.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Niklas Baudy <niklas.baudy@vanniktech.de>
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 am back from vacations. Looks good to me.
Thanks, conflicts resolved |
Fixes #3572
The rules used to determine if the Kotlin type should be used:
I still need to add tests for new behavior, but all existing tests should pass.