-1

In a recent code review, I was asked to change

Amount > 0

to

ISNULL(Amount,0) > 0

Is there a chance these might right different results? From what I can tell, they would both return false if Amount is null or 0 and true in any other case

To give further details, I want to get all rows in one table, as long as they do not have a corresponding row in a second table with an amount > 0

-- Get members that do not have unpaid fees
Select m.Name from Members m
left join Fees f on m.Id = F.memberId and F.AmountDue > 0
where F.memberId is null
GMB
  • 216,147
  • 25
  • 84
  • 135
Kevin
  • 7,162
  • 11
  • 46
  • 70
  • 1
    Try `NOT (Amount > 0)` to realise that comparison against `NULL`s doesn't return `FALSE`. – Damien_The_Unbeliever Apr 17 '20 at 14:17
  • It is dangerous to make broad statements about snippets of code and a lack of context. But you are incorrect in one way - if amount is NULL the result of the expression is UNKNOWN, not FALSE. Is that significant? Impossible to say without context. – SMor Apr 17 '20 at 14:18
  • 1
    See [Three-valued logic](https://en.wikipedia.org/wiki/Three-valued_logic) which does contain a specific section on SQL – Damien_The_Unbeliever Apr 17 '20 at 14:18
  • 3
    Did you ask the reviewer why you have to change it? – HoneyBadger Apr 17 '20 at 14:19
  • HB makes a good point. A code review is an educational tool as well as a correctness tool. If someone suggests you change something and you don't understand, you should ask questions. And if someone is making a suggestion, that suggestion should be offered with solid reasons why it is an "improvement". – SMor Apr 17 '20 at 14:29
  • Is Fees.Amount a nullable column? If not (and I doubt it should be) the suggestion is pointless. And to be honest, I cannot see why that column would allow anything but a value > 0. – SMor Apr 17 '20 at 14:33
  • @SMor See my update. It is a nullable column (which doesn't have any null values in the actual table) – Kevin Apr 17 '20 at 14:37
  • They might be basing this on something like [code analysis rule SR0007](https://stackoverflow.com/q/7471740/73226) unfortunately that rule is bad advice. – Martin Smith Apr 17 '20 at 14:56
  • If your requirement (as appears in comment) is "do not have unpaid fees", then the condition "AmountDue > 0" will meet that requirement if that is the correct logic. Any value that is null will be ignored in the same manner as any value <= 0. – SMor Apr 17 '20 at 14:56
  • As a rule, injecting _magic values_ is a bad thing. Using `( Amount > 0 or Amount is NULL )` is clear. The problem arises when the _magic value_ starts to have a special meaning, e.g. when `Amount = 0` means "complimentary subscription". Suddenly some innocent code that used that value breaks when the meaning changes from "unlikely value" to "specific peculiar meaning". Then someone decides `-1` means a left-handed subscriber and ... . Aside: Curious that you join on `F.memberId` and then use `where F.memberId is null` to filter the results. – HABO Apr 17 '20 at 16:45
  • @HABO I'm filtering on f.memberId is null because I *don't want* any results with corresponding entries – Kevin Apr 17 '20 at 16:56

2 Answers2

2

amount > 0 checks if amount is stricty greater than 0. If the value is NULL, the condition returns unknown (which is not true, but actually other than false)..

ISNULL(amount, 0) > 0 does the same thing. NULL values are turned to 0, which also fails the check - however in that case false is returned instead of unknown.

Depending on what you want to do with the result of the condition, both conditions may or may not behave identically. If you use that in a where clause for example, there is no notable difference. However the second condition is less efficient, since it requires additional computing, and is likely to not be able to take advantage of an existing index on amount, if any.

GMB
  • 216,147
  • 25
  • 84
  • 135
  • @Kevin: ok. Well, for your query, there is no advantage in using `ISNULL()` (and it makes the code less efficient, as explained). I wouln't use it in that context. – GMB Apr 17 '20 at 14:36
  • Comparing to NULL returns UNKNOWN, not NULL. [A boolean datatype](https://learn.microsoft.com/en-us/sql/t-sql/language-elements/comparison-operators-transact-sql?view=sql-server-ver15) has only 3 possible values: TRUE, FALSE, UNKNOWN – SMor Apr 17 '20 at 15:02
0

Comparing anything to NULL results in UNKNOWN rather than FALSE.

This does essentially have the same effect as returning false in that the query will only return rows where the amount is not null and greater than 0 but you should be careful not to assume that when amount is null the result of the comparison is false

For example this will also fail to return null rows :

WHERE NOT(amount > 0) 

amount > 0 is UNKNOWN not FALSE. If it were false then NOT would turn it true

You could conceive that NULL is a contaminant that makes its way all the up through every Boolean condition and operation it touches, turning them all UNKNOWN and then "becomes false" right at the very last moment because the final assessor of the Boolean predicate demands a TRUE

By and large you can consider UNKNOWN as synonymous with NULL; UNKNOWN is relatively confined to Boolean logic. In other contexts such as mathematical operations involving NULL results in null (0+NULL = NULL). The only operation that can work on null and produce a Boolean is amount IS [NOT] NULL


Per your code review, if they're going to insist on a blanket "must coalesce any value that might be null" It would appear that the business rule is that the system is intolerant of nulls – in this case it should be that the column is made not null and filled only with actual values. Passing any value through a function that may or may not change it cripples the use of indexes and introduces problems for query planning. One could hope that a smart optimizer could expand/rewrite ISNULL(amount, 0) > 0 to amount > 0 or (amount is null and 0>0) before simplifying to remove the bracketed condition entirely but it's not necessarily always that simple. Better not to feed crap in in the first place than shrug and go "well it doesn't seem to make things slower in my simple test case"

Applying a blanket rule without any thought or justification could lead to a very poorly performing system. How you will feed this back to the people who make the rules could be s difficult office politic to handle, because their approach is wrong but they're unlikely to admit it or want to be told


As an aside you might feel that the equivalent query better captures the notion of what you're trying to do:

SELECT m.Name 
FROM Members m
WHERE NOT EXISTS(
  SELECT 1 
  FROM Fees f 
  WHERE m.Id = F.memberId and F.AmountDue > 0
)

The left anti join pattern is a favourite of mine for all sorts but I do feel that this is more self explanatory - "all members that don't have a related record where they owe money". NOT IN also makes this quite readable but should be used with care as some database implementations can be quite naive:

SELECT m.Name 
FROM Members m
WHERE NOT IN (
  SELECT F.MemberId 
  FROM Fees f 
  WHERE F.AmountDue > 0
)

SQL server will highly likely rewrite a LEFT JOIN WHERE NULL/WHERE NOT EXISTS/WHERE NOT IN so they're planned and executed identically, but you might find there isn't much appreciation of this/dim view of the IN approach at the next code review :)

Caius Jard
  • 72,509
  • 5
  • 49
  • 80
  • I added some details/context to question – Kevin Apr 17 '20 at 14:33
  • 1
    Comparing anything to NULL does not return NULL - that is just incorrect no matter how many edits. Comparing to NULL returns UNKNOWN. – SMor Apr 17 '20 at 15:00
  • 1
    @SMor I've always felt that's a matter of semantics rather than an appreciable technical difference but I appreciate we're striving for precision here. In terms of memory load for developers I've always felt it's probably better to keep to a tri state logic they already grok and carry a notion they probably already appreciate (0+null=null) into the realm of comparison, than to introduce a fourth element and discuss UNKNOWN as if it's somehow behaviorally different to NULL in other operations. – Caius Jard Apr 17 '20 at 15:25