PHPLIB-1374 Add tests on all stages #56

GromNaN · 2024-01-24T18:55:51Z

https://www.mongodb.com/docs/manual/reference/operator/aggregation-pipeline/

~~$addFields~~ already done
$bucket
$bucketAuto
$changeStream
$changeStreamSplitLargeEvent
$collStats
$count
$densify
$documents
$facet
$fill
$geoNear
$graphLookup
$group
$indexStats
$limit
$listSampledQueries
$listSearchIndexes
$listSessions
$lookup
$match
$merge
$out
$planCacheStats
$project
$redact
$replaceRoot
$replaceWith
$sample
~~$search~~ (no example)
~~$searchMeta~~ (no example)
$set
$setWindowFields
$skip
$sort
$sortByCount
$unionWith
$unset
$unwind

generator/config/stage/currentOp.yaml

GromNaN · 2024-01-25T14:22:20Z

generator/config/stage/collStats.yaml

+ $collStats:
+ latencyStats:
+ histograms: true


The config contains nested object definition. That's why I kept a generic "config" object argument. DX could be improved with advanced type in phpdoc.

SGTM. I don't think this stage will be used very often.

I agree it will seldom be used, if at all.

But it seems a bit arbitrary to use config here, as it isn't anything found in the $collStats API. Was it really to much to model the various options like you did with currentOp.yaml?

The options themselves don't seem terribly complex, so I don't see a big benefit to not modeling them unless we're concerned about the server API changing.

Ok, I added the 4 parameters.

GromNaN · 2024-01-25T14:23:06Z

generator/config/stage/densify.yaml

+ range:
+ bounds: 'full'
+ step: 200


The "range" config could have a factory and use and enum for "bounds".

generator/config/stage/densify.yaml

GromNaN · 2024-01-25T14:24:40Z

generator/config/stage/fill.yaml

+ time: 1
+ output:
+ price:
+ method: 'linear'


We an create a factory for the output subtype.

Agree. This would look better:

Stage::fill( sortBy: object(date: 1), output: object( score: Fill::locf, price: Fill::linear, ) );

@alcaeus: That's basically what you did for ODM already, yes?

@GromNaN: Will you be creating a ticket to track addition of an output factory?

Indeed: https://github.com/doctrine/mongodb-odm/blob/c49d55a8ea5f96c19c0950485e2fd959ffe9efca/tests/Doctrine/ODM/MongoDB/Tests/Aggregation/Stage/FillTest.php#L23..L25

GromNaN · 2024-01-25T14:26:24Z

generator/config/stage/merge.yaml

+ whenMatched: 'replace'
+ whenNotMatched: 'insert'


I was just about to leave a comment on # WhenMatched in the option definition above.

Is this todo item ticketed?

Should this be ticketed under PHPLIB-1381?

GromNaN · 2024-01-25T14:27:30Z

generator/config/stage/out.yaml

@@ -3,26 +3,38 @@ name: $out
 link: 'https://www.mongodb.com/docs/manual/reference/operator/aggregation/out/'
 type:
 - stage
-encode: object
+encode: single


I have to change this type, because an object with only { coll } is not accepted. The value must be a string or an object with { coll, db }

We could have a factory that return a string or an object, that could be used in $merge also.

@GromNaN: This reminds me of another operator where you were always encoding the long form (array I think) instead of a conditional short form. Perhaps that was $type? Although I'm not sure if it's worth addressing that since it isn't technically required like it is for $out.

In another comment, @alcaeus asked about creating custom encoder logic. Is that feasible with the current implementation, or is it more complicated because the entire OutStage file is generated (and thus would require addressing this through the generator code itself).

IIRC, the original intention of this project was to have an ongoing dependency on the generator, so I understand if there's presently no way to maintain stages and operators through PHP along (in an alternate reality where the generator was only used to create the initial implementation).

Asking the above more for my own curiosity.

I understand the current change to use encode: single here is the most flexible and probably sets you up to later use string|OutCollection for the parameter.

GromNaN · 2024-01-25T14:29:22Z

