Doctrine 1
  1. Doctrine 1
  2. DC-701

Aggregates functions do not return proper values when using many relationships and limits

    Details

    • Type: Bug Bug
    • Status: Reopened
    • Priority: Blocker Blocker
    • Resolution: Unresolved
    • Affects Version/s: 1.2.2
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Environment:
      XP Xamp

      Description

      Hi All

      I have encountered a problem that seems very core to the way that doctrine works – if you apply an aggregate function to a column in a table and then join to another table via a many relationship while also using a limit, like so:

      $q = Doctrine_Query::create();
      $q->from('Customer Customer'); 
      $q->addSelect('COUNT(Customer.id) as COUNT_customer_id');
      $q->addSelect('Customer_Order.id as order_id'); 
      $q->leftJoin('Customer.Order Customer_Order'); // Many relationship here
      $q->limit(20);
      

      It produces this correct DQL:

      SELECT COUNT(Customer.id) as COUNT_customer_id, Customer_Order.id as order_id 
      FROM Customer Customer 
        LEFT JOIN Customer.Order Customer_Order 
      LIMIT 20
      

      However the SQL it produces will not return an accurate count – the count is restricted by the limit:

      SELECT COUNT(p.id) AS p__0, p2.id AS p2__1 
      FROM product_customers p 
        LEFT JOIN product_orders p2 ON p.id = p2.customer_id 
      WHERE p.id IN ('1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12', '13', '14', '15', '16', '17', '18', '19', '20')
      

      This produces a count of 21 instead of what it should be (1000).

      The reason for this is because Doctrine's internal functionality does an intermediary query where it gets gets the ids needed from the customer table in order to use them as the IN portion of the constraints on the final query – like so:

      SELECT DISTINCT p3.id FROM product_customers p3 LIMIT 20
      

      It may seem strange that I am applying a limit to a query which will aggregate to 1 to row . In the actual queries that alerted me to this problem I am using a group by. The reason I am not reporting this bug with a group by in my example however is that you can not use a group by with a many relationship and a limit in doctrine 1.2.2 as it works currently (see bug: DC-594). However I am personally able to use a limit with a group by and many relationship since I am using a version of the code I patched my self to repair bug DC-594.

      Here is my sample schema in case its helpful.

      detect_relations: false
      package: Example
      options:
        type: INNODB
        charset: utf8
      Order:
        tableName: orders
        columns:
          order_id:
            type: integer(4)
            primary: true
            notnull: true
          customer_id:
            type: integer(4)
          order_date: timestamp
        relations:
          Customer:
            type: one
            local: customer_id
            foreign: customer_id
        options:
          type: InnoDB
      Customer:
        tableName: customers
        columns:
          customer_id:
            type: integer(4)
            primary: true
            notnull: true
            autoincrement: true
          firstname:
            type: string(45)
          lastname:
            type: string(45)
          streetaddress:
            type: string(45)
          city:
            type: string(45)
          state:
            type: string(45)
          postalcode:
            type: string(45)
        relations:
          Order:
            type: many
            local: customer_id
            foreign: customer_id
        options:
          type: InnoDB
      

      Thanks much.

      Will Ferrer

      edit: split SQL code to make the discussion readable without huge horizontal scrolling...

        Activity

        Hide
        Jonathan H. Wage added a comment -

        Is this still a problem? I am not sure if this can be patched easily. The limit subquery algorithm is flawed deeply but also a core part of the current way Doctrine 1 works.

        Show
        Jonathan H. Wage added a comment - Is this still a problem? I am not sure if this can be patched easily. The limit subquery algorithm is flawed deeply but also a core part of the current way Doctrine 1 works.
        Hide
        will ferrer added a comment -

        Hi Jon

        This bug is still an issue for me but I think it may be VERY hard to patch since it seems very core to the way Doctrine 1 works.

        The project I am working on at the moment is a visual query builder that is highly reliant on Doctrine. In order to prevent users from making queries that would trigger this bug I am currently giving an alert when ever a query that would trigger this bug is created.

        It would be great if this were patchable but if not I should be able to get by.

        Does Doctrine 2 fix this problem?

        Thanks for the help.

        Will Ferrer

        Show
        will ferrer added a comment - Hi Jon This bug is still an issue for me but I think it may be VERY hard to patch since it seems very core to the way Doctrine 1 works. The project I am working on at the moment is a visual query builder that is highly reliant on Doctrine. In order to prevent users from making queries that would trigger this bug I am currently giving an alert when ever a query that would trigger this bug is created. It would be great if this were patchable but if not I should be able to get by. Does Doctrine 2 fix this problem? Thanks for the help. Will Ferrer
        Hide
        Jonathan H. Wage added a comment -

        In Doctrine 2 the limit subquery algorithm does not exist. It is now up to the developer to write the query to handle the scenario instead of Doctrine "trying" to automate it for you. Since the situation where it is needed it is so rare, and each case can be slightly different it is better to let the developer handle it in those cases.

        Show
        Jonathan H. Wage added a comment - In Doctrine 2 the limit subquery algorithm does not exist. It is now up to the developer to write the query to handle the scenario instead of Doctrine "trying" to automate it for you. Since the situation where it is needed it is so rare, and each case can be slightly different it is better to let the developer handle it in those cases.
        Hide
        will ferrer added a comment -

        Hi Jon

        Does that mean that Doctrine 2 can no longer intelligently handle limits with many relationships, and doesn't have the ability to hydrate a return with sub arrays in it for many relationships?

        Thanks again for your help.

        Will Ferrer

        Show
        will ferrer added a comment - Hi Jon Does that mean that Doctrine 2 can no longer intelligently handle limits with many relationships, and doesn't have the ability to hydrate a return with sub arrays in it for many relationships? Thanks again for your help. Will Ferrer
        Hide
        Jonathan H. Wage added a comment -

        That is correct. We do not automatically try and limit the relationships with a "limit subquery" as it causes more problems then it helps.

        Show
        Jonathan H. Wage added a comment - That is correct. We do not automatically try and limit the relationships with a "limit subquery" as it causes more problems then it helps.
        Hide
        will ferrer added a comment -

        Hi Jon

        I was thinking about what you said here – that the "limit subquery" causes more harm than good. It occur to me that I really do like being able to use this method (it comes in very handy very often) but some times it would really help to be able to turn it off (the bug in this thread is a good example such a time). So I figured why not just make an option to turn it off and have the best of both worlds? I looked at the code and found it was really very easy to put in a property which can be set on the query object to enable or disable the use of the limit subquery.

        I made the change in my code base and found it very useful for several situations I was dealing with in my own project.

        The one thing I couldn't do is make a test case for this new feature – I couldn't find an existing test case that was actually triggering the limit subquery as I understood it to work (I couldn't find a test case that would put an WHERE IN constraint with all the ids gathered in the limit subquery. I tried some test cases that I THOUGHT would activate this feature but it didn't seem that they did).

        After adding this feature to my code base I also wanted a new hydrator – one that worked like scalar but would put in array key names that were the same as the ones you would see in HYDRATE_ARRAY. I noticed that HYDRATE_SCALAR already had the ability to do this but it required that a value of false be passed into the $aliasPrefix argument of the _gatherRowData function. I made a custom hydrator I call HYDRATE_ARRAY_SHALLOW that passes in this false value. I then realized this might be handy to add to doctrine so I integrated it into my code base and made a few test cases for it.

        There are 2 patches I am now attaching this to thread:

        disableLimitSubquery_2010-06-10_Doctrine_1.2_SVN.patch – this patch adds the disableLimitSubquery property to query objects so that users can turn off the use of the subquery for those times when they don't want to use that behavior (fixing the bug in this thread and probably others)

        and

        disableLimitSubquery_and_HYDRATE_ARRAY_SHALLOW_2010-06-10_Doctrine_1.2_SVN.patch – this is the same as the first patch but also contains the HYDRATE_ARRAY_SHALLOW hydration type I mentioned above (which tends to go well with disableLimitSubquery turned on).

        I put these in 2 patches incase you liked 1 feature but disliked the other.

        If you like either of this features I would be very happy to see them added to doctrine.

        Also if you have any advice on how to improve these features (or info on how to make a good test case for disableLimitSubquery) please let me know.

        Hope you are well.

        Will Ferrer

        Show
        will ferrer added a comment - Hi Jon I was thinking about what you said here – that the "limit subquery" causes more harm than good. It occur to me that I really do like being able to use this method (it comes in very handy very often) but some times it would really help to be able to turn it off (the bug in this thread is a good example such a time). So I figured why not just make an option to turn it off and have the best of both worlds? I looked at the code and found it was really very easy to put in a property which can be set on the query object to enable or disable the use of the limit subquery. I made the change in my code base and found it very useful for several situations I was dealing with in my own project. The one thing I couldn't do is make a test case for this new feature – I couldn't find an existing test case that was actually triggering the limit subquery as I understood it to work (I couldn't find a test case that would put an WHERE IN constraint with all the ids gathered in the limit subquery. I tried some test cases that I THOUGHT would activate this feature but it didn't seem that they did). After adding this feature to my code base I also wanted a new hydrator – one that worked like scalar but would put in array key names that were the same as the ones you would see in HYDRATE_ARRAY. I noticed that HYDRATE_SCALAR already had the ability to do this but it required that a value of false be passed into the $aliasPrefix argument of the _gatherRowData function. I made a custom hydrator I call HYDRATE_ARRAY_SHALLOW that passes in this false value. I then realized this might be handy to add to doctrine so I integrated it into my code base and made a few test cases for it. There are 2 patches I am now attaching this to thread: disableLimitSubquery_2010-06-10_Doctrine_1.2_SVN.patch – this patch adds the disableLimitSubquery property to query objects so that users can turn off the use of the subquery for those times when they don't want to use that behavior (fixing the bug in this thread and probably others) and disableLimitSubquery_and_HYDRATE_ARRAY_SHALLOW_2010-06-10_Doctrine_1.2_SVN.patch – this is the same as the first patch but also contains the HYDRATE_ARRAY_SHALLOW hydration type I mentioned above (which tends to go well with disableLimitSubquery turned on). I put these in 2 patches incase you liked 1 feature but disliked the other. If you like either of this features I would be very happy to see them added to doctrine. Also if you have any advice on how to improve these features (or info on how to make a good test case for disableLimitSubquery) please let me know. Hope you are well. Will Ferrer
        Hide
        will ferrer added a comment -

        Reopened because I added a patch that I think in essence fixes the issues.

        Show
        will ferrer added a comment - Reopened because I added a patch that I think in essence fixes the issues.
        Hide
        Jonathan H. Wage added a comment -
        Show
        Jonathan H. Wage added a comment - Thanks for the issue and patches. Fixed here http://github.com/doctrine/doctrine1/commit/2ad78e62e360133efc04bf6897bf679c7f3d833b
        Hide
        will ferrer added a comment -

        individual patch for this issue with out other features included

        Show
        will ferrer added a comment - individual patch for this issue with out other features included
        Hide
        will ferrer added a comment -

        adding test cases for these features

        Show
        will ferrer added a comment - adding test cases for these features
        Hide
        will ferrer added a comment -

        reopened because I posted some test cases to add to doctrine along with the patchs previously posted

        Show
        will ferrer added a comment - reopened because I posted some test cases to add to doctrine along with the patchs previously posted

          People

          • Assignee:
            Jonathan H. Wage
            Reporter:
            will ferrer
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated: