feat/sql null comparisons

[MiguelMJ]

Currently, a predicate checking a null value like myColumn.eq(null) translates to the following SQL:

my_column = null

which is valid SQL, but doesn't actually check if my_column is null.

• I found out that IS [NOT] NULL is defined as a unary operator (1) so I implemented the corresponding UnaryOperator class for them.
• I added a check on the SqlWhereBuilder to translate EQ/NE comparisons with a null value to a IS_NULL/IS_NOT_NULL.
• I added predicates and extensions to make the null checks too.

[alejandropg]

Hi @MiguelMJ

I like your proposal, thanks a lot!

But I don't understand why you need the code in archimedes-data-jdbc/src/main/kotlin/io/archimedesfw/data/sql/criteria/builder/SqlWhereBuilder.kt in the is BinaryPredicate<*> -> {

I mean, If you added the UnaryPredicate, I think you should use it to compare if a column is null with the isNull() instead of the eq(null).

For example the "where" test could be:

    @Test
    internal fun `build with null comparison`() {
        where.predicates.add(tBook.pagesCount.isNull())

instead of your actual code

    @Test
    internal fun `build with null comparison`() {
        where.predicates.add(tBook.pagesCount.eq(null))

So, I would remove all the code:

            is BinaryPredicate<*> -> {
//                val isNullRight = predicate.right is ParameterExpression && predicate.right.value == null
//
//                if (isNullRight && predicate.operator == BinaryOperator.EQ) {
//                    appendPredicate(sb, UnaryPredicate(predicate.left, UnaryOperator.IS_NULL), parameters)
//                } else if (isNullRight && predicate.operator == BinaryOperator.NE) {
//                    appendPredicate(sb, UnaryPredicate(predicate.left, UnaryOperator.IS_NOT_NULL), parameters)
//                } else {
                    appendPredicate(sb, predicate.left, parameters)
                    sb.append(predicate.operator.sql)
                    appendPredicate(sb, predicate.right, parameters)
//                }
            }

What do you think? Why did you add these code in the is BinaryPredicate<*> -> { section? What was your intention?

[MiguelMJ]

Hello!

My reasoning was that when you have a nullable type T? then a dynamic comparison myColumn.eq(myNullableValue) would need that internal conversion for the comparison to work as one would intuitively expect when translated to SQL.

In other words, it acts as a shortcut for if (myNullableValue == null) myColumn.isNull() else myColumn.eq(myNullableValue). However, I understand that this code is maybe what the user should write in these cases. If you think so, I will make the suggested changes and push again.

Thank you for the review!
PD: Excuse the late response, I'm just back from vacation 😅

[alejandropg]

Hi @MiguelMJ

In this case, please, I prefer If you remove that code in the BinaryPredicate case.

Thanks!

[MiguelMJ]

Done ;)