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

Complex HAVING & WHERE queries #491

Closed
hrach opened this issue Feb 25, 2021 · 11 comments · Fixed by #496
Closed

Complex HAVING & WHERE queries #491

hrach opened this issue Feb 25, 2021 · 11 comments · Fixed by #496
Labels

Comments

@hrach
Copy link
Member

hrach commented Feb 25, 2021

Complex condition like

$users = $this->orm->authors->findBy([
	ICollection::OR,
	['books->title' => 'Book 1'],
	[
		CompareSmallerThanEqualsFunction::class,
		[CountAggregateFunction::class, 'tagFollowers->tag'],
		2,
	],
]);

creates an invalid SQL:

SELECT `authors`.* 
FROM `authors` AS `authors` 
   LEFT JOIN `books` AS `books` ON (`authors`.`id` = `books`.`author_id`)
   LEFT JOIN `tag_followers` AS `tagFollowers` ON (`authors`.`id` = `tagFollowers`.`author_id`)
GROUP BY `authors`.`id`
HAVING ((`books`.`title` = 'Book 1')) OR (COUNT(`tagFollowers`.`tag_id`) <= 2)

Db server doesn't know the books.title value since it is not aggregate in having.

Doing aggregation is not possible here, so we have to employ complex query rewrite:

  • put non-aggregated condition to join
  • select the joined table PK column
  • compare the selected column to not null

which will produce something like:

SELECT `authors`.*, `books`.`id` AS `_booksid`
FROM `authors` AS `authors` 
LEFT JOIN `books` AS `books` ON (`authors`.`id` = `books`.`author_id` and `books`.`title` = 'Book 1') 
LEFT JOIN `tag_followers` AS `tagFollowers` ON (`authors`.`id` = `tagFollowers`.`author_id`) 
GROUP BY `authors`.`id`
HAVING (`_booksid` IS NOT NULL) OR (COUNT(`tagFollowers`.`tag_id`) <= 2)

Failing testcase: https://github.com/nextras/orm/tree/complex_having_where_aggregation_condition

@hrach hrach added the bug label Feb 25, 2021
@hrach
Copy link
Member Author

hrach commented Feb 25, 2021

The problem here is that the SQL construction happens along the way of processing the expression and the move to HAVING may have happen at the end when the need for rewrite is almost impossible to do. Really not sure how the fix should look like.

@hrach
Copy link
Member Author

hrach commented Feb 25, 2021

Ok, the wild proof-of-concept implementation is here: 613c763. Hopefully it doesn't break any other thing. The solution is: do the has-many join rewrite always - i.e. for every has-many relationship do not consider if rewrite is needed for having clause condition.

This final SQL is then

 SELECT "authors".*
 FROM "public"."authors" AS "authors"
 LEFT JOIN "books" as "books" ON (("authors"."id" = "books"."author_id") AND "books"."title" = 'Book 1')
 LEFT JOIN "tag_followers" as "tagFollowers" ON ("authors"."id" = "tagFollowers"."author_id")
 GROUP BY "authors"."id", "books"."id"
 HAVING ((COUNT("books"."id") > 0)) OR (COUNT("tagFollowers"."tag_id") <= 2)

@stpnkcrk
Copy link
Contributor

stpnkcrk commented Feb 26, 2021

Tried this on a project and the complex condition seems to be working but it breaks the rest quite a lot unfortunately.

Simple condition $this->changeRequests->findBy(['assignee->id' => 19])->fetchAll() generates wrong query.

Generated:

SELECT `change_request`.* 
FROM `change_request` AS `change_request` 
LEFT JOIN `user` as `assignee` ON (`change_request`.`assignee_id` = `assignee`.`id`) 
WHERE (`assignee`.`id`)

Expected:

SELECT `change_request`.* 
FROM `change_request` AS `change_request` 
LEFT JOIN `user` AS `assignee` ON (`change_request`.`assignee_id` = `assignee`.`id`) 
WHERE (`assignee`.`id` = 19)

@hrach
Copy link
Member Author

hrach commented Feb 26, 2021

Thanks for testing. This is probably some small bug which I will fix. The main fear here is that some quries may have:

  • different (worse) performance until now
  • different semantic - returning something else
    Will think it through and check it, this require some additional focus.

@hrach hrach linked a pull request Mar 2, 2021 that will close this issue
@hrach
Copy link
Member Author

hrach commented Mar 26, 2021

Merged into master. Hopefully it won't break more then it fixes :)

@pdrhlik
Copy link

pdrhlik commented Apr 5, 2023

Hi @hrach,

I'm using nextras/orm:4.0.6 and I can't figure out what I'm doing wrong but I still keep getting the same error. I'm trying to fetch results where challenge has a country with id 1 or has no assigned countries at all. A challenge can have multiple assigned countries. This is my call:

$this->orm->challenges->findBy([
	ICollection::OR,
	["countries->country" => 1],
	[
		CompareEqualsFunction::class,
		[CountAggregateFunction::class, "countries->country"],
		0
	]
]);

It generates the following SQL which is invalid and throws this error Error in query (1054): Unknown column 'countries.country_id' in 'having clause':

SELECT `challenge`.*
FROM `challenge` AS `challenge`
LEFT JOIN `challenge_country` AS `countries` ON (`challenge`.`id` = `countries`.`challenge_id`)
GROUP BY `challenge`.`id`
HAVING ((`countries`.`country_id` = 1)) OR (COUNT(`countries`.`country_id`) = 0)

Manually adjusting the code by adding the countries.country_id column to the select resolves the issue:

SELECT `challenge`.*, `countries`.`country_id`
FROM `challenge` AS `challenge`
LEFT JOIN `challenge_country` AS `countries` ON (`challenge`.`id` = `countries`.`challenge_id`)
GROUP BY `challenge`.`id`
HAVING ((`countries`.`country_id` = 1)) OR (COUNT(`countries`.`country_id`) = 0)

Do you have any idea if I'm doing something wrong or is there an issue with the library? Thanks a lot for the help!

Cheers,
Patrik

@hrach
Copy link
Member Author

hrach commented Apr 5, 2023

@pdrhlik this is, sadly, still in main only. use 5.0@dev (ideally report an issue in main if not usable for you).

@pdrhlik
Copy link

pdrhlik commented Apr 5, 2023

Thanks for the info, I'll try upgrading to 5.0.

@hrach
Copy link
Member Author

hrach commented Apr 5, 2023

TBH I have to put more effort there and release 5.0 finally.

@pdrhlik
Copy link

pdrhlik commented Apr 5, 2023

Great, I'll be waiting for the release then. I just quickly tried updating it and it didn't work because of some compatibility issues anyway.

@hrach
Copy link
Member Author

hrach commented Apr 5, 2023

Great, I'll be waiting for the release then.

I wanted to so for last year and more, no promises now :D

it didn't work because of some compatibility issues anyway

Please, report them as new issue. This would help me a lot. at least with their documentation. Thank you.

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

Successfully merging a pull request may close this issue.

3 participants