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

[5.7] Implemented having between #26758

Merged
merged 4 commits into from
Dec 6, 2018
Merged

[5.7] Implemented having between #26758

merged 4 commits into from
Dec 6, 2018

Conversation

BrokenSt0rm
Copy link
Contributor

In the past few weeks we needed to do an having between at work.
We found out that Laravel only supports this type of having with a raw clause, so I'm submitting this PR to help anyone that have our needs and do not want to write a raw clause.
It does not affect the way havings are built so it's a non breaking change.
I have included some basic tests.

@BrokenSt0rm BrokenSt0rm changed the title Implemented having between [5.7] Implemented having between Dec 5, 2018
@mfn
Copy link
Contributor

mfn commented Dec 6, 2018

On which databases did you test this?

Are we sure it works on all supported databases (mssql,mysql,pgsql,sqlite) that way?

@BrokenSt0rm
Copy link
Contributor Author

BrokenSt0rm commented Dec 6, 2018

The having clause with the between operator is valid on all databases that Laravel supports.
I have tested this syntax on all of them.

@@ -185,6 +185,7 @@ class Builder
'rlike', 'regexp', 'not regexp',
'~', '~*', '!~', '!~*', 'similar to',
'not similar to', 'not ilike', '~~*', '!~~*',
'between',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was for my initial implementation, I'm removing it right now.

@@ -36,6 +36,7 @@ class Grammar extends BaseGrammar
'offset',
'unions',
'lock',
'between',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was for my initial implementation, I'm removing it right now.

@BrokenSt0rm
Copy link
Contributor Author

Removed, sorry. My mistake.

@taylorotwell taylorotwell merged commit 910694c into laravel:5.7 Dec 6, 2018
@nanaya
Copy link
Contributor

nanaya commented Dec 14, 2018

Just want to say HAVING in postgres and sqlite3 (and maybe more?) turns the query into grouped query.

// postgres
mydb=> select * from users having id between 10 and 20;
ERROR:  column "users.id" must appear in the GROUP BY clause or be used in an aggregate function
LINE 1: select * from users having id between 10 and 20;
// sqlite3
SQLite version 3.25.1 2018-09-18 20:20:44
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
sqlite> create table users (id integer);
sqlite> select * from users having id between 10 and 20;
Error: a GROUP BY clause is required before HAVING
sqlite>

Also HAVING ... BETWEEN query doesn't use index in mysql (except in rare cases).

[test]> create table users (id int, email varchar(255));
Query OK, 0 rows affected (0.67 sec)

[test]> create index users_id on users(id);
Query OK, 0 rows affected (0.62 sec)
Records: 0  Duplicates: 0  Warnings: 0

[test]> explain select * from users having id between 1 and 10;
+----+-------------+-------+------------+------+---------------+------+---------+------+------+----------+-------+
| id | select_type | table | partitions | type | possible_keys | key  | key_len | ref  | rows | filtered | Extra |
+----+-------------+-------+------------+------+---------------+------+---------+------+------+----------+-------+
|  1 | SIMPLE      | users | NULL       | ALL  | NULL          | NULL | NULL    | NULL |    1 |   100.00 | NULL  |
+----+-------------+-------+------------+------+---------------+------+---------+------+------+----------+-------+
1 row in set, 1 warning (0.00 sec)

[test]> explain select * from users where id between 1 and 10;
+----+-------------+-------+------------+-------+---------------+----------+---------+------+------+----------+-----------------------+
| id | select_type | table | partitions | type  | possible_keys | key      | key_len | ref  | rows | filtered | Extra                 |
+----+-------------+-------+------------+-------+---------------+----------+---------+------+------+----------+-----------------------+
|  1 | SIMPLE      | users | NULL       | range | users_id      | users_id | 5       | NULL |    1 |   100.00 | Using index condition |
+----+-------------+-------+------------+-------+---------------+----------+---------+------+------+----------+-----------------------+
1 row in set, 1 warning (0.01 sec)

There's WHERE ... BETWEEN which actually works everywhere (and uses index).

@mfn
Copy link
Contributor

mfn commented Dec 14, 2018

That was my fear. Seems a lot of edge cases to be aware for this feature.

I get the feeling this is better left for users who need it and just should use any *raw* variant and do it themselves then having this integrated and working problematic.

TL;DR: I'm in favour of removing it.

@BrokenSt0rm
Copy link
Contributor Author

@nanaya @mfn

It's obvious that you have to use a group by in combination with an having clause, and obviously you must select only fields used in grouping clause, not select *.
The where between can't be used when you have to filter after a group operation, which was our case in my company, because where clauses are applied before the grouping operation, it's not a solution.
Only MySQL will not comply with the query provided because it's more flexible in this case, but that query makes no sense at all.
Following that thinking line, we should remove also having method because that errors would still show up.
When you use a database you must know how it works, an ORM like Eloquent don't make any changes to this assumption.

@nanaya
Copy link
Contributor

nanaya commented Dec 15, 2018

I only mentioned it because the wrong example query made it to laravel news page.

izazueta pushed a commit to izazueta/framework that referenced this pull request Apr 22, 2020
* We need this to use the operator in a having clause.
* On Laravel 5.5, this operator was removed by laravel#22182
* On Laravel 5.7 there is a new havingBetween method that we can use instead laravel#26758
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants