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 <=) #39613

Merged
merged 1 commit into from Jul 6, 2020

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Jun 13, 2020

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("create_at >=": time) is better alternative than where("create_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

posts = Post.order(:id)
posts.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')
posts.where("created_at >= ?", time) # => []

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

@kamipo kamipo force-pushed the where_with_custom_operator branch 2 times, most recently from c37b0f7 to 9f87149 Compare Jun 16, 2020
@kamipo kamipo force-pushed the where_with_custom_operator branch from 9f87149 to aadc9fa Compare Jun 23, 2020
activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
@bogdan
Copy link
Contributor

bogdan commented Jun 23, 2020

Will != be supported? I know about where.not but it can be more readable to use != when it is mixed with other conditions.

@sedubois
Copy link
Contributor

sedubois commented Jun 23, 2020

Could such an approach be extended to JSON operators?

posts.find_by("slugs ?": slug)

@kamipo
Copy link
Member Author

kamipo commented Jun 23, 2020

Will != be supported? I know about where.not but it can be more readable to use != when it is mixed with other conditions.

Could such an approach be extended to JSON operators?

posts.find_by("slugs ?": slug)

It is not a plan for this PR. I'd not like to expand the scope in this PR.

@kamipo kamipo force-pushed the where_with_custom_operator branch 2 times, most recently from 343c51f to ba397bd Compare Jun 24, 2020
@simi
Copy link
Contributor

simi commented Jun 24, 2020

What about to make this extendable (as a follow-up PR I can work on) to make possible to introduce custom operators?

@kamipo
Copy link
Member Author

kamipo commented Jun 24, 2020

Seems it is fine, but not a plan for this PR.

@simi
Copy link
Contributor

simi commented Jun 24, 2020

Providing wrong operator results into wrong SQL constructed (unknown column).

irb(main):003:0> User.where('id ?': 1)
Traceback (most recent call last):
ActiveRecord::StatementInvalid (PG::UndefinedColumn: ERROR:  column users.id ? does not exist)
LINE 1: SELECT "users".* FROM "users" WHERE "users"."id ?" = $1 LIMI...
                                            ^
HINT:  Perhaps you meant to reference the column "users.id".

What about to provide custom exception "UnknownOperator" for that? Exception message can also name all supported operators.

@kamipo
Copy link
Member Author

kamipo commented Jun 24, 2020

Even if where("id ?", 1) as old syntax, it also raise StatementInvalid.

At least I'm not excited extra work in this PR to make slower things for such like unsupported usage.

@simi
Copy link
Contributor

simi commented Jun 24, 2020

🤔 But where("id ?", 1) is not the symbol, but string syntax. When passing symbol AR threats it as column name. With this nice change it threats it as column and (optional) operator. We can check if any operator is present and if that one is supported. I don't think it will slow things down. Anyway again, I can take a look once this is merged.

🔍 Looking into conflicting syntax I have found out actually id > is valid column name for PostgreSQL (I have not checked other DBs).

CREATE TABLE test ("id" int, "id >" int);

Having this table and using Test.where('id >': 1) would result (AFAIK) into "id" > 1, but actually both variants "id >" = 1 and "id" > 1 would be valid in here.

Anyway it is really edge case.

@kamipo
Copy link
Member Author

kamipo commented Jun 24, 2020

I know "id >" is valid column name.

If we should care about that, we cannot take this idea.

At least currently all valid column names does not always perfectly works in Active Record (e.g. non primary "id" column, "." include column name, etc).

@simi
Copy link
Contributor

simi commented Jun 24, 2020

Cool, I was about to mention it at least for future reference. Definitely edge case Rails should not care about.

simi
simi approved these changes Jun 24, 2020
Copy link
Contributor

@simi simi left a comment

I did some local testing, all looks good. ❤️

I think some operators could be added (<>, !=, =), but as mentioned it will not be done as part of this case. I can try to open some follow-ups later.

@kamipo kamipo changed the title Support where with custom operators (>, >=, <, and <=) Support where with comparison operators (>, >=, <, and <=) Jun 24, 2020
@smridge
Copy link

smridge commented Jun 25, 2020

Ruby beginless/endless ranges can also be used as an out of the box solution:

Post.where(id: 1..)
=>   Post Load (0.4ms)  SELECT "posts".* FROM "posts" WHERE "posts"."id" >= $1  [["id", 1]]

Post.where(id: ..9)
=>   Post Load (0.3ms)  SELECT "posts".* FROM "posts" WHERE "posts"."id" <= $1  [["id", 9]]

Post.where(id: ...9)
=>   Post Load (0.3ms)  SELECT "posts".* FROM "posts" WHERE "posts"."id" < $1  [["id", 9]]

```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("create_at >=": time)` is better alternative than `where("create_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">]
```
@kamipo kamipo force-pushed the where_with_custom_operator branch from 6fba4ec to 6d6ec6f Compare Jul 6, 2020
@kamipo kamipo merged commit b806450 into rails:master Jul 6, 2020
2 checks passed
@kamipo kamipo deleted the where_with_custom_operator branch Jul 6, 2020
@simi simi mentioned this pull request Jul 14, 2020
@tenderlove
Copy link
Member

tenderlove commented Jul 16, 2020

This is an interesting change, but since this changes public API I think we need to have a discussion with the rest of the core team before merging. Bugfixes and performance improvements are fine for merging without discussion, but this type of change needs a bit more scrutiny by the rest of the core team. Can we please revert this and discuss more before merging?

@kamipo
Copy link
Member Author

kamipo commented Jul 17, 2020

Sure, I will be going to revert this.

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, let me explain in a later PR).

Hopefully the hash syntax would be approved from the core team...

kamipo added a commit to kamipo/rails that referenced this issue Jul 17, 2020
…operator"

This reverts commit b806450, reversing
changes made to 8714b35.

Reason: This is not approved from the core team yet...
kamipo added a commit that referenced this issue Jul 17, 2020
…erators

Revert "Merge pull request #39613 from kamipo/where_with_custom_operator"
kamipo added a commit to kamipo/rails that referenced this issue Jul 17, 2020
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 added a commit to kamipo/rails that referenced this issue Jan 12, 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 added a commit to kamipo/rails that referenced this issue 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 added a commit to kamipo/rails that referenced this issue Mar 11, 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
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

7 participants