Fix optimization of CASE op WHEN
#33869
Conversation
ranma42
force-pushed
the
fix-case-with-operand
branch
from
d3993f7 to
9e22f89
Compare
The new translation avoids duplicating sub-expressions and introducing negations.
ranma42
force-pushed
the
fix-case-with-operand
branch
from
9e22f89 to
a656e25
Compare
| return _sqlExpressionFactory.Case( | ||
| instance, | ||
| new[] | ||
| { | ||
| new CaseWhenClause( | ||
| _sqlExpressionFactory.Equal(instance, _sqlExpressionFactory.Constant(false)), | ||
| _sqlExpressionFactory.Constant(false), | ||
| _sqlExpressionFactory.Constant(false.ToString())), | ||
| new CaseWhenClause( | ||
| _sqlExpressionFactory.Equal(instance, _sqlExpressionFactory.Constant(true)), | ||
| _sqlExpressionFactory.Constant(true), | ||
| _sqlExpressionFactory.Constant(true.ToString())) |
There was a problem hiding this comment.
this is the same approach recommended by @roji in #33706 (comment) 🚀
There was a problem hiding this comment.
Will let @maumar review/approve but this looks great... I remember this form of CASE/WHEN (with an operand) was introduced a bit later back in the day, I don't think we were aware of it originally; so I'm not surprised we have the less efficient other variant in various places - may be worth doing a pass over the code base for those.
There was a problem hiding this comment.
I will do the full sweep of our code, but that can be done independently of this PR
|
Thanks @ranma42 - great stuff as always! |
| b => b.HasTranslation(args => new CaseExpression( | ||
| operand: args[0], | ||
| [ | ||
| new CaseWhenClause(new SqlConstantExpression(true, typeMapping: BoolTypeMapping.Default), args[1]), |
There was a problem hiding this comment.
@ranma42 while bringing the PG provider up to date, this fails because BoolTypeMapping doesn't work there - its literal representation is 1/0, whereas PG has a true boolean type with TRUE/FALSE as its literals. It's no big deal - I'm overriding the definition to use NpgsqlBoolTypeMapping.
If anything, this shows the shortcomings of our current HasTranslation API; users shouldn't need to manually deal with type mappings like this, etc.
There was a problem hiding this comment.
ouch, sorry; is there a simple way to make it more portable?
There was a problem hiding this comment.
We can always expose some overridable method for providers to construct their boolean type mapping, but honestly it isn't worth it... They can just override the function definition (as I've done).
The
CASE op WHEN ...expression was incorrectly optimized as if it were aCASE WHENexpression if the test expressions contained aTRUEvalue.This also makes it possible to use the
CASE op WHENexpression to avoid duplicating some subexpressions in the translation of the bool to string conversion.Fixes #33867.