Skip to content
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

SQL Server Boolean Literals Incorrectly Translated #1685

Closed
deusaquilus opened this issue Oct 31, 2019 · 1 comment · Fixed by #1923
Closed

SQL Server Boolean Literals Incorrectly Translated #1685

deusaquilus opened this issue Oct 31, 2019 · 1 comment · Fixed by #1923
Assignees
Labels
documented-solution quats-feature Features that are now possible to implement using Quill-Application-Types (Quats)

Comments

@deusaquilus
Copy link
Collaborator

Version: (e.g. 3.4.8)
Module: (e.g. quill-sql)
Database: (e.g. SQL Server)

Using literal false inside of select clause causes 1=0 to be expressed. Needs to be 0. The 1=0 needs to be detected as a tail entity and removed.

Or else this happens:

val ctx = new SqlMirrorContext(SQLServerDialect, SnakeCase)
case class Something(id:Int, value:Boolean)
run(query[Something].map(s => (s.id, s.value, false)))
// SELECT s.id, s.value, 1=0 FROM something s
// This is invalid because you can't have 1=0 inside of a select-value clause

By "tail entity" I mean the following:
You can translate if (false) true else false
Into CASE WHERE 1=0 THEN 1 ELSE 0 END
But not into: CASE WHERE 1=0 THEN 1=1 ELSE 1=0 END

However, this kind of logic needs to be done recursively
e.g. if you have: if ( if (false) true else false ) true else false
Needs to become: case when (CASE WHEN (1=0) THEN 1 ELSE 0 END) = 1 then 1 else 0 END
Notice how we need to add that =1 at the end of the inner case.

In where clauses, false needs to become 1=0:
select 1 where 1=0
but not in all cases e.g:
select 1 where (CASE WHEN 1=1 THEN 1 ELSE 0 END) = 1

As far as I understand the rule is:
When it's inside of a condition (e.g. CASE, WHERE) then translate false to 1=0 (and true 1=1) unless a comparison is being made (i.e. you are returning from a inner CASE) at which point you add a =1.

Workaround

@getquill/maintainers

@deusaquilus
Copy link
Collaborator Author

deusaquilus commented Jul 22, 2020

Now since Quats are merged, we should use them to solve this problem, a sub-type of Quat.Value should be introduced called Quat.Boolean which in turn has two sub-types: Quat.BooleanValue and Quat.BooleanExpression, here's why. In dialects that do not have true and false (e.g. SQL Server, Oracle, etc...), things inside where clauses and case statements that determine trueness are expressions e.g. in

case when 1=1 then 'foo' else 'bar'

the 1=1 is a expression.

On the other hand, when a value is actually selected that's a boolean value e.g. in

