Skip to content
This repository has been archived by the owner on Jul 22, 2022. It is now read-only.

Fix mssql limit issue #671

Closed
wants to merge 1 commit into from

Conversation

wbloszyk
Copy link
Member

@wbloszyk wbloszyk commented May 20, 2020

Subject

This PR replace limit from database to php. This will resolve problem with diffrence usages of limit function in some sql.

SQL Server / MS Access Syntax:

SELECT TOP number|percent column_name(s)
FROM table_name
WHERE condition;

MySQL Syntax:

SELECT column_name(s)
FROM table_name
WHERE condition
LIMIT number;

Oracle Syntax:

SELECT column_name(s)
FROM table_name
WHERE ROWNUM <= number;

I am targeting this branch, because it is PATCH.

Part of sonata-project/dev-kit#689

@greg0ire greg0ire added the patch label May 20, 2020
@greg0ire
Copy link
Contributor

What is the issue? How does your patch fix it?

@@ -101,10 +101,9 @@ public function getProductCount(CategoryInterface $category, $limit = 1000)
AND (c.enabled = :categoryEnabled OR c.enabled IS NULL)
AND p.parent_id IS NULL
AND pc.category_id = :categoryId
LIMIT %d
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you seen the comment on line 87? Also, what are the consequences of removing this LIMIT for other RDBMSs?

Copy link
Member Author

Choose a reason for hiding this comment

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

I update RP description.

Copy link
Member

Choose a reason for hiding this comment

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

IMO, we should try to leverage the DBAL as much as possible in order to avoid dealing with specific RDBMSs by our own responsibility. This way, we could build the subquery using DQL or a query builder, and then if it is really required, get the raw SQL query to be used with the main query.
In the worst case, if that is not possible, we should build the different syntaxes by hand.
Even if this query is just intended to return the counted results, I guess leaving the limit open in the persistence layer is not good.

Copy link
Contributor

Choose a reason for hiding this comment

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

@phansys have you seen the comment on line 87? @wbloszyk what about you?

Copy link
Member

Choose a reason for hiding this comment

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

@phansys have you seen the comment on line 87? @wbloszyk what about you?

Yes. What I mean is something like this:

$dql = 'SELECT DISTINCT(pc.product_id) FROM %s pc ... LIMIT %d';
$sql = 'SELECT count(cnt.product_id) AS "cntId" FROM('.$dql->getSQL().');';

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, I read to fast it seems… sorry!

Copy link
Member Author

Choose a reason for hiding this comment

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

@phansys have you seen the comment on line 87? @wbloszyk what about you?

Yes. What I mean is something like this:

$dql = 'SELECT DISTINCT(pc.product_id) FROM %s pc ... LIMIT %d';
$sql = 'SELECT count(cnt.product_id) AS "cntId" FROM('.$dql->getSQL().');';
  • Subqueris is not allowed.
  • Generating SQL crash (DQL look the same)
  • Modify SQL for each platform is to mest

ProductCategoryManager::getProductCount() is only use in ProductMenuBuilder::getCategoryTitle() to display counts in menu.

We can inject ProductManager and use:

public function getCategoryActiveProductsQueryBuilder(?CategoryInterface $category = null, $filter = null, $option = null)

This change will also limit products count when filters will be set.

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, the syntax SELECT COUNT(DISTINCT pc.product_id) is supported by DQL, even by query builder with Expr::countDistinct(). Why we need a subquery then?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know doctrine to much. Anyway we should get the same result from ProductCategoryManager::getProductCount() and ProductManager::getProductCountInCategory
Names are example. We should reuse query as much as possible. It is also yours words. I will add some core example in Monday.

Copy link
Member

Choose a reason for hiding this comment

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

Great! 👍
Just let me know if you need help. I'm not using this bundle, but I can provide support with the ORM if required.

@@ -101,10 +101,9 @@ public function getProductCount(CategoryInterface $category, $limit = 1000)
AND (c.enabled = :categoryEnabled OR c.enabled IS NULL)
AND p.parent_id IS NULL
AND pc.category_id = :categoryId
LIMIT %d
Copy link
Member

Choose a reason for hiding this comment

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

IMO, we should try to leverage the DBAL as much as possible in order to avoid dealing with specific RDBMSs by our own responsibility. This way, we could build the subquery using DQL or a query builder, and then if it is really required, get the raw SQL query to be used with the main query.
In the worst case, if that is not possible, we should build the different syntaxes by hand.
Even if this query is just intended to return the counted results, I guess leaving the limit open in the persistence layer is not good.

@github-actions
Copy link

This PR 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.

@github-actions github-actions bot added the stale label Nov 20, 2020
@github-actions github-actions bot closed this Nov 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants