-
Notifications
You must be signed in to change notification settings - Fork 390
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
Add special treatment for an In node where the right side is [nil, true/false] #528
Conversation
This allows us to generate special SQL for a NotIn node which contains just a two element array that consists solely of nil and either true or false. Translated to ActiveRecord, this means that User.where.not(active: [nil, true]) will generate: SELECT * FROM "users" WHERE "users"."active" IS NOT TRUE; and User.where.not(active: [nil, false]) will generate: SELECT * FROM "users" WHERE "users"."active" IS NOT FALSE;
(@rails-bot has picked a reviewer for you, use r? to override) |
Hah I was trying a couple different approaches - looks like I left the wrong one in. Fixed! |
@rafaelfranca any thoughts on this? |
Will not this now return records which values are not This make sense to boolean columns but the code you implemented will also generate that SQL for string columns that will include more than just the records with |
I think I just don't see a benefit to special-casing this... it makes the SQL a bit shorter, but as @rafaelfranca points out, there are cases where it could introduce an ambiguity. And in the ones it doesn't, the SQL engine will treat both spellings identically. |
The idea is that for boolean columns it will return values which are NULL or TRUE only
That makes sense and is not the desired result at all. I'll see if it's possible to ensure this new behavior only for boolean columns.
|
I think the biggest benefit here is that it provides a way to generate IS TRUE / IS FALSE syntax without breaking backwards compatibility. It also removes an OR from the generated SQL which seems to always cause subtle problems. If those have any value for boolean columns, I'll work to ensure this behavior is only triggered for boolean columns if possible, thus removing any issues with string columns. If those benefits aren't valuable, however, I'm willing to drop this. Could I get a consensus on whether or not to continue? Thanks!
|
As we've established, though, it just breaks different backwards compatibility. I'm dubious about the value of the special-use Avoiding OR is uninteresting to me: it's generated, so any paren-placement confusion is a bug. As a general observation, we don't try to second-guess the SQL engine to "simplify" the SQL: it'll do a much better job of that for itself. |
Not sure it breaks any backwards compatibility if it's restricted to boolean columns only. However, I'll close this PR as a no-go and see what I can do about adding a node for IS DISTINCT FROM |
This allows us to generate special SQL for a NotIn node which contains
just a two element array that consists solely of nil and either true
or false.
Translated to ActiveRecord, this means that
will generate:
instead of the old:
and
will generate: