-
Notifications
You must be signed in to change notification settings - Fork 697
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: Add ALL
and ANY
operators accepting array, subquery, or table parameters
#1886
Changes from 7 commits
6aeed36
be49f65
2c549b1
020313c
6076057
7d0ef40
c03aa1f
93253a3
dd296e6
7157813
eeec230
30d4b85
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -730,6 +730,9 @@ fun decimalParam(value: BigDecimal): Expression<BigDecimal> = QueryParameter(val | |
/** Returns the specified [value] as a blob query parameter. */ | ||
fun blobParam(value: ExposedBlob): Expression<ExposedBlob> = QueryParameter(value, BlobColumnType()) | ||
|
||
/** Returns the specified [value] as an array query parameter. */ | ||
fun <T> arrayParam(value: Array<T>): Expression<Array<T>> = QueryParameter(value, UntypedAndUnsizedArrayColumnType) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove this as it isn't necessary for the |
||
|
||
// Misc. | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,10 +5,10 @@ package org.jetbrains.exposed.sql | |
import org.jetbrains.exposed.dao.id.EntityID | ||
import org.jetbrains.exposed.dao.id.EntityIDFunctionProvider | ||
import org.jetbrains.exposed.dao.id.IdTable | ||
import org.jetbrains.exposed.sql.ops.InListOrNotInListBaseOp | ||
import org.jetbrains.exposed.sql.ops.PairInListOp | ||
import org.jetbrains.exposed.sql.ops.SingleValueInListOp | ||
import org.jetbrains.exposed.sql.ops.TripleInListOp | ||
import org.jetbrains.exposed.sql.SqlExpressionBuilder.eq | ||
import org.jetbrains.exposed.sql.functions.AllFunction | ||
import org.jetbrains.exposed.sql.functions.AnyFunction | ||
import org.jetbrains.exposed.sql.ops.* | ||
import org.jetbrains.exposed.sql.vendors.FunctionProvider | ||
import org.jetbrains.exposed.sql.vendors.currentDialect | ||
import java.math.BigDecimal | ||
|
@@ -96,6 +96,27 @@ fun <T : Any?> ExpressionWithColumnType<T>.varPop(scale: Int = 2): VarPop<T> = V | |
*/ | ||
fun <T : Any?> ExpressionWithColumnType<T>.varSamp(scale: Int = 2): VarSamp<T> = VarSamp(this, scale) | ||
|
||
// using `Op` | ||
|
||
/** Returns this array of data wrapped in the `ALL` operator. */ | ||
fun <T> Array<T>.allOp(): Op<T> = AllAnyOp("ALL", this) | ||
|
||
/** Returns this array of data wrapped in the `ANY` operator. The name is explicitly distinguished from [Array.any]. */ | ||
fun <T> Array<T>.anyOp(): Op<T> = AllAnyOp("ANY", this) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having these be extension functions makes it difficult to extend the functionality to, for example, a subquery. It also doesn't read like a natural SQL query: Please replace them with fun <T> anyFrom(array: Array<T>): AnyFromArrayOp<T> = AnyFromArrayOp(array)
fun <T> allFrom(array: Array<T>): AllFromArrayOp<T> = AllFromArrayOp(array)
// use
WHERE { Users.id eq anyFrom(arrayOf(1, 2)) }
WHERE { Users.id eq anyFrom(Users.selectAll()) } Also, please place the functions in the subsection titled "// Array Comparisons". |
||
|
||
// using `CustomFunction` | ||
|
||
/** Returns this array of data wrapped in the `ALL` operator. */ | ||
fun <T> Array<T>.allFunction(): CustomFunction<T> = AllFunction(this) | ||
|
||
/** Returns this array of data wrapped in the `ANY` operator. The name is explicitly distinguished from [Array.any]. */ | ||
fun <T> Array<T>.anyFunction(): CustomFunction<T> = AnyFunction(this) | ||
|
||
/** Checks if this expression is equal to any element from [array]. | ||
* This is a more efficient alternative to [ISqlExpressionBuilder.inList] on PostgreSQL and H2. */ | ||
infix fun <T> ExpressionWithColumnType<T>.eqAny(array: Array<T>): Op<Boolean> = | ||
this eq array.anyOp() // TODO or `array.anyFunction()` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This won't be necessary once the functions above are switched to take an argument instead of the more verbose extension function. |
||
|
||
// Sequence Manipulation Functions | ||
|
||
/** Advances this sequence and returns the new value. */ | ||
|
@@ -578,7 +599,7 @@ interface ISqlExpressionBuilder { | |
|
||
// Array Comparisons | ||
|
||
/** Checks if this expression is equals to any element from [list]. */ | ||
/** Checks if this expression is equal to any element from [list]. */ | ||
infix fun <T> ExpressionWithColumnType<T>.inList(list: Iterable<T>): InListOrNotInListBaseOp<T> = SingleValueInListOp(this, list, isInList = true) | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
package org.jetbrains.exposed.sql.functions | ||
|
||
import org.jetbrains.exposed.sql.UntypedAndUnsizedArrayColumnType | ||
import org.jetbrains.exposed.sql.CustomFunction | ||
import org.jetbrains.exposed.sql.arrayParam | ||
|
||
abstract class AllAnyFunction<T>(functionName: String, array: Array<T>) : CustomFunction<T>(functionName, UntypedAndUnsizedArrayColumnType, arrayParam(array)) | ||
class AllFunction<T>(array: Array<T>) : AllAnyFunction<T>("ALL", array) | ||
class AnyFunction<T>(array: Array<T>) : AllAnyFunction<T>("ANY", array) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package org.jetbrains.exposed.sql.ops | ||
|
||
import org.jetbrains.exposed.sql.Op | ||
import org.jetbrains.exposed.sql.QueryBuilder | ||
import org.jetbrains.exposed.sql.UntypedAndUnsizedArrayColumnType | ||
|
||
class AllAnyOp<T>(val opName: String, val array: Array<T>) : Op<T>() { | ||
|
||
override fun toQueryBuilder(queryBuilder: QueryBuilder) = queryBuilder { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My suggestion would be to replace this with a sealed class, something like this: sealed class AnyOrAllFromBaseOp<T>(
val isAny: Boolean,
val subSearch: Any
) : Op<T>() {
override fun toQueryBuilder(queryBuilder: QueryBuilder): Unit = queryBuilder {
if (isAny) {
+"ANY ("
} else {
+"ALL ("
}
when (subSearch) {
is Array<*> -> registerArgument(UntypedAndUnsizedArrayColumnType, subSearch)
}
+")"
}
}
class AnyFromArrayOp<T>(array: Array<T>) : AnyOrAllFromBaseOp<T>(isAny = true, subSearch = array)
class AllFromArrayOp<T>(array: Array<T>) : AnyOrAllFromBaseOp<T>(isAny = false, subSearch = array) With that, we'd be able to easily go in an extend support to subqueries, etc. by just adding a branch to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I refactored Also I added a |
||
+opName | ||
+'(' | ||
registerArgument(UntypedAndUnsizedArrayColumnType, array) | ||
+')' | ||
} | ||
} | ||
|
||
inline fun <reified T> AllAnyOp(opName: String, list: List<T>) = | ||
AllAnyOp(opName, list.toTypedArray()) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,6 +127,10 @@ abstract class DataTypeProvider { | |
open fun jsonBType(): String = | ||
throw UnsupportedByDialectException("This vendor does not support binary JSON data type", currentDialect) | ||
|
||
/** Data type for arrays with no specified size or element type, used only as types of [QueryParameter]s for PostgreSQL and H2. | ||
* An array with no element type cannot be used for storing arrays in a column in either PostgreSQL or H2. */ | ||
abstract fun untypedAndUnsizedArrayType(): String | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's go by the majority here, since only 2 dialects are implemented. Please swap this with the following and remove all exceptions from unsupported dialect files: open fun untypedAndUnsizedArrayType(): String =
throw UnsupportedByDialectException("This vendor does not support array data type", currentDialect) |
||
// Misc. | ||
|
||
/** Returns the SQL representation of the specified expression, for it to be used as a column default value. */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,11 +5,13 @@ import org.jetbrains.exposed.crypt.encryptedBinary | |
import org.jetbrains.exposed.crypt.encryptedVarchar | ||
import org.jetbrains.exposed.dao.id.IntIdTable | ||
import org.jetbrains.exposed.sql.* | ||
import org.jetbrains.exposed.sql.SqlExpressionBuilder.eq | ||
import org.jetbrains.exposed.sql.tests.DatabaseTestsBase | ||
import org.jetbrains.exposed.sql.tests.TestDB | ||
import org.jetbrains.exposed.sql.tests.shared.assertEquals | ||
import org.jetbrains.exposed.sql.tests.shared.entities.EntityTests | ||
import org.junit.Test | ||
import java.math.BigDecimal | ||
import kotlin.test.assertNull | ||
|
||
class SelectTests : DatabaseTestsBase() { | ||
|
@@ -211,6 +213,55 @@ class SelectTests : DatabaseTestsBase() { | |
} | ||
} | ||
|
||
val testDBsSupportingArrays = | ||
listOf(TestDB.POSTGRESQL, TestDB.POSTGRESQLNG, TestDB.H2, TestDB.H2_MYSQL, TestDB.H2_MARIADB, TestDB.H2_PSQL, TestDB.H2_ORACLE, TestDB.H2_SQLSERVER) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
// adapted from `testInList01` | ||
// TODO all the other tests of `inList` can be adapted to test both `inList` and `eqAny` if necessary | ||
fun testEqAny(eqOp: Column<String>.() -> Op<Boolean>) { | ||
withDb(testDBsSupportingArrays) { | ||
withCitiesAndUsers { _, users, _ -> | ||
val r = users.select { users.id.eqOp() }.orderBy(users.name).toList() | ||
|
||
assertEquals(2, r.size) | ||
assertEquals("Alex", r[0][users.name]) | ||
assertEquals("Andrey", r[1][users.name]) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I appreciate that this helps reduce redundancy, especially since you implemented the feature using both The tests that would be great to see are:
|
||
} | ||
} | ||
|
||
fun testEqAny(anyExpression: Expression<String>) = | ||
testEqAny { this eq anyExpression } | ||
|
||
val userIds = arrayOf("andrey", "alex") | ||
|
||
@Test | ||
fun testEqAnyOp() = testEqAny(userIds.anyOp()) | ||
|
||
@Test | ||
fun testEqAnyFunction() = testEqAny(userIds.anyFunction()) | ||
|
||
@Test | ||
fun testEqAny() = testEqAny { this eqAny userIds } | ||
|
||
val amounts = arrayOf(1, 10, 100, 1000).map { it.toBigDecimal() }.toTypedArray() | ||
|
||
fun testGreaterEqAll(allExpression: Expression<BigDecimal>) { | ||
withDb(testDBsSupportingArrays) { | ||
withSales { _, sales -> | ||
val r = sales.select { sales.amount greaterEq allExpression }.toList() | ||
assertEquals(3, r.size) | ||
r.forEach { assertEquals("coffee", it[sales.product]) } | ||
} | ||
} | ||
} | ||
|
||
@Test | ||
fun testGreaterEqAllOp() = testGreaterEqAll(amounts.allOp()) | ||
|
||
@Test | ||
fun testGreaterEqAllFunction() = testGreaterEqAll(amounts.allFunction()) | ||
|
||
@Test | ||
fun testSelectDistinct() { | ||
val tbl = DMLTestsData.Cities | ||
|
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 appreciate the need for this and for the scope of this PR it's ok. It does have some problems, for example:
But we need to flesh out the ArrayColumnType more thoroughly anyway, so I can work on these problems after this PR is merged.
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 didn't remove this because the implemented
Op
s still depend on it. As you commented this will be fleshed out, so to not keep these new definitions in the PR, one way I can think of is to replace its usage with a temporary inlined anonymousColumnType
with itssqlType
being the empty string or just "ARRAY" and the tests still pass.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.
Sure, that sounds like a good temporary option to me.
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, so shall I remove it and adopt this approach in the next commit?