Need to reverse order of null check and equality check when using == with Options

[Rogach]

Version: 3.6.0-RC3, 3.5.2 Module: quill-async-postgres Database: PostgreSQL 13

Expected behavior

I enjoyed the idea with new Scala-idiomatic Option equality a lot, and expected the following to work (this is an excerpt, link to full runnable code below):

case class t(name: Option[String])
import context._

val name: Option[String] = Some("A")
run(query[t].filter(t => t.name == lift(name)))

Actual behavior

PostgreSQL throws an error:

ERROR:  could not determine data type of parameter $1
SELECT t.name FROM t t WHERE t.name IS NULL AND $1 IS NULL OR t.name IS NOT NULL AND $2 IS NOT NULL AND t.name = $3

Steps to reproduce the behavior

Full minimized reproducer project is here: Rogach/quill-issue-2052

createdb test-quill-2052
psql test-quill-2052 -c "create table t(name text)"
git clone https://github.com/Rogach/quill-issue-2052.git
cd quill-issue-2052
# adjust user in Main.scala
sbt run

Workaround

Previously I used "is not distinct from" operator via infix function to achieve the same result:

.filter(t => infix"${t.name} is not distinct from ${lift(name)}".pure.as[Boolean])

By the way, it would probably be nicer if for PostgreSQL option equality compiled into "is distinct from"/"is not distinct from" operators instead of three-parameter "if is null and is null or is not null and is not null".

@getquill/maintainers

[adarshaj]

I've the same issue with an field whose type is UUID and its an Option[UUID] in scala. The suggested workaround works!

[deusaquilus]

Okay, the solution to this is a bit more involved. It requires changes to a bunch of places. Basically, we need to do 2 things:

1. Step 1: Change the way positional parameters work so that lifts that are the same have the same positional parameter name i.e. from this:

   SELECT t.name FROM t t 
     WHERE 
       t.name IS NULL AND $1 IS NULL OR 
       t.name IS NOT NULL AND $2 IS NOT NULL AND 
       t.name = $3
   

   To this:

   SELECT t.name FROM t t 
     WHERE 
       t.name IS NULL AND $1 IS NULL OR 
       t.name IS NOT NULL AND $1 IS NOT NULL AND 
       t.name = $1
   

2. Step 2: Change the ordering of the clauses so that the to-column comparison t.name = $1 will come first. I.e. from this:

   SELECT t.name FROM t t 
     WHERE 
       t.name IS NULL AND $1 IS NULL OR 
       t.name IS NOT NULL AND $1 IS NOT NULL AND 
       t.name = $1
   

   To this:

   SELECT t.name FROM t t 
     WHERE 
       t.name = $1
       t.name IS NULL AND $1 IS NULL OR 
       t.name IS NOT NULL AND $1 IS NOT NULL AND 
   

In order to do Step 1:

1. In Ast.scala. Change ScalarValueLift, ScalarQueryLift, ScalarQueryLiftto have an additional uuid: String parameter (add this to the base ScalarLift trait for convenience). This should be assigned in Parser.scala and ignored in all transformation stages. The copy functions ScalarValueLift, ScalarQueryLift, ScalarQueryLift should be modified to copy-over this parameter. Also the Lifter and Unlifter need to be modified to handle this parameter too.
2. In ReifyStatement.scala, before token2string is called, go through all the ScalarLiftToken (I think you can ignore ScalarTagToken since that's a ProtoQuill thing) instances and take each unique UUID instance in the ScalaLifts and build a UUID->Index map. Take this map and pass it into token2string. You should then use this UUID->Index map instead of doing liftingPlaceholder(liftingSize), in the ScalarLiftToken(lift) case of token2string you should do something like liftingPlaceholder(map(lift.uuid))
3. The above steps should ensure that identical lifts get identical positional parameters. Make sure to actually test. You can use the translate method in PostgresJAsyncContext for this purpose.

In order to do Step 2:

1. In FlattenOptionOperation change: This:

     apply((IsNullCheck(ast) +||+ reduced): Ast)
   

   To this:

     apply((reduced +||+ IsNullCheck(ast)): Ast)
   

   Change This:

     val output: Ast = (IsNotNullCheck(ast) +&&+ expr) +||+ (IsNullCheck(ast) +&&+ alternative)
   

   To this:

     val output: Ast = (expr +&&+ IsNotNullCheck(ast)) +||+ (alternative +&&+ IsNullCheck(ast))
   

   Change This:

     apply((IsNullCheck(ast) +||+ (IsNotNullCheck(ast) +&&+ reduction)): Ast)
   

   To this:

     apply((IsNullCheck(ast) +||+ (reduction +&&+ IsNotNullCheck(ast))): Ast)
   

   Change This:

     apply((IsNotNullCheck(ast) +&&+ reduction): Ast)
   

   To this:

     apply(reduction +&&+ (IsNotNullCheck(ast)): Ast)
   

   Note that doing this will break quite a few tests in the quill-sql suites. You will have to fix them up.

[justcoon]

@deusaquilus i will take this issue

[lachezar]

I am getting similar error in a query that is using lift(opt).forall(_ == ???) in combination with an sql"...".asCondition expression.

The generated query looks like

WHERE (? IS NULL OR ? = t.fkid) AND (t.name ILIKE ? OR ...)

and the error is could not determine data type of parameter $2

The only workaround I have found is to rewrite the query as a single combined sql"..." expression

WHERE (t.fkid = ? OR ?::uuid IS NULL) AND (t.name ILIKE ? OR ...)

I guess the parameter in ? IS NULL is not the same as in ? = t.fkid otherwise Postgres should be able to figure its type 🤔

[matwojcik]

My workaround for the same issue:

.filter{
  case entity =>  
    quote(infix"(${lift(optionalParam).orNull}::varchar IS NULL OR ${entity.field} = ${lift(optionalParam).orNull})".as[Boolean])
}