Add support for GREATEST and LEAST functions #2351
Conversation
There was a problem hiding this comment.
Thanks for getting started on this feature so quickly @solonovamax !
Please see the requested changes, mainly regarding extending Function instead and allowing users to provide multiple arguments with a function format instead of 2-argument infix format. It's preferable to be as general as the databases allow on the class level, then users can customize to restrict as much as they want on the function level.
Additionally, please rebase from main when addressing changes (should fix SQL Server issue on TC).
Please do add at minimum 2 tests, 1 for greatest() and least() each. I think a good goal to have would be the following functionality:
// assumes a basic table with all integer columns for example
// works with all columns
val top1 = greatest(TestTable.col1, TestTable.col2, TestTable.col3).alias("top1")
TestTable.select(top).where { TestTable.id eq 1 }
// works with columns + various expressions (including null)
val top2 = greatest(TestTable.col1, intLiteral(999), Op.nullOp(), intParam(56666)).alias("top2")
TestTable.select(top2).where { TestTable.id eq 1 }
// works with all columns + literals
val top3 = greatest(TestTable.col1, 999, 1, 65).alias("top3")
TestTable.select(top3).where { TestTable.id eq 1 }
// works with all literals (by using an expression as the first arg)
val top4 = greatest(intLiteral(24), 999, 1, 65).alias("top4")
TestTable.select(top4).where { TestTable.id eq 1 }
// works as part of a conditional operator
TestTable
.select(TestTable.col1)
.where { greatest(TestTable.col2, TestTable.col3) greaterEq 50 }It would also be great if 1 of the columns could also be nullable() to ensure that it working as expected (rules may vary with db).
| /** | ||
| * The greatest (maximum) of this expression and [t]. | ||
| */ | ||
| infix fun <T> ExpressionWithColumnType<T>.greatest(t: T): GreatestOp<T, T> = GreatestOp(this, wrap(t)) |
There was a problem hiding this comment.
Two thoughts about this:
Moving forward we are trying to align our DSL as closely to SQL when possible. So if the SQL looks like:
SELECT GREATEST(test_table.number_1, test_table.number_2, test_table.number_3, ...) FROM test_tableit would be preferable for our DSL to keep the function format instead of using infix.
TestTable
.select(greatest(TestTable.number1, TestTable.number2, TestTable.number3, ...)Secondly, unless I'm missing a use case, it will not be possible to use greatest() alone in a WHERE clause for example. So there should be no reason to include it in ISqlExpressionBuilder. These functions can remain in the same file, but please move them above to the misc section as top-level functions.
There was a problem hiding this comment.
Moving forward we are trying to align our DSL as closely to SQL when possible. So if the SQL looks like:
SELECT GREATEST(test_table.number_1, test_table.number_2, test_table.number_3, ...) FROM test_table
yeah, will do. I was also somewhat unsure of this, so I will change it.
Secondly, unless I'm missing a use case, it will not be possible to use
greatest()alone in aWHEREclause for example. So there should be no reason to include it inISqlExpressionBuilder. These functions can remain in the same file, but please move them above to the misc section as top-level functions.
It seems like there are other non-receiver and non-extension functions in ISqlExpressionBuilder, such as:
-
fun <T, S : T?> coalesce( expr: ExpressionWithColumnType<S>, alternate: Expression<out T>, vararg others: Expression<out T> ): Coalesce<T, S> = Coalesce<T, S>(expr, alternate, others = others)
-
fun case(value: Expression<*>? = null): Case = Case(value)
-
fun rank(): Rank = Rank()
-
fun concat(vararg expr: Expression<*>): Concat = Concat("", *expr)
So I'm not entirely clear on why the distinction is made for certain functions to be included in ISqlExpressionBuilder versus not.
There was a problem hiding this comment.
@solonovamax The functions should be moved outside ISqlExpressionBuilder for 3 reasons:
- Yes, there is legacy code that goes against what I'm asking, but we have been actively trying to improve the logic in the design as part of our ongoing efforts towards a higher standard of technical quality in this library. We are trying to do better moving forward with every new piece of code, particularly to ease the way for any future maintainers and contributors joining in.
- The pattern currently is that any new standalone function should be placed outside. Or rather, only functions/operators that are only used for building larger expressions in clauses should be included in the interface. Functions that can be used alone (for example, like
SELECT function()) should be left out. - If these functions are left in the interface, they will only be accessible (also in IDE typing suggestions) for invokation as methods in that context. Some users like to store lengthy function definitions in variables, which will only be possible in a not very nice way and trying to use them with
select()will also be cumbersome. This is both for legibility and to be able to use the variable when accessing values fromResultRow, and a reason why many of our users go straight to the classesConcat()andCoalesce()directly:
// will not compile
val topNumber = greatest(tester.number1, tester.number2, tester.number3).alias("top")
// also will not compile
tester.select(greatest(tester.number1, tester.number2, tester.number3))
// need to do this
val topNumber = SqlExpressionBuilder.greatest(tester.number1, tester.number2, tester.number3).alias("top")
// or this
tester.select(Greatest(tester.number1, tester.number2, tester.number1.columnType, tester.number3))
exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/SQLExpressionBuilder.kt
Outdated
Show resolved
Hide resolved
exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/vendors/FunctionProvider.kt
Outdated
Show resolved
Hide resolved
Alright, I wasn't sure whether it should be a
Alright, will do. At the time of PR I had used the latest commit to main to base my branch off of, however I assume this was a recent fix?
Will do. At the time I was just tired due to it being late at night, so I just submitted without the tests.
I'll need some time to look into the rules of this for each supported database, so I'll update this PR later. |
solonovamax
force-pushed
the
feature/greatest-least-function
branch
2 times, most recently
from
0a3e37d to
8bd8a2a
Compare
Adds support for GREATEST(X, Y) and LEAST(X, Y) functions. In SQLite, these are named MAX(X, Y) and MIN(X, Y), respectively. (because you just had to be different, huh sqlite?) Signed-off-by: solonovamax <[email protected]>
- Inherit from Function instead of Op - Make both functions accept varargs - Don't use an infix function for the expression builder Signed-off-by: solonovamax <[email protected]>
| class Greatest<T>( | ||
| val expr1: Expression<in T>, | ||
| val expr2: Expression<in T>, | ||
| columnType: IColumnType<T & Any>, | ||
| vararg val others: Expression<in T> | ||
| ) : Function<T>(columnType) { |
There was a problem hiding this comment.
@solonovamax The type parameters here won't allow for the possibility of the function returning a null value.
Please see how Min() is implemented or use my suggestion for Greatest from one of my previous comments. The type parameters for the functions will also need to be adjusted.
This can be tested using a nullable column as one of the function arguments (with null as the inserted value). Databases that consider null as the highest value (SQLite, Oracle, MySQL, MariaDB) will fail with NPE otherwise.
There was a problem hiding this comment.
The type parameters here won't allow for the possibility of the function returning a
nullvalue. Please see howMin()is implemented or use my suggestion forGreatestfrom one of my previous comments. The type parameters for the functions will also need to be adjusted.This can be tested using a nullable column as one of the function arguments (with
nullas the inserted value). Databases that considernullas the highest value (SQLite, Oracle, MySQL, MariaDB) will fail with NPE otherwise.
You sure?
Because according to the test cases I have, it's passing perfectly fine when returning a null value.
I hadn't pushed them previously because I had to do a bit of cleanup before being able to push them, however they're now ready to push.
|
@solonovamax Here is the style guide with a brief section on writing tests. Off the top of my head, I don't have a single example that would cover all the conventions you may need. But here's the test that I was using to review nullability, please feel free to use it as a starting point: @Test
fun testGreatestFunction() {
val highNumber = 99
val tester = object : IntIdTable("tester") {
val number1 = integer("number_1").default(highNumber)
val number2 = integer("number_2").nullable()
val number3 = integer("number_3").default(33)
}
withTables(tester) { testDb ->
val nullIsNotHigh = TestDB.ALL_POSTGRES_LIKE + TestDB.ALL_SQLSERVER_LIKE + TestDB.ALL_H2_V1
val id1 = tester.insertAndGetId { }
val top1 = greatest(tester.number1, tester.number2, tester.number3).alias("top1")
val result1 = tester.select(top1).where { tester.id eq id1 }.single()[top1]
assertEquals(if (testDb in nullIsNotHigh) highNumber else null, result1)
val top2 = greatest(highNumber, tester.number3.sum()).alias("top2")
val result2 = tester.select(top2).where { tester.id eq 1 }.single()[top2]
assertEquals(highNumber, result2)
val top3 = greatest(tester.number1, intLiteral(11), intParam(22), Op.nullOp()).alias("top3")
val result3 = tester.select(top3).where { tester.id eq 1 }.single()[top3]
assertEquals(if (testDb in nullIsNotHigh) highNumber else null, result3)
val result4 = tester
.select(tester.id)
.where { greatest(tester.number1, tester.number3) greaterEq 50 }
.single()[tester.id]
assertEquals(id1, result4)
}
}Please place the unit tests in either FunctionTests.kt or MathFunctionTests.kt. Please also check gradle task |
I don't believe that any of those detekt warnings are relevant to my code as they're all triggering in files I have not modified in this PR: |
Signed-off-by: solonovamax <[email protected]>
solonovamax
force-pushed
the
feature/greatest-least-function
branch
from
8bd8a2a to
c23adc0
Compare
nvm, it just does not include the detekt warnings in the CI logs. I ran it myself and found the offending code. Here is the warnings: And the offending code: override fun toQueryBuilder(queryBuilder: QueryBuilder): Unit = queryBuilder {
currentDialect.functionProvider.greatest(queryBuilder, expr1, expr2, *others)
}and override fun toQueryBuilder(queryBuilder: QueryBuilder): Unit = queryBuilder {
currentDialect.functionProvider.least(queryBuilder, expr1, expr2, *others)
}Should I suppress this warning? Because I don't think there's any other way to do this, tbh. |
Description
Summary of the change: Adds support for
GREATEST(X, Y)andLEAST(X, Y)functions.Detailed description:
GREATEST(X, Y)andLEAST(X, Y)SQL functions, to calculate the maximum and minimum of two values.This is especially useful in cases where you are performing a computation that contains a division on a value that could be zero.
GreatestOpandLeastOp. I've chosen to make them inherit fromOpfor now rather than fromFunction, however if it would be preferable that they inherit fromOp, then that could easily be done as well.I chose to delegate to
FunctionProviderrather than use awhenblock that matches against the dialect, as it's probably more maintainable like that.From what I can tell, SQLite is the only database which wants to be weird and instead of naming them
GREATEST(X, Y)andLEAST(X, Y), they're namedMAX(X, Y),MIN(X, Y).I'm unsure on the syntax of the functions for
ISqlExpressionBuilder, as current it would be used like this:However, it may be better if instead it was
I have not added any unit tests for this, however if desired I can add some.
Type of Change
Please mark the relevant options with an "X":
Updates/remove existing public API methods:
Affected databases:
Checklist
Related Issues
EXPOSED-681