feat: Add ALL and ANY operators accepting array, subquery, or table parameters

[ShreckYe]

ALL and ANY are 2 SQL operators to perform a comparison between a single column value and a range of other values.

According to java - PreparedStatement IN clause alternatives? - Stack Overflow, using = ANY(?) and PreparedStatement.setArray if supported can be more efficient than using IN (inList in Exposed) when passing lists of different sizes because a single cached prepared statement can be reused. This is the major motive of this PR.

Since according to references online they are called operators not functions, and CustomOperator in Exposed has 2 operands, I am not sure whether it is better to extend Op or CustomFunction, and so I have implemented both and they are both tested to work. Please help remove the less appropriate one if this PR is accepted.

[e5l]

@bog-walk, could you please check?

[e5l]

Hey @ShreckYe, thanks for the PR. Could you please run apiDump task and commit the changes?

[bog-walk]

Hi @ShreckYe and thanks for working on this PR. These operators will be a great addition to the API, so I've requested some changes to ensure that this implementation is extensible enough to be adapted later.

Using these operators with an array argument is very niched to PostgreSQL, and the standard way of using ANY|ALL is with a subquery on the right hand side (supported by all except SQLite). This will be the expectation and natural progression if these operators are supported.
MySQL also has a very niched use case, accepting a table on the right hand side. We need to be able to add that functionality if users ask for it, especially if we're adding the PostgreSQL array.

The most ideal way to keep your functionality and future use cases possible would be to implement the feature in a way similar to SubQueryOp, but this would mean creating a class and a function for every single comparison operator that can precede ANY|ALL and the DSL would become more verbose.

I think your way of incorporating the existing comparison operators reads better, so I would suggest extending Op and creating a sealed class that allows us to build on it later. I've included this class and how to use it in my comments below.

[bog-walk]

I appreciate the need for this and for the scope of this PR it's ok. It does have some problems, for example:

• If a logger is enabled, the array object value is not processed in the output string, which isn't helpful to users wanting to see the values.
• It won't work with edge cases like if a user wants to check if a datetime column value is in an array of LocalDateTimes.

But we need to flesh out the ArrayColumnType more thoroughly anyway, so I can work on these problems after this PR is merged.

[ShreckYe]

I didn't remove this because the implemented Ops 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 anonymous ColumnType with its sqlType being the empty string or just "ARRAY" and the tests still pass.

[bog-walk]

Sure, that sounds like a good temporary option to me.

[ShreckYe]

OK, so shall I remove it and adopt this approach in the next commit?

[bog-walk]

Please remove this as it isn't necessary for the Op implementation. An arrayParam() will be introduced to the API when the ArrayColumnType is fully supported for storing arrays.

[bog-walk]

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: WHERE { Users.id eq arrayOf(1, 2).anyOp() }.

Please replace them with anyFrom which takes an array argument, that way we can make an override for subqueries and both will read well:

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".

[bog-walk]

This won't be necessary once the functions above are switched to take an argument instead of the more verbose extension function.

[bog-walk]

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 when block and some more subclasses.

[ShreckYe]

I refactored opName into isAny as you suggested but there might still be other row and array comparison operators, for example SOME though it's only an alias of ANY. (see PostgreSQL docs and MySQL docs) If it's necessary to leave room for them (though I haven't find many cases supporting this), this class can be renamed to RowAndArrayComparisonOp as how PostgreSQL calls them and the old opName approach can be adopted.

Also I added a SubSearch type parameter instead of using Any for better type safety and implemented registerArgument in subclasses for better extensibility from my point of view. If this is unnecessary I can change them to what you suggested.

[bog-walk]

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)

[bog-walk]

TestDB.allH2TestDB is available to replace listing all H2 modes.

[bog-walk]

I appreciate that this helps reduce redundancy, especially since you implemented the feature using both Op and Function. After refactoring, please use this (or whatever code you're testing) directly in each unit test instead. That way it will be more clear what the DSL we're testing actually looks like when used.

The tests that would be great to see are:

1. eq anyFrom
2. One other comparison op: the example below is good -> greaterEq anyFrom
3. neq anyFrom
4. eq anyFrom(emptyArray())

[ShreckYe]

@bog-walk thank you for your comments. I have modified the sources as you suggested and requested. For some of them I still have some questions which I leave in their respective reply comments.

[ShreckYe]

The ANY and ALL operators with subquery arguments are not supported by SQLite as is also commented in the commit message. Actually ANY and ALL are not keywords of SQLite at all according to this SO question.

[bog-walk]

@ShreckYe I forgot to mention something. Please take AbstractQuery<*> as the argument so we don't restrict it to just Query. It should be possible to use a SetOperation for example as a subquery in the SQL.

[ShreckYe]

Interestingly, The operators with table arguments are supported by PostgreSQL and H2 as tested though they haven't clearly documented about this, but the tests fail on MySQL who claims supporting it. I think it's probably because it's not supported by its JDBC connector. I verified by copying the SQL statement to an online MySQL executor and it works.

[bog-walk]

Yes, there is currently an issue with the mysql8 docker setup in our tests, so the test failing is a false negative.

[ShreckYe]

OK, so shall I allow the tests to run on MySQL related dialects and just let them fail, or skip the tests as how they are now?

[bog-walk]

@ShreckYe Thanks for all the work so far, it's looking great. I've left some comments and requested a few small edits.

Also, please address the detekt edit requested in the failed action.

[bog-walk]

Sure, that sounds like a good temporary option to me.

[bog-walk]

Yes, there is currently an issue with the mysql8 docker setup in our tests, so the test failing is a false negative.

[bog-walk]

@ShreckYe There's already quite a bit in this PR. Please remove these aliases and then SOME can be introduced in another PR once this one is finalized. It won't take much tweaking with the current setup.

[bog-walk]

A "not" is probably missing from the KDocs here.

[ShreckYe]

@ShreckYe Thanks for all the work so far, it's looking great. I've left some comments and requested a few small edits.

Also, please address the detekt edit requested in the failed action.

Sorry I don't know how to fix the detekt failure. I don't see a blank line at the the location it mentions and my detekt plugin keeps crashing. I downloaded the detekt command line client and ran auto correction but nothing changed. Please help me check if it persists.

[ShreckYe]

OK I got the plugin to work on another computer and ran "AutoCorrect by detekt Rules" which removes Line 236 of the mentioned file.

[e5l]

Hey @ShreckYe, thanks for the PR!
LGTM. Merged