case when blah then 1 else 0`

the 1 and 0 are values.

Now for databases like postgres these things can be used interchanbeability e.g.

case when true then true=true else true=false

So, the 1st 'slot' of a case statement is a BooleanExpression, the second and third are BooleanValues. Similarly, in a Ast.Filter clause, the Ast that goes in the Body must always be a Quat.BooleanExpression.

Additionally, in a BinaryOperation where Operator is BooleanOperator (i.e. ! or && or || the quat of the BinaryOperation AST element should always be Quat.BooleanExpression. On the other hand, constants true/false or an scala boolean Ident should be a Quat.BooleanValue (this should be a simple addition to QuatMaking.scala).

Putting this knowledge together, we can then introduce a AST transformation phase called "VendorizeBooleans" (please feel free propose a different name if you come up with something better) which will perform the following conversions for databases that do not have true/false).

Additionally, Constant will now need an actual Quat variable to be passed in by QuatMaking (like Ident, Infix and some others) as opposed to always being a Quat.Value.

Rule 1
For any Slot Requiring a BooleanExpression
 * If the Quat in the Slot is already a BooleanExpression, you're done.
 * If the Quat in the Slot is a BooleanValue `bv`, change it to `bv = true` which will correctly be a BooleanExpression (Note how it will eventually be `bv = 1` but that comes later).
 * If any other kind of Quat is in the slot ignore it, let the SQL interpreter deal with potential issues

Rule 2
For any Slot Requiring a BooleanValue
 * If the Quat in the Slot is already a BooleanValue, you're done.
 * If the Quat in the Slot is a BooleanExpression `be` convert it to `Ast.If(be, true, false)` (Note how eventually it will become `Ast.If(be, 1, 0)` but that comes later.
 * If any other kind of Quat is in the slot ignore it, let the SQL interpreter deal with potential issues

Apply these rules recursively
Then change all true/false constants in the AST to 1/0 respectively.

(Some work in encoders might be needed but I don't think so)

Here's how these rules exist, say that you have a crazy expression that looks like this:

case x then ('foo' == 'bar') == true else false -- where 'x' is a Ident whose Quat is 
-- ===== First Apply Rule 1 =====
case x == true then ('foo' == 'bar') == true else false
--Think of it like adding a ==1 to x: case x == 1 then ('foo' == 'bar') == true else false

-- ===== Then Apply Rule 2 - 1st Recursion (depth-first) =====
case x == true then (if ('foo' == 'bar') then true else false) == true else false 
--Think of it like adding then 1 else 0 to the inner clause: 
--case x == 1 then (if ('foo' == 'bar') then 1 else 0) == true else false

-- ===== Then Apply Rule 2 - 2nd Recursion (depth-first) =====
case x == true then (if((if ('foo' == 'bar') then true else false) == true) then true else false) else false  
--Think of it like adding then 1 else 0 to the outer clause: 
--case x == 1 then (if((if ('foo' == 'bar') 1 true else 0) == 1) then 1 else 0) else false

-- Finally, change true/false to 1/0 respectively
case x == 1 then (if((if ('foo' == 'bar') then 1 else 0) == 1) then 1 else 0) else 1

StatelessTransformer ideal for these kinds of recursions e.g. here is a simple implementation of Rule1 for Ast.If assuming you are using a StatelessTransformer.

def apply(ast: Ast) =
  ast match {
    case If(cond, then, else) =>
      If(expressifyValue(apply(cond)), valuefyExpression(apply(then)), valuefyExpression(apply(else)))
  }

// The action taken to apply Rule1
def expressifyValue(bv: Quat.Boolean) =
  bv match {
    case v: Quat.BooleanValue => v +==+ true // The method +==+ is from QuatOps, it just creates a BinaryExpression
    case e: Quat.BooleanExpression => e
    case other => other
  }

// The action taken to apply Rule2
def valuefyExpression(be: Quat.Boolean) =
  be match {
    case e: Quat.BooleanExpression => e
    case v: Quat.BooleanValue => If(v, Constant(true, Quat.BooleanValue), Constant(false, Quat.BooleanValue))
    case other => other
  }

Also note that BinaryOperation.quat now will depend on the operator

case class BinaryOperation(a: Ast, operator: BinaryOperator, b: Ast) extends Operation { 
  import BooleanOperator._

  def quat =
    operator match {
      case `!` | `&&` | `||` => Quat.BinaryExpression
      case _ => Quat.Value
    }
}

Also note that in valueParser you're going to need to add a inferQuat clause for Constant

val valueParser: Parser[Ast] = Parser[Ast] {
  case l @ Literal(c.universe.Constant(v)) => Constant(v, l.tpe) // or maybe it's l.tpe.typeSymbol.typeSignature, you'll have to try them both
}

Additionally, remember to add Liftable/Unliftable[Quat.BooleanValue], Liftable/Unliftable[Quat.BooleanExpression], Liftable/Unliftable[Quat.Boolean], and a clause for Quat.Boolean in Liftable/Unliftable[Quat] (you need all of them since Liftables/Unliftables are invariant).

@deusaquilus deusaquilus added quats-feature Features that are now possible to implement using Quill-Application-Types (Quats) documented-solution labels Jul 22, 2020
@juliano juliano self-assigned this Jul 28, 2020
@juliano juliano mentioned this issue Aug 3, 2020
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documented-solution quats-feature Features that are now possible to implement using Quill-Application-Types (Quats)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants