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

Add $setWindowFields aggregation pipeline stage and window operators #2517

Merged
merged 6 commits into from
Apr 12, 2023

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Mar 28, 2023

Q A
Type feature
BC Break no
Fixed issues

Summary

Last but not least, this pull request adds support for window operators and the $setWindowFields aggregation pipeline stage to the builder. There is some code duplication between group accumulators and window operators, as the latter includes all group accumulators except for $mergeObjects. To avoid polluting the interfaces and traits, I've decided to accept the duplication considering that the methods only proxy calls to the Expr class.

@alcaeus alcaeus added this to the 2.6.0 milestone Mar 28, 2023
@alcaeus alcaeus requested a review from jmikola March 28, 2023 14:09
@alcaeus alcaeus self-assigned this Mar 28, 2023
@alcaeus alcaeus changed the title Feature/set window fields agg stage Add $setWindowFields aggregation pipeline stage and window operators Mar 28, 2023

public function getExpression(): array
{
return array_merge_recursive(
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, the result of array_map() is going to be an associative array of field names to { window: ... } objects. Expr::getExpression() is documented as returning array<string, mixed>.

Isn't that problematic if the expression value isn't an array? Actually, even if it is an array how do you merge a stdClass into it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, this is one where I wasn't sure whether using array_merge_recursive would be sensible. To explain, while the Expr class can generate the expressions used in the setWindowFields output, it can't handle the additional window option used there. Expr can generate this:

{ foo: { $rank: {} }
// Generated using $expr->field('foo')->rank();

But not this as is valid in a window stage:

{ foo: {
    $rank: {},
    window: {...}
}}

To work around this, we use the Expr class to handle operators and store the windows for fields ourselves. In getExpression, we generate the output of the Expr object, which gives us something like this:

[
    'foo' => ['$rank' => []],
    'bar' => ['$documentNumber' => []],
]

The windows property contains the windows (in my example, only foo uses a window):

[
    'foo' => ['documents' => ['unbounded', 'current']],
]

The array_map call then generates the window portion, resulting in this:

[
    'foo' => ['window' => (object) ['documents' => ['unbounded', 'current']]],
]

We can then use array_merge_recursive to recursively merge windows and field expressions together, resulting in this:

[
    'foo' => [
        '$rank' => [],
        'window' => (object) [...],
    ],
    'bar' => ['$documentNumber' => []],
]

To make a long story short, I'll add a comment explaining what this does.

@alcaeus alcaeus requested a review from jmikola March 29, 2023 07:36
@alcaeus alcaeus force-pushed the feature/setWindowFields-agg-stage branch from d8a6cdd to a14ca8a Compare April 11, 2023 09:08
@alcaeus alcaeus force-pushed the feature/setWindowFields-agg-stage branch from a14ca8a to 4099c96 Compare April 11, 2023 10:13
private function getWindowOptionsForFields(): array
{
return array_map(
static fn ($window): object => (object) ['window' => $window],
Copy link
Member

Choose a reason for hiding this comment

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

PHP-CS-Fixer/PHP-CS-Fixer#5449 (comment) was an interesting read about the relevance of static here.

@alcaeus alcaeus merged commit 3bcf829 into doctrine:2.6.x Apr 12, 2023
@alcaeus alcaeus deleted the feature/setWindowFields-agg-stage branch November 24, 2023 08: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.

2 participants