Skip to content

feat: function array_group_by - #7438

Merged
kenjis merged 12 commits into
codeigniter4:4.4from
rumpfc:feature-array-group-by
May 14, 2023
Merged

kenjis merged 12 commits into
codeigniter4:4.4from
rumpfc:feature-array-group-by

Conversation

@rumpfc

@rumpfc rumpfc commented Apr 14, 2023

Copy link
Copy Markdown
Contributor

Description
A function to group data rows by values defined by indexes. The function supports the "dot syntax". Optionally it can include empty values (where empty($value) === true) and puts them under key ''.

The depth of return result equals count($indexes).

Example:

$data = [
    [
        'id'    => 1,
        'item'  => 'ball',
        'color' => 'blue',
    ],
    [
        'id'    => 2,
        'item'  => 'book',
        'color' => 'red',
    ],
    [
        'id'   => 3,
        'item' => 'bird',
        'age'  => 5,
    ],
    [
        'id'    => 4,
        'item'  => 'jeans',
        'color' => 'blue',
    ],
];

$result = array_group_by($data, ['color'], true);

Returns following result

$result = [
    'blue' => [
        [
            'id'    => 1,
            'item'  => 'ball',
            'color' => 'blue',
        ],
        [
            'id'    => 4,
            'item'  => 'jeans',
            'color' => 'blue',
        ],
    ],
    'red' => [
        [
            'id'    => 2,
            'item'  => 'book',
            'color' => 'red',
        ],
    ],
    '' => [
        [
            'id'   => 3,
            'item' => 'bird',
            'age'  => 5,
        ],
    ],
];

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the enhancement PRs that improve existing functionalities label Apr 15, 2023
Comment thread system/Helpers/array_helper.php Outdated
Comment thread system/Helpers/array_helper.php Outdated
Comment thread system/Helpers/array_helper.php Outdated
@kenjis

kenjis commented Apr 15, 2023

Copy link
Copy Markdown
Member

Please fix the PHPStan error:

Error: Offset mixed on array{} in isset() does not exist.
 ------ ---------------------------------------------------- 
  Line   system/Helpers/array_helper.php                     
 ------ ---------------------------------------------------- 
  256    Offset mixed on array{} in isset() does not exist.  
 ------ ---------------------------------------------------- 

https://github.com/codeigniter4/CodeIgniter4/actions/runs/4706875155/jobs/8348394968?pr=7438

@kenjis kenjis added the 4.4 label Apr 15, 2023
@rumpfc

rumpfc commented Apr 15, 2023

Copy link
Copy Markdown
Contributor Author
------ ----------------------------------------------------------------- 
  Line   system/Helpers/array_helper.php                                  
 ------ ----------------------------------------------------------------- 
  260    Call to function array_key_exists() with mixed and array{} will  
         always evaluate to false.                                        
 ------ ----------------------------------------------------------------- 

I tried different assignments, incl. string only, but i still get the same error that the function always evaluates to false. But this function call is important for the code to work. Any suggestions?

@kenjis

kenjis commented Apr 15, 2023

Copy link
Copy Markdown
Member

@rumpfc
Can you remove the references? It is difficult to understand.

@kenjis

kenjis commented Apr 15, 2023

Copy link
Copy Markdown
Member

Please run: $ composer cs-fix

@rumpfc

rumpfc commented Apr 15, 2023

Copy link
Copy Markdown
Contributor Author

@rumpfc Can you remove the references? It is difficult to understand.

The references are absolutely necessary. Each iteration of $indexes goes one level deeper into the nested $result. So $currentLevel assigns itself the reference of the subarray.

I can try to implement a recursive alternative (similar how dot_array_search handles it)

@kenjis

kenjis commented Apr 15, 2023

Copy link
Copy Markdown
Member

I checked the behavior. It seems you are correct, and the PHPStan error seems to be false positive.

If you cannot rewrite easily or if the rewrite code will be more complicated, I suggest you suppress the PHPStan error.

@kenjis

kenjis commented Apr 15, 2023

Copy link
Copy Markdown
Member

Can you add test case for dot array syntax?

@rumpfc

rumpfc commented Apr 15, 2023

Copy link
Copy Markdown
Contributor Author

Can you add test case for dot array syntax?

They are already included. See the final yield of both data providers in ArrayHelperTest.

@kenjis

kenjis commented Apr 15, 2023

Copy link
Copy Markdown
Member

Oh, I missed that. Thank you.

@rumpfc

rumpfc commented Apr 16, 2023

Copy link
Copy Markdown
Contributor Author

The recursive approach satisfies both rector and phpstan, but I feel like it requires more memory. TIme performance should be the same.

@kenjis

kenjis commented Apr 19, 2023

Copy link
Copy Markdown
Member

My quick benchmark on macOS shows recursive is slower, but the memory usage are the same.

