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

fetchGroup() #87

Merged
merged 3 commits into from Jan 8, 2015
Merged

fetchGroup() #87

merged 3 commits into from Jan 8, 2015

Conversation

mbrevda
Copy link
Contributor

@mbrevda mbrevda commented Dec 18, 2014

Add a fetchGroup() fetch strategy that groups returned data by the first column. Unlike fetchAssoc(), fetchGroup() returns a much flatter set of values.

Also added a test command (composer test) and set some variables for phpunit.

@@ -57,7 +57,7 @@ Alternatively, [download a release](https://github.com/auraphp/Aura.Sql/releases
[![Code Coverage](https://scrutinizer-ci.com/g/auraphp/Aura.Sql/badges/coverage.png?b=develop-2)](https://scrutinizer-ci.com/g/auraphp/Aura.Sql/)
[![Build Status](https://travis-ci.org/auraphp/Aura.Sql.png?branch=develop-2)](https://travis-ci.org/auraphp/Aura.Sql)

To run the unit tests at the command line, issue `phpunit -c tests/unit/`. (This requires [PHPUnit][] to be available as `phpunit`.)
To run the unit tests at the command line, issue `composer test`. (This requires [PHPUnit][] to be available as `phpunit`.)
Copy link
Member

Choose a reason for hiding this comment

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

Interesting approach. But the problem is Aura.Sql is truly standalone and people don't need composer for testing :) .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Who doesn't use composer?! 😄

@mbrevda
Copy link
Contributor Author

mbrevda commented Dec 22, 2014

Here are examples for the same exact query, and how the results are returned for different fetch strategies. This is the query:

$res = $this->db
            ->select()
            ->cols([
                'fooId',
                'id'
            ])
            ->from('foobar')
            ->where('fooId in(:in)')
            ->bindValue('in', $in)
            ->fetchAll();

and this is the table:

mysql> select id, fooId from foobar;                                                                 
+----+-------+
| id | fooId |
+----+-------+
|  1 |    90 |
|  2 |    90 |
|  3 |    90 |
|  4 |    90 |
|  5 |    90 |
|  7 |    90 |
|  8 |    90 |
|  9 |    90 |
| 10 |    90 |
| 11 |    90 |
| 13 |    90 |
| 14 |    90 |
| 15 |    90 |
| 16 |    90 |
| 17 |    90 |
| 19 |    90 |
| 20 |    90 |
| 21 |    91 |
| 22 |    91 |
| 23 |    91 |
| 25 |    91 |
| 26 |    91 |
| 27 |    91 |
| 28 |    91 |
| 29 |    91 |
| 31 |    91 |
| 32 |    91 |
| 33 |    91 |
| 34 |    91 |
| 35 |    91 |
+----+-------+
30 rows in set (0.00 sec)

FetchAll()

Array
(
    [0] => Array
        (
            [fooId] => 90
            [id] => 1
        )

    [1] => Array
        (
            [fooId] => 90
            [id] => 2
        )

    [2] => Array
        (
            [fooId] => 90
            [id] => 3
        )

    [3] => Array
        (
            [fooId] => 90
            [id] => 4
        )

    [4] => Array
        (
            [fooId] => 90
            [id] => 5
        )

    [5] => Array
        (
            [fooId] => 90
            [id] => 7
        )

    [6] => Array
        (
            [fooId] => 90
            [id] => 8
        )

    [7] => Array
        (
            [fooId] => 90
            [id] => 9
        )

    [8] => Array
        (
            [fooId] => 90
            [id] => 10
        )

    [9] => Array
        (
            [fooId] => 90
            [id] => 11
        )

    [10] => Array
        (
            [fooId] => 90
            [id] => 13
        )

    [11] => Array
        (
            [fooId] => 90
            [id] => 14
        )

    [12] => Array
        (
            [fooId] => 90
            [id] => 15
        )

    [13] => Array
        (
            [fooId] => 90
            [id] => 16
        )

    [14] => Array
        (
            [fooId] => 90
            [id] => 17
        )

    [15] => Array
        (
            [fooId] => 90
            [id] => 19
        )

    [16] => Array
        (
            [fooId] => 90
            [id] => 20
        )

    [17] => Array
        (
            [fooId] => 91
            [id] => 21
        )

    [18] => Array
        (
            [fooId] => 91
            [id] => 22
        )

    [19] => Array
        (
            [fooId] => 91
            [id] => 23
        )

    [20] => Array
        (
            [fooId] => 91
            [id] => 25
        )

    [21] => Array
        (
            [fooId] => 91
            [id] => 26
        )

    [22] => Array
        (
            [fooId] => 91
            [id] => 27
        )

    [23] => Array
        (
            [fooId] => 91
            [id] => 28
        )

    [24] => Array
        (
            [fooId] => 91
            [id] => 29
        )

    [25] => Array
        (
            [fooId] => 91
            [id] => 31
        )

    [26] => Array
        (
            [fooId] => 91
            [id] => 32
        )

    [27] => Array
        (
            [fooId] => 91
            [id] => 33
        )

    [28] => Array
        (
            [fooId] => 91
            [id] => 34
        )

    [29] => Array
        (
            [fooId] => 91
            [id] => 35
        )

)

FetchAssoc()

Array
(
    [90] => Array
        (
            [fooId] => 90
            [id] => 20
        )

    [91] => Array
        (
            [fooId] => 91
            [id] => 35
        )

)

FetchGroup()

Array
(
    [90] => Array
        (
            [0] => 1
            [1] => 2
            [2] => 3
            [3] => 4
            [4] => 5
            [5] => 7
            [6] => 8
            [7] => 9
            [8] => 10
            [9] => 11
            [10] => 13
            [11] => 14
            [12] => 15
            [13] => 16
            [14] => 17
            [15] => 19
            [16] => 20
        )

    [91] => Array
        (
            [0] => 21
            [1] => 22
            [2] => 23
            [3] => 25
            [4] => 26
            [5] => 27
            [6] => 28
            [7] => 29
            [8] => 31
            [9] => 32
            [10] => 33
            [11] => 34
            [12] => 35
        )

)

Notice how only fetchGroup() returns a simple array that can be appended to an entity as simple as and without any further manipulation for the results:

foreach ($entity as $e) {
    $e->children = isset($results[$e->id]) ? $results[$e->id] : [];
}

@pmjones
Copy link
Member

pmjones commented Jan 8, 2015

I like it. Essentially it exposes PDO::FETCH_GROUP as a method. If you make these two changes, I can merge it directly:

  1. Remove the PHPUnit changes. (The PHPUnit-related stuff needs to be identical across libraries so that the release script works properly.)

  2. Instead of a boolean $singleColoumn (sic) as the third param, replace with something like this ...

    public function fetchGroup(
        $statement,
        array $values = array(),
        $style = null
    ) {
        $args = self::FETCH_GROUP;
    
        if ($style) {
            $args = $args | $style;
        }
    

    ... and update the README to indicate you can pass self::FETCH_COLUMN or self::FETCH_NAMED for changed behavior. (Unless there is some reason that FETCH_GROUP cannot be used by itself, in which case please instruct me.)

Aside from that, very nice, and good testing.

@mbrevda
Copy link
Contributor Author

mbrevda commented Jan 8, 2015

Sorry, testing leaked in from a different branch. Will remove.
Glad you like it, I'll have the changes shortly.

@mbrevda
Copy link
Contributor Author

mbrevda commented Jan 8, 2015

  1. When fetching as a group, it is always necessary to fetch as either FETCH_COLUMN or FETCH_NAMED - your code doesn't include that.
  2. The reason for the boolean value is that no other fetch style is appropriate here. Furthermore, when using fluent influences (such as aurasql-queryproxy) it is cumbersome to remember which class to import and which constant to use.
  3. In order for this method to work, it NEEDS to know how many columns are being fetched - hence the specifically named parameter
  4. Just cause I can dribble out a few lines of php doesn't mean I know how to spell 😞

@pmjones
Copy link
Member

pmjones commented Jan 8, 2015

it NEEDS to know how many columns are being fetched - hence the specifically named parameter

Gotcha.

As a matter of principle, boolean params are not great. Two options:

  1. Make that particular param default to FETCH_COLUMN, and note in the README that it can be swapped for FETCH_NAMED.
  2. Create two methods: fetchGroupColumn(), and fetchGroupNamed(), with the appropriate arguments embedded in the methods. (I am not married to those method names.)

Option 2 is the probably better plan if there are only two useful FETCH_* styles. If there are more than two, Option 1 is probably the better plan.

@mbrevda
Copy link
Contributor Author

mbrevda commented Jan 8, 2015

How is fetchGroup() and fetchGroupMultiple()?

@pmjones
Copy link
Member

pmjones commented Jan 8, 2015

The comments at http://php.net/manual/en/pdostatement.fetchall.php lead me to believe that there are more valid FETCH_* styles than just those two (e.g., http://php.net/manual/en/pdostatement.fetchall.php#88699). That makes me think Option 1 is the appropriate one here.

@mbrevda
Copy link
Contributor Author

mbrevda commented Jan 8, 2015

I changed the third param to take a style and default to FETCH_COLUMN

@pmjones
Copy link
Member

pmjones commented Jan 8, 2015

OK, now I need you to ... no, just kidding.

pmjones pushed a commit that referenced this pull request Jan 8, 2015
Add fetchGroup() functionality.
@pmjones pmjones merged commit 3fe91eb into auraphp:develop-2 Jan 8, 2015
@mbrevda
Copy link
Contributor Author

mbrevda commented Jan 8, 2015

thanks! Now for the test stuff - what's the best medium to discuss that?

@mbrevda mbrevda deleted the fetchGroup branch January 8, 2015 22:09
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.

None yet

3 participants