Skip to content
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

Support where with comparison operators (>, >=, <, and <=) Take 2 #39863

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Jul 17, 2020

Revert "Revert "Merge pull request #39613 from kamipo/where_with_custom_operator""

This reverts commit da02291.

posts = Post.order(:id)

posts.where("id >": 9).pluck(:id)  # => [10, 11]
posts.where("id >=": 9).pluck(:id) # => [9, 10, 11]
posts.where("id <": 3).pluck(:id)  # => [1, 2]
posts.where("id <=": 3).pluck(:id) # => [1, 2, 3]

From type casting and table/column name resolution's point of view,
where("created_at >=": time) is better alternative than where("created_at >= ?", time).

class Post < ActiveRecord::Base
  attribute :created_at, :datetime, precision: 3
end

time = Time.now.utc # => 2020-06-24 10:11:12.123456 UTC

Post.create!(created_at: time) # => #<Post id: 1, created_at: "2020-06-24 10:11:12.123000">

# SELECT `posts`.* FROM `posts` WHERE (created_at >= '2020-06-24 10:11:12.123456')
Post.where("created_at >= ?", time) # => []

# SELECT `posts`.* FROM `posts` WHERE `posts`.`created_at` >= '2020-06-24 10:11:12.123000'
Post.where("created_at >=": time) # => [#<Post id: 1, created_at: "2020-06-24 10:11:12.123000">]

As a main contributor of the predicate builder area, I'd recommend to
people use the hash syntax, the hash syntax also have other useful
effects (making boundable queries, unscopeable queries, hash-like
relation merging friendly, automatic other table references detection).

  • Making boundable queries

While working on #23461, I realized that Active Record doesn't generate
boundable queries perfectly, so I've been improving generated queries to
be boundable for a long time.

e.g.

#26117
7d53993
#39219

Now, where with the hash syntax will generate boundable queries
perfectly.

I also want to generate boundable queries with a comparison operator in
a third party gem, but currently there is no other way than calling
predicate_builder directly.

kufu/activerecord-bitemporal#62

  • Unscopeable queries, Hash-like relation merging friendly

Unscopeable, and Hash-like merging friendly queries are relying on where
clause is an array of attr with value, and attr name is normalized as a
string (i.e. using User.arel_table[:name] is not preferable for
unscope and merge).

Example:

id = User.arel_table[:id]

users = User.where(id.gt(1).and(id.lteq(10)))

# no-op due to `id.gt(1).and(id.lteq(10))` is not an attr with value
users.unscope(:id)
  • Automatic other table references detection

It works only for the hash syntax.

ee7f666

@pirj
Copy link

pirj commented Jul 18, 2020

Won't the following provide roughly the same functionality while being less intrusive?

module WhereComparison
  refine Symbol do
    def >(value)
      return "#{self} > ?", value
    end
  end
end
include WhereComparison
posts = Post.order(:id)

posts.where(:id > 9).count #   (5.2ms)  SELECT COUNT(*) FROM `posts` WHERE (id > 9)

@rails rails locked and limited conversation to collaborators Jul 18, 2020
@rails rails unlocked this conversation Jul 18, 2020
@eugeneius
Copy link
Member

eugeneius commented Jul 18, 2020

That refinement provides a roughly-equivalent syntax, but still generates an SQL fragment, which loses the other benefits:

boundable queries, unscopeable queries, hash-like relation merging friendly, automatic other table references detection.

Those benefits are the point of this change, rather than the particular user-facing API. If you don't care about those things, where("id > ?", 9) is already good enough.

