sjaveed commented 6 years ago

This brings comparisons against booleans in sync with comparisons against null values

https://www.postgresql.org/docs/9.3/static/functions-comparison.html

For the reasoning behind this, try the following:

Given a User table with one nullable boolean field named "active", create 3 users with the following values for the active field: true, false, nil

Before this pull request, the Equality and NotEqual nodes generated this SQL with the following results:

SELECT COUNT(*) FROM users WHERE users.active = FALSE returns 1 record (active == false only)
SELECT COUNT(*) FROM users WHERE users.active != FALSE returns 1 record (active == true only i.e. active != false but only counting not-nullable columns)

The new approach seems to be more logical but feel free to let me know if it doesn't fit with the arel thinking:

SELECT COUNT(*) FROM users WHERE users.active IS FALSE returns 1 record (active == false only)
SELECT COUNT(*) FROM users WHERE users.active IS NOT FALSE returns 2 records (active == true and active == null which seems to be a better version of active != false)

rails-bot commented 6 years ago

r? @tenderlove

(@rails-bot has picked a reviewer for you, use r? to override)

matthewd commented 6 years ago

Hi, and thanks for the suggestion / patch!

The new approach seems to be more logical

I don't disagree...

but feel free to let me know if it doesn't fit with the arel thinking

Yeah, that. :confused:

To me this has a few (IMO show-stopping) issues:

it changes null-handling behaviour only for booleans -- if "!= false" returns nulls, why shouldn't "!= 3"?
it changes the meaning of existing (and non-obscure) expressions
it deviates from Arel's preference to offer a syntax bridge to the underlying SQL operators without editorialising on how they should work. I'll concede the IS NULL special case is already doing that to some extent.. my counter would be that it's different because = NULL is never true -- we give meaning to an otherwise-useless expression, instead of changing the meaning of something that someone might've intended to use with its defined semantics

I'd argue # 1 even if we were designing the library from scratch; # 2 is obviously just a consequence of existing history, but it's still important. # 3 is more debatable, both because it's a general principle, and because it's not something the existing library always sticks to.

sjaveed commented 6 years ago

@matthewd thank you for your comments! I was concerned about all three of your points before I pushed up this PR but figured it would serve as a provocation if nothing else.

1 is interesting because, at least in PostgreSQL, non-boolean fields don't have operators like `IS TRUE` and `IS NOT TRUE` to deliver that functionality. I'd imagine if the database has that inconsistency then we should reflect that.

2 is definitely a concern since the meaning of equality/non-equality to booleans now returns more results than before which is a breaking change but breaking changes aren't inherently bad - they just need more planning. I'm definitely not trivializing the work this involves but putting it out there.

3 The editorializing bit is interesting because this PR definitely does that in a way that might be more in line with the majority use case but I can't be certain of that.

One thing that's interesting is that for an ActiveRecord query like User.where.not(active: false) the new behavior introduced in this PR can be replicated by User.where(active: true).or(User.where(active: nil)) but or has been notoriously finicky to use. With this PR, you can get the old (current) behavior using User.where(active: false).where.not(active: nil) which is a lot more tolerant.

In any case I really appreciate your comments and, if this is a no-go, would love it if you could recommend an alternative approach that adds to Arel instead of changing the Equality/NotEqual functionality for PostgreSQL and MySQL!

Thanks!

sjaveed commented 6 years ago

@matthewd I've pushed up a new PR that might help alleviate your concerns: #528 - it preserves existing behavior but allows using the new behavior in the specific case where it makes a difference: when IS NOT TRUE or IS NOT FALSE is desired.

I'd love your thoughts on it!

sjaveed commented 6 years ago

Closing since #528 supersedes this now

rails / arel

Boolean equality comparisons now use IS TRUE and IS FALSE for PostgreSQL and MySQL #526

1 is interesting because, at least in PostgreSQL, non-boolean fields don't have operators like `IS TRUE` and `IS NOT TRUE` to deliver that functionality. I'd imagine if the database has that inconsistency then we should reflect that.

2 is definitely a concern since the meaning of equality/non-equality to booleans now returns more results than before which is a breaking change but breaking changes aren't inherently bad - they just need more planning. I'm definitely not trivializing the work this involves but putting it out there.

3 The editorializing bit is interesting because this PR definitely does that in a way that might be more in line with the majority use case but I can't be certain of that.

rails / arel

Boolean equality comparisons now use IS TRUE and IS FALSE for PostgreSQL and MySQL #526

1 is interesting because, at least in PostgreSQL, non-boolean fields don't have operators like IS TRUE and IS NOT TRUE to deliver that functionality. I'd imagine if the database has that inconsistency then we should reflect that.

3 The editorializing bit is interesting because this PR definitely does that in a way that might be more in line with the majority use case but I can't be certain of that.

1 is interesting because, at least in PostgreSQL, non-boolean fields don't have operators like `IS TRUE` and `IS NOT TRUE` to deliver that functionality. I'd imagine if the database has that inconsistency then we should reflect that.