generator/config/stage/set.yaml

+ # The $sum expression is always build as an array, even if the value is an array field name
+ # $sum: '$homework'
+ $sum: ['$homework']
+ totalQuiz:
+ # $sum: '$quiz'
+ $sum: ['$quiz']


I've checked all this variations from the doc examples.

GromNaN · 2024-01-25T14:29:42Z

generator/config/stage/setWindowFields.yaml

+ cumulativeQuantityForState:
+ $sum: '$quantity'
+ window:
+ documents: ['unbounded', 'current']


We need an enum for unbound / current...

Should this be ticketed under PHPLIB-1381?

I already have this list of enums in expression.php. I don't feel the need to create a ticket for each.

Noted. I missed those @todo comments.

Feel free to resolve any threads where I was asking about tickets, then -- unless there's some other open question.

GromNaN · 2024-01-25T14:30:54Z

tests/Builder/Stage/UnionWithStageTest.php

+ array_map(
+ fn ($year) => Stage::unionWith(
+ coll: 'sales_' . $year,
+ pipeline: new Pipeline(
+ Stage::set(
+ _id: (string) $year,
+ ),
+ ),
+ ),
+ range(2018, 2020),
+ ),


Experimentation to use array_map. But array unpacking can't be used in the middle of 2 other positional arguments. So I had to unpack the result of an array_merge.

The Pipeline class could expand lists of provided as argument.

new Pipeline( Stage::set(/* ... */), array_map(/* ... */), Stage::sort(/* ... */), );

See #57

For the time being, I'd format it manually the way it was done in the second test. I know that's a bit more effort to write, the ...array_merge() logic with array_map is a bit fancy to understand.

Ok, reverted.

tests/Builder/Stage/ProjectStageTest.php

GromNaN · 2024-01-25T14:37:31Z

generator/config/stage/out.yaml

- name: timeseries
- type:
- - object
- optional: true
- description: |
- If set, the aggregation stage will use these options to create or replace a time-series collection in the given namespace.


I removed this argument, since it needs to be inside the coll object. But I could not find an example where it is used. I don't remember where I found it.

I've no idea what this was referring to, as $out only talks about not being able to output to a time-series collection. I don't see any reference to it as an option.

Found here: Use the timeseries option with the $out aggregation stage

https://jira.mongodb.org/browse/PHPLIB-1386

Thanks. This looks like a docs omission, so I've opened DOCSP-36120. I'll defer to you if you'd like to re-introduce the timeseries option or defer it for a later time.

I removed this argument, since it needs to be inside the coll object.

Looking at the example you shared above, timeseries is a sibling of coll, not a child. So I see no reason it cannot be modeled in the YAML definition.

GromNaN · 2024-01-25T17:39:52Z

generator/config/stage/bucket.yaml

+ default: 'Other'
+ output:
+ count:
+ $sum: 1


Most examples use: $sum: 1 where $count: {} could be used.

I believe this is left over from times before $count was introduced.

where $count: {} could be used.

Just to confirm, you're referring to the $count aggregation operator, correct? The syntax is { $count: <string> }, so I wasn't sure if you were just abbreviating or referring to something else.

@jmikola the docs you linked to are for the $count aggregation stage. MongoDB 5.0 introduced a $count aggregation accumulator (with the same name to make the confusion perfect) that serves as a shortcut for { $sum: 1 }: $count (aggregation accumulator)

GromNaN · 2024-01-25T17:40:43Z

tests/Builder/Stage/CurrentOpStageTest.php

+ ),
+ Stage::match(
+ active: false,
+ transaction: Query::exists(true),


We could have true as default value. So that it can be written: Query::exists(),

Sounds like a good idea 👍

PHPLIB-1384

GromNaN · 2024-01-26T08:15:21Z

tests/Builder/Stage/SortStageTest.php

+ {
+ $pipeline = new Pipeline(
+ Stage::sort(
+ object(


The $sort operator will need some changes to be variadic object PHPLIB-1269. But this sort specs are shared with other operators as property. So we also need a SortSpec factory that returns an object.

I noticed that in a number of other examples. Ideally, we would be able to write this like so:

Stage::sort( age: -1, posts: 1, );

IIRC there were some other instances where we pass sort specifications, I'll dig into some pipelines to find those occurrences to see if this applies there as well.

generator/config/expression/sum.yaml

alcaeus · 2024-01-26T09:37:06Z

tests/Builder/Stage/FacetStageTest.php

+ $pipeline = new Pipeline(
+ Stage::facet(
+ ...[
+ 'categorizedByYears(Auto)' => new Pipeline(


Noted that this field is the reason for using the splat operator. I think it's fine to leave this in as an example when using this strategy can be helpful instead of fixing the example.

This is also a case where I think a comment explaining the necessity of splat is helpful (even if we repeat it throughout tests whenever this comes up).

alcaeus · 2024-01-26T09:40:45Z

tests/Builder/Stage/DensifyStageTest.php

+ new UTCDateTime(new DateTimeImmutable('2021-05-18T00:00:00.000Z')),
+ new UTCDateTime(new DateTimeImmutable('2021-05-18T08:00:00.000Z')),


Noted that UTCDateTime does not supporting creating a date from a time string. I created PHPC-2352 to track this.

alcaeus · 2024-01-26T09:43:11Z

generator/config/stage/fill.yaml

+ time: 1
+ output:
+ price:
+ method: 'linear'


Agree. This would look better:

Stage::fill( sortBy: object(date: 1), output: object( score: Fill::locf, price: Fill::linear, ) );

alcaeus · 2024-01-26T10:25:35Z

tests/Builder/Stage/OutStageTest.php

+ Stage::out(
+ object(
+ db: 'reporting',
+ coll: 'authors',
+ ),
+ ),


I dislike this signature, as passing an object seems overly complex. This is one of those cases where a separate (not auto-generated) encoder would work better, so we can essentially have the following signature:

function out(string $coll, ?string $db = null)

Related to #56 (comment)

tests/Builder/Stage/ProjectStageTest.php

tests/Builder/Stage/SortStageTest.php

alcaeus · 2024-01-26T10:34:56Z

tests/Builder/Stage/UnionWithStageTest.php

+ array_map(
+ fn ($year) => Stage::unionWith(
+ coll: 'sales_' . $year,
+ pipeline: new Pipeline(
+ Stage::set(
+ _id: (string) $year,
+ ),
+ ),
+ ),
+ range(2018, 2020),
+ ),


For the time being, I'd format it manually the way it was done in the second test. I know that's a bit more effort to write, the ...array_merge() logic with array_map is a bit fancy to understand.

jmikola

Publishing the first round of feedback.

jmikola · 2024-01-29T19:39:00Z

generator/config/stage/bucket.yaml

+ default: 'Other'
+ output:
+ count:
+ $sum: 1


where $count: {} could be used.

Just to confirm, you're referring to the $count aggregation operator, correct? The syntax is { $count: <string> }, so I wasn't sure if you were just abbreviating or referring to something else.

jmikola · 2024-01-29T19:49:29Z

generator/config/stage/collStats.yaml

+ $collStats:
+ latencyStats:
+ histograms: true


I agree it will seldom be used, if at all.

But it seems a bit arbitrary to use config here, as it isn't anything found in the $collStats API. Was it really to much to model the various options like you did with currentOp.yaml?

The options themselves don't seem terribly complex, so I don't see a big benefit to not modeling them unless we're concerned about the server API changing.

jmikola · 2024-01-29T19:59:38Z

generator/config/stage/densify.yaml

+ -
+ $densify:
+ field: 'timestamp'
+ range:


Did you mean to model the range option? I see you have object # Range above.

tests/Builder/Stage/DocumentsStageTest.php

generator/config/stage/facet.yaml

jmikola · 2024-01-30T00:10:30Z

generator/config/stage/merge.yaml

@@ -12,7 +12,7 @@ arguments:
 name: into
 type:
 - string
- #- OutCollection
+ - object # OutCollection


Is there an outstanding ticket to model OutCollection? This may relate to my earlier comment about modeling or typing collection and database names, since libraries may want to hook into that.

generator/config/stage/merge.yaml

jmikola · 2024-01-30T00:15:52Z

generator/config/stage/merge.yaml

+ whenMatched: 'replace'
+ whenNotMatched: 'insert'


I was just about to leave a comment on # WhenMatched in the option definition above.

Is this todo item ticketed?

jmikola · 2024-01-30T00:19:35Z

tests/Builder/Stage/MergeStageTest.php

+ db: 'reporting',
+ coll: 'orgArchive',
+ ),
+ on: ['dept', 'fiscal_year'],


I'm beating a dead horse here, but this left me wondering if some users might try to use fieldPath() methods here. Would that yield an error during static analysis?

The StringFieldPath is not accepted where string is expected.

But phpdoc types are not expressive enough to restrict an array of FieldPath.

Also @alcaeus if we want to be able to alias field names, we will need objects similar to FieldPath instead of only raw strings here.

PHPLIB-1382

if we want to be able to alias field names, we will need objects similar to FieldPath instead of only raw strings here.

Yes, I was thinking the same when I saw the code. We can add that when we look at integrating the builder into ODM itself.

@GromNaN feel free to resolve this.

generator/config/stage/merge.yaml

jmikola

Finishing with the second half of my review. Aside from individual, open comment threads, OutStage seems to be an outstanding thing to address.

jmikola · 2024-01-30T19:15:04Z

generator/config/stage/out.yaml

@@ -3,26 +3,38 @@ name: $out
 link: 'https://www.mongodb.com/docs/manual/reference/operator/aggregation/out/'
 type:
 - stage
-encode: object
+encode: single


In another comment, @alcaeus asked about creating custom encoder logic. Is that feasible with the current implementation, or is it more complicated because the entire OutStage file is generated (and thus would require addressing this through the generator code itself).

IIRC, the original intention of this project was to have an ongoing dependency on the generator, so I understand if there's presently no way to maintain stages and operators through PHP along (in an alternate reality where the generator was only used to create the initial implementation).

Asking the above more for my own curiosity.

I understand the current change to use encode: single here is the most flexible and probably sets you up to later use string|OutCollection for the parameter.

jmikola · 2024-01-30T19:16:17Z

generator/config/stage/out.yaml

- name: timeseries
- type:
- - object
- optional: true
- description: |
- If set, the aggregation stage will use these options to create or replace a time-series collection in the given namespace.


I've no idea what this was referring to, as $out only talks about not being able to output to a time-series collection. I don't see any reference to it as an option.

jmikola · 2024-01-30T19:20:17Z

tests/Builder/Stage/ProjectStageTest.php

+ ...['author.first' => 0],
+ ...['lastModified' => 0],


How are you using the splat operator twice, once for each positional argument? Wouldn't the correct syntax be: ...['author.first' => 0, 'lastModified' => 0]?

Both work. I used this syntax to keep 1 field per line.

TIL. There is seemingly no mention of this within PHP docs (just read through Function Arguments). Tested it myself: https://3v4l.org/sJ5KB#v8.3.2

jmikola · 2024-01-30T19:26:16Z

generator/config/stage/set.yaml

+ -
+ $set:
+ totalHomework:
+ # The $sum expression is always build as an array, even if the value is an array field name


Suggested change

# The $sum expression is always build as an array, even if the value is an array field name

# The $sum expression is always built as an array, even if the value is an array field name

Alternatively: "The $sum expression always builds as..."

Does the type of the referenced field factor in here? I assume ['$homework'] and $homework would still be computed the same and follows the $sum: Array Operand rules. In other words, the array wrapping has no effect other than visually being the long form API.

generator/config/stage/set.yaml

generator/config/stage/unionWith.yaml

jmikola · 2024-01-30T19:40:30Z

generator/config/stage/unset.yaml

+ $unset:
+ - 'isbn'
+ - 'author.first'
+ - 'copies.warehouse'


Same thought as #56 (comment) (re: fieldPath() usage), which you noted would be addressed by PHPLIB-1382.

generator/config/stage/merge.yaml

jmikola · 2024-01-30T19:45:56Z

generator/config/stage/merge.yaml

+ whenMatched: 'replace'
+ whenNotMatched: 'insert'


Should this be ticketed under PHPLIB-1381?

jmikola

GromNaN force-pushed the PHPLIB-1374 branch from 4ca7e63 to a1b956d Compare January 25, 2024 14:19

GromNaN commented Jan 25, 2024

View reviewed changes

GromNaN marked this pull request as ready for review January 25, 2024 14:31

GromNaN commented Jan 25, 2024

View reviewed changes

tests/Builder/Stage/ProjectStageTest.php Outdated Show resolved Hide resolved

GromNaN force-pushed the PHPLIB-1374 branch from a1b956d to dc9fdf6 Compare January 25, 2024 14:32

GromNaN requested review from jmikola and alcaeus January 25, 2024 14:32

GromNaN commented Jan 25, 2024

View reviewed changes

GromNaN force-pushed the PHPLIB-1374 branch from dc9fdf6 to 4fec58f Compare January 26, 2024 08:01

GromNaN commented Jan 26, 2024

View reviewed changes

alcaeus reviewed Jan 26, 2024

View reviewed changes

GromNaN force-pushed the PHPLIB-1374 branch 2 times, most recently from 57a0273 to 2cd0eb2 Compare January 26, 2024 14:11

GromNaN mentioned this pull request Jan 26, 2024

PHPLIB-1363 PHPLIB-1355 Add enum for $type query and $meta expression #38

Closed

jmikola reviewed Jan 30, 2024

View reviewed changes

GromNaN force-pushed the PHPLIB-1374 branch from 2cd0eb2 to 0fa04ac Compare January 30, 2024 08:54

jmikola reviewed Jan 30, 2024

View reviewed changes

GromNaN added 5 commits January 31, 2024 12:58

PHPLIB-1374 Add tests on all stages

7ddbdf1

Revert array_map

6b37286

Add parameters to $collStats

1b7ddfe

Fix bson date in Yaml

b556237

Update comment for $\unwind examples

248551d

GromNaN force-pushed the PHPLIB-1374 branch from d48df7e to 8f59d53 Compare January 31, 2024 13:00

Add comment for all examples converted to verbose form

a02228c

GromNaN force-pushed the PHPLIB-1374 branch from 8f59d53 to a02228c Compare January 31, 2024 13:01

jmikola approved these changes Jan 31, 2024

View reviewed changes

GromNaN merged commit ca173b2 into mongodb:0.1 Jan 31, 2024
5 checks passed

GromNaN deleted the PHPLIB-1374 branch January 31, 2024 16:06

		new UTCDateTime(new DateTimeImmutable('2021-05-18T00:00:00.000Z')),
		new UTCDateTime(new DateTimeImmutable('2021-05-18T08:00:00.000Z')),

	# The $sum expression is always build as an array, even if the value is an array field name
	# The $sum expression is always built as an array, even if the value is an array field name

PHPLIB-1374 Add tests on all stages #56

PHPLIB-1374 Add tests on all stages #56

Conversation

GromNaN commented Jan 24, 2024 • edited Loading

GromNaN Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

jmikola Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus Jan 26, 2024 • edited by jmikola Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN commented Jan 24, 2024 •

edited

Loading

GromNaN Jan 25, 2024 •

edited

Loading

jmikola Jan 29, 2024 •

edited

Loading

GromNaN Jan 25, 2024 •

edited

Loading

jmikola Jan 29, 2024 •

edited

Loading

jmikola Jan 31, 2024 •

edited

Loading

GromNaN Jan 26, 2024 •

edited

Loading

GromNaN Jan 31, 2024 •

edited

Loading

jmikola Jan 31, 2024 •

edited

Loading

alcaeus Jan 26, 2024 •

edited by jmikola

Loading

jmikola Jan 30, 2024 •

edited

Loading