This pull request was opened to solicit feedback from the core team (see #39613 (comment)); let's wait for that feedback. 🙂

@pirj
Copy link

pirj commented Jul 19, 2020

Sure, I understand.
Just a note that the monkey-patched Symbol doesn't necessarily have to return a String with SQL.
It can equally return a lambda, that would call ->(model_or_relation) { model_or_relation.arel_table[:id].gt(9) }, or an object.

kamipo added a commit to kamipo/rails that referenced this pull request Jul 19, 2020
In Active Record internal, `arel_table` is not directly used but
`arel_attribute` is used, since `arel_table` doesn't normalize an
attribute name as a string, and doesn't resolve attribute aliases.

For the above reason, `arel_attribute` should be used rather than
`arel_table`, but most people directly use `arel_table`, both
`arel_table` and `arel_attribute` are private API though.

Although I'd not recommend using private API, `arel_table` is actually
widely used, and it is also problematic for unscopeable queries and
hash-like relation merging friendly, as I explained at rails#39863.

To resolve the issue, this change moves Arel attribute normalization
(attribute name as a string, and attribute alias resolution) into
`arel_table`.
@inopinatus
Copy link
Contributor

inopinatus commented Sep 4, 2020

I've been using the isomorphism of intervals and inequalities for some time, i.e. passing a range to the predicate builder:

posts.where(id: 9..).ids # => [9, 10, 11]
posts.where(id: ...3).ids  # => [1, 2]
posts.where(id: ..3).ids # => [1, 2, 3]

The only missing inequality is greater-than (>), I think that is an omission from Ruby itself that could be fixed.

Edit/addendum: greater-than is the logical inverse of less-than-or-equal. So in a pinch, one may already write posts.where.not(id: ..9).ids # => [10,11]. This still correctly omits rows with nulls, if present. Unfortunately, this inverted form is harder to instantly comprehend, and that "pass around any interval-equivalent range" narrative remains just out of reach.

@raghubetina
Copy link

raghubetina commented Nov 18, 2020

@inopinatus I think > is missing because the beginning of a Range is always included in it; it's only the ending that is included or excluded. So 9.. (i.e. 9..Float::INFINITY) is the same, technically, as 9... (i.e. 9...Float::INFINITY); they both include 9 and fail to exclude infinity.

Still, I also was surprised by the missing > when I tried an implicit endless exclusive range with where; so I wrote up #40628 to add it. It doesn't require changes to Ruby itself.

@marian13
Copy link

marian13 commented Dec 22, 2020

Hi @kamipo.

I have used your suggestion of where with comparison operators for my answers to some old Stack Overflow questions about SQL comparison in Rails (here and here) and it looks like folks like this idea.

Could you tell me, please, do you have any plans to continue its development?

Thanks in advance.

@kamipo kamipo force-pushed the where_with_comparison_operator branch 2 times, most recently from 72bbb89 to 0f5f596 Compare Jan 14, 2021
Base automatically changed from master to main Jan 14, 2021
Revert "Revert "Merge pull request rails#39613 from kamipo/where_with_custom_operator""

This reverts commit da02291.

```ruby
posts = Post.order(:id)

posts.where("id >": 9).pluck(:id)  # => [10, 11]
posts.where("id >=": 9).pluck(:id) # => [9, 10, 11]
posts.where("id <": 3).pluck(:id)  # => [1, 2]
posts.where("id <=": 3).pluck(:id) # => [1, 2, 3]
```

From type casting and table/column name resolution's point of view,
`where("created_at >=": time)` is better alternative than `where("created_at >= ?", time)`.

```ruby
class Post < ActiveRecord::Base
  attribute :created_at, :datetime, precision: 3
end

time = Time.now.utc # => 2020-06-24 10:11:12.123456 UTC

Post.create!(created_at: time) # => #<Post id: 1, created_at: "2020-06-24 10:11:12.123000">

# SELECT `posts`.* FROM `posts` WHERE (created_at >= '2020-06-24 10:11:12.123456')
Post.where("created_at >= ?", time) # => []

# SELECT `posts`.* FROM `posts` WHERE `posts`.`created_at` >= '2020-06-24 10:11:12.123000'
Post.where("created_at >=": time) # => [#<Post id: 1, created_at: "2020-06-24 10:11:12.123000">]
```

As a main contributor of the predicate builder area, I'd recommend to
people use the hash syntax, the hash syntax also have other useful
effects (making boundable queries, unscopeable queries, hash-like
relation merging friendly, automatic other table references detection).

* Making boundable queries

While working on rails#23461, I realized that Active Record doesn't generate
boundable queries perfectly, so I've been improving generated queries to
be boundable for a long time.

e.g.

rails#26117
7d53993
rails#39219

Now, `where` with the hash syntax will generate boundable queries
perfectly.

I also want to generate boundable queries with a comparison operator in
a third party gem, but currently there is no other way than calling
`predicate_builder` directly.

kufu/activerecord-bitemporal#62

* Unscopeable queries, Hash-like relation merging friendly

Unscopeable, and Hash-like merging friendly queries are relying on where
clause is an array of attr with value, and attr name is normalized as a
string (i.e. using `User.arel_table[:name]` is not preferable for
`unscope` and `merge`).

Example:

```ruby
id = User.arel_table[:id]

users = User.where(id.gt(1).and(id.lteq(10)))

# no-op due to `id.gt(1).and(id.lteq(10))` is not an attr with value
users.unscope(:id)
```

* Automatic other table references detection

It works only for the hash syntax.

ee7f666
@kamipo kamipo force-pushed the where_with_comparison_operator branch from 0f5f596 to 8827baa Compare Mar 11, 2021
@jon-sully
Copy link

jon-sully commented Apr 2, 2021

Hoping to not add unneeded noise here but would love to see this go in 👍🏻

For others in the meantime, especially those that want this because (I believe) it allows the query fragment to adhere to the join-table-aliasing introduced in #40106, I'll be using the hash syntax by logically flipping the filter and using an infinite range.

E.g. where (with this PR) I would use

Model.where('created_at <=': Date.today)

I'll flip the where, use an infinite date starting at tomorrow, and use the hash syntax:

Model.where.not(created_at: Date.tomorrow..)

Which does adhere to the join table aliasing (calling out your Model.arel_table[:column] would break it) 👍🏻

@p8
Copy link
Member

p8 commented Apr 23, 2021

What about:

Post.where(updated_at: Arel.gt(1.day.ago))
Post.where(author: { name: Arel.like("Ryuta %") })
Post.where(author: { id: Arel.not(42) })

@rails-bot
Copy link

rails-bot bot commented Jul 22, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Jul 22, 2021
@jon-sully
Copy link

jon-sully commented Jul 22, 2021

Would still love to see this go in 👍🏻

@rails-bot rails-bot bot removed the stale label Jul 22, 2021
@rails-bot
Copy link

rails-bot bot commented Oct 20, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Oct 20, 2021
@kamipo kamipo removed the stale label Oct 26, 2021
@rails-bot
Copy link

rails-bot bot commented Jan 24, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Jan 24, 2022
@rails-bot rails-bot bot closed this Jan 31, 2022
@jon-sully
Copy link

jon-sully commented Feb 26, 2022

Hoping this can be reopened and re-looked at. The value of having inequality operators with the hash syntax that respects table aliases is worth a serious look IMO. 🙏🏻

@kamipo kamipo reopened this Feb 26, 2022
@rails-bot rails-bot bot removed the stale label Feb 26, 2022
@jon-sully
Copy link

jon-sully commented Mar 3, 2022

I've obviously thrown my support behind this above but I had to write this not + 'backwards range' again today and hope this PR can get a review/consensus in time 🙂

UserDeal.joins(:deal).where.not(deal: { closes_on: Date.tomorrow... })

While the syntax (stringed-symbol) isn't the cleanest, the quick understandability of this alternative feels really valuable

UserDeal.joins(:deal).where(deal: { "closes_on <=": Date.today })

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

Successfully merging this pull request may close these issues.

None yet

8 participants