memory usage: 114232
recursive: 0.8359
reference: 0.6689

    public function test()
    {
        $iterator = new \CodeIgniter\Debug\Iterator();

        $memory_base = memory_get_usage();

        $iterator->add('array_group_by', static function () {
            $indexes = ['gender', 'hr.department'];
            $data = [
                [
                    'id'         => 1,
                    'first_name' => 'Urbano',
                    'gender'     => null,
                    'hr'         => [
                        'country'    => 'Canada',
                        'department' => 'Engineering',
                    ],
                ],
                [
                    'id'         => 2,
                    'first_name' => 'Case',
                    'gender'     => 'Male',
                    'hr'         => [
                        'country'    => null,
                        'department' => 'Marketing',
                    ],
                ],
                [
                    'id'         => 3,
                    'first_name' => 'Emera',
                    'gender'     => 'Female',
                    'hr'         => [
                        'country'    => 'France',
                        'department' => 'Engineering',
                    ],
                ],
                [
                    'id'         => 4,
                    'first_name' => 'Richy',
                    'gender'     => null,
                    'hr'         => [
                        'country'    => null,
                        'department' => 'Sales',
                    ],
                ],
                [
                    'id'         => 5,
                    'first_name' => 'Mandy',
                    'gender'     => null,
                    'hr'         => [
                        'country'    => 'France',
                        'department' => 'Sales',
                    ],
                ],
                [
                    'id'         => 6,
                    'first_name' => 'Risa',
                    'gender'     => 'Female',
                    'hr'         => [
                        'country'    => null,
                        'department' => 'Engineering',
                    ],
                ],
                [
                    'id'         => 7,
                    'first_name' => 'Alfred',
                    'gender'     => 'Male',
                    'hr'         => [
                        'country'    => 'France',
                        'department' => 'Engineering',
                    ],
                ],
                [
                    'id'         => 8,
                    'first_name' => 'Tabby',
                    'gender'     => 'Male',
                    'hr'         => [
                        'country'    => 'France',
                        'department' => 'Marketing',
                    ],
                ],
                [
                    'id'         => 9,
                    'first_name' => 'Ario',
                    'gender'     => 'Male',
                    'hr'         => [
                        'country'    => null,
                        'department' => 'Sales',
                    ],
                ],
                [
                    'id'         => 10,
                    'first_name' => 'Somerset',
                    'gender'     => 'Male',
                    'hr'         => [
                        'country'    => 'Germany',
                        'department' => 'Marketing',
                    ],
                ],
            ];
            $actual = array_group_by($data, $indexes, true);
        });

        $htmlTable = $iterator->run(3000);

        echo memory_get_peak_usage() - $memory_base;
        echo $htmlTable; exit;
    }

@datamweb

Copy link
Copy Markdown
Contributor

My quick benchmark on macOS shows recursive is slower, but the memory usage are the same.

Win 11

image

@kenjis

kenjis commented Apr 24, 2023

Copy link
Copy Markdown
Member

@datamweb It is meaningless unless both (reference and recursive) results are compared in the same environment.

@datamweb

Copy link
Copy Markdown
Contributor

@kenjis thanks, Can you suggest a link or reference for a better understanding of this issue?

@kenjis

kenjis commented Apr 24, 2023

Copy link
Copy Markdown
Member

@kenjis

kenjis commented Apr 24, 2023

Copy link
Copy Markdown
Member

@datamweb What is this issue?

If we are talking about benchmarks, it means that they are meaningless unless we compare the results of running them in the same environment. The faster the machine, the faster the results.

What we are discussing now in this GitHub issue is whether reference or recursive code is better on
readability, performance.
For me, recursion seems to be a little slower, but it's not that different, so recursion is fine.

The PHPStan error is a bug in PHPStan; PHPStan does not seem to understand that references change values.

@kenjis

kenjis commented Apr 26, 2023

Copy link
Copy Markdown
Member

Thanks, but there is "Others" section.
Please move to "Helpers and Functions", and use :php:func: to link to the function page.

--- a/user_guide_src/source/changelogs/v4.4.0.rst
+++ b/user_guide_src/source/changelogs/v4.4.0.rst
@@ -87,6 +87,9 @@ Libraries
 Helpers and Functions
 =====================
 
+- **Array:** Added :php:func:`array_group_by()` helper function to group data
+  values together. Supports dot-notation syntax.
+
 Others
 ======
 
@@ -103,7 +106,6 @@ Others
 - **Request:** Added ``IncomingRequest::setValidLocales()`` method to set valid locales.
 - **Table:** Added ``Table::setSyncRowsWithHeading()`` method to synchronize row columns with headings. See :ref:`table-sync-rows-with-headings` for details.
 - **Error Handling:** Now you can use :ref:`custom-exception-handlers`.
-- **Array:** Added ``array_group_by()`` helper function to group data values together. Supports dot-notation syntax.
 
 Message Changes
 ***************

@kenjis kenjis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

Comment thread system/Helpers/array_helper.php Outdated
@kenjis
kenjis merged commit 3089cda into codeigniter4:4.4 May 14, 2023
@rumpfc
rumpfc deleted the feature-array-group-by branch August 23, 2023 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement PRs that improve existing functionalities

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants