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

Equality should handle NOT values with != () or NOT LIKE () #165

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@avit
Copy link

avit commented Mar 1, 2013

This fixes SQL syntax when composing Not nodes into binary comparison nodes and cancels double-negatives:

  • .eq(not_x) "!= x"
  • .not_eq(not_x) "= x"
  • .matches(not_x) "NOT LIKE x"
  • .does_not_match(not_x) "LIKE x"

Use case: in a Ransack search I want to use eq or not_eq to include "y != 'chunky' OR y IS NULL". (Simple != will skip NULL rows.) This currently does not work:

attr.eq_any( [Nodes::Not('chunky'), nil] )
@ernie

This comment has been minimized.

Copy link
Collaborator

ernie commented Jun 3, 2013

@avit I don't think making not_eq send back different node types is the answer. Do you think this could be handled in the visitor, instead?

@avit

This comment has been minimized.

Copy link
Author

avit commented Jun 3, 2013

@ernie Sure, I could have another look at doing it that way. It seems like we could accomplish the same thing fairly easily by unpacking Not nodes in visit_Arel_Nodes_Equality & friends.

@avit

This comment has been minimized.

Copy link
Author

avit commented Jun 3, 2013

Some of the other predication methods do the same thing, e.g. .in and .not_in also return different node types based on the right-hand value. Is the reasoning for this logic going into the visitor different from those?

I suppose someone could manually build a tree with Not(Not(...)) and then the visitor would get it wrong?

@ernie

This comment has been minimized.

Copy link
Collaborator

ernie commented Jun 4, 2013

@avit Fair point re: in and not_in. I think that was mostly a concession to the fact that Arel is really focused on generating SQL, and since SQL has different syntax for comparison of a begin and end vs a list of values, it was deemed appropriate to handle in this way. I'm not entirely convinced this wouldn't be cleaner at the visitor level, too, but for purposes of demonstrating consistency with the existing code, you win. ;)

Equality should handle NOT values with != () or NOT LIKE ()
Fix SQL syntax and cancel double-negatives:

* .eq(not_x)       '!= x'
* .not_eq(not_x)   '= x'
* .like(not_x)     'NOT LIKE x'
* .not_like(not_x) 'LIKE x'

(Issue #165)
@avit

This comment has been minimized.

Copy link
Author

avit commented Jun 5, 2013

@ernie, please review changes, I moved the logic into the visitor. Works the same.

@bf4

This comment has been minimized.

Copy link

bf4 commented Sep 19, 2013

bump

@ernie

This comment has been minimized.

Copy link
Collaborator

ernie commented Sep 19, 2013

@avit I'm not sure how I missed this somewhat odd use case in the original description. I'm not sure a Ransack requirement should change the behavior of Not in this way. Reasoning is that back in the Arel 1.x days I made a similar change but we didn't bother porting it for 2.x.

@avit

This comment has been minimized.

Copy link
Author

avit commented Sep 19, 2013

@ernie I didn't request this strictly for Ransack: I would argue that handling double-negative algebra almost certainly belongs here in Arel rather than working around it downstream.

I pointed out the "somewhat odd use case" as one example. The convoluted SQL syntax that comes out when composing & negating ActiveRecord scopes is sometimes valid ((NOT(status != 'active'))), but often it outputs broken SQL syntax even though the Arel AST should be workable. It would be nice if it worked consistently; and the output would read more simply too.

This patch is working fine for us in our application. Are there any real objections for just fixing this here? In that case let's just close this...

@ernie

This comment has been minimized.

Copy link
Collaborator

ernie commented Sep 19, 2013

@avit one of the guiding principles of Arel has been that it shouldn't police your SQL for correctness, so I wouldn't say that potential output of bad SQL (if what you built in the AST would map to that bad SQL) is an issue, necessarily.

I don't want to close this without giving @tenderlove a chance to weigh in, though, in case he disagrees.

@bf4

This comment has been minimized.

Copy link

bf4 commented Sep 19, 2013

@ernie Certainly there's at least something salvageable in this PR. I'm not a maintainer, but it seems an improvement re: principle of least surprise.

@tenderlove

This comment has been minimized.

Copy link
Member

tenderlove commented Sep 19, 2013

I think Ernie is correct. Arel has "pivoted" from a relational algebra library to a library for building SQL asts and statements. It shouldn't prevent you from building wrong statements.

Aaron Patterson
http://tenderlovemaking.com/
I'm on an iPhone so I apologize for top posting.

On Sep 19, 2013, at 5:29 AM, Ernie Miller notifications@github.com wrote:

@avit one of the guiding principles of Arel has been that it shouldn't police your SQL for correctness, so I wouldn't say that potential output of bad SQL (if what you built in the AST would map to that bad SQL) is an issue, necessarily.

I don't want to close this without giving @tenderlove a chance to weigh in, though, in case he disagrees.


Reply to this email directly or view it on GitHub.

@avit

This comment has been minimized.

Copy link
Author

avit commented Sep 20, 2013

sad :panda:

This question is ultimately about how to rely on Arel to build the AST... I guess the question is whether Arel should know some semantics about its own nodes, or whether the downstream application needs to sniff node types to recombine the AST correctly. Consider:

scope :matching_selected, ->(other_nodes) { where my_attr.matches(other_nodes) }

If we want to negate the other_nodes, right now we can't just pass in other_nodes.not safely without checking its class and unpacking it:

scope :matching_selected, ->(other_nodes) {
  if Arel::Nodes::Not === other_nodes
    where my_attr.does_not_match(other_nodes.value)
  else
    where my_attr.matches(other_nodes)
  end
}

My personal opinion is that Arel should do it.

@tamird

This comment has been minimized.

Copy link

tamird commented Sep 16, 2014

looks like this was decided against. should we close it?

@matthewd matthewd closed this Sep 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment