PHPLIB-1254 Create Yaml for all operators and stages #1180

GromNaN · 2023-10-04T10:03:50Z

Operators and stages specifications

To create Yaml for all the stages and operators (query, pipeline), I started by filling a Google Sheet manually, from online documentation. Even if that took hours, it was a good learning. This document is temporary and will not be used after this PR.

The Yaml files are generated with the scrape command, that will be removed before merging.

The schema.json applies to Yaml files.

All operators and stages have an "encoding" type, that describes how to convert the object into BSON.

mongo-php-library/generator/config/schema.json

Lines 36 to 40 in 27c9f21

    
           "$comment": "Specifies how operator parameters are encoded.", 
        
           "$comment": "array: parameters are encoded as an array of values in the order they are defined by the spec", 
        
           "$comment": "object: parameters are encoded as an object with keys matching the parameter names", 
        
           "$comment": "single: get the single parameter value", 
        
           "$comment": "group: specific for $group stage",

Types

The types will be reworked in PHPLIB-1251.

Add AccumulatorInterface to type hint Group Accumulator Operators. It might be necessary to distinct the accumulators for each stage ($group, $setWindowFields...)
Add QueryInterface distinct query operators from expressions marked with ExpressionInterface.

Add the MongoDB\object function to create stdClass from variadic named arguments.

mongo-php-library/src/functions.php

Lines 62 to 71 in 27c9f21

    
           /** 
        
            * Create a new stdClass instance with the provided properties. 
        
            * 
        
            * @psalm-suppress MoreSpecificReturnType 
        
            * @psalm-suppress LessSpecificReturnStatement 
        
            */ 
        
           function object(mixed ...$values): stdClass 
        
           { 
        
               return (object) $values; 
        
           }

GromNaN

~~Very WIP~~

GromNaN · 2023-10-04T10:08:36Z

generator/src/Command/ScrapeCommand.php

+        $filename = $dirname . '/' . $name . '.yaml';
+
+        // Add a schema reference to the top of the file
+        $schema = '# $schema: ../schema.json' . PHP_EOL;


PHPStorm will support it soon.

Will be available in 2023.3 Release.

I committed .idea/jsonSchemas.xml for the time being.

GromNaN · 2023-10-04T10:09:23Z

generator/src/Command/ScrapeCommand.php

+
+    public function execute(InputInterface $input, OutputInterface $output): int
+    {
+        $index = file_get_contents('https://docs.google.com/spreadsheets/d/e/2PACX-1vROpGTJGXAKf2SVuSZaw16NwMVtzMVGH9b-YiMtddgZRZOjOO6jK2YLbTUZ0N_qe74nxGY9hYhUe-l2/pubhtml');


I published the Google Sheet publicly as an HTML document because csv and tsv doesn't have correct support for newline. Also, I can edit in Google and run my command without all the hassle of Google authentication.

GromNaN · 2023-10-05T06:56:45Z

generator/src/Command/ScrapeCommand.php

+
+use const PHP_EOL;
+
+final class ScrapeCommand extends Command


I'll probably remove this command before merging. The google sheet is a step for generating hundreds of Yaml file. But it is not suitable for day-to-day maintenance and versioning.

Would it potentially come in handy down the line to track necessary changes to YAML files? Drivers typically do not get notified of all updates to MQL (query and aggregation) syntax, so I think it's likely we'll fall behind on this and may want some tooling to compare docs against our own mappings.

It's all done by hand. Unless you use AI, I don't see how you can recreate this file simply to detect changes.

The form in which things are documented varies greatly and some informations where extracted from the server source code. I'd better maintain Yaml files and get notified of changes.

generator/src/Definition/OperatorDefinition.php

.github/workflows/benchmark.yml

alcaeus · 2023-10-06T09:14:55Z

generator/config/expressions.php

@@ -20,14 +24,44 @@ function typeFieldPath(string $resolvesTo): array
 }

 return [
+    'mixed' => ['scalar' => true, 'types' => ['mixed']],


mixed is very PHP-ish. Should we rename this to any to make it more applicable to other programming languages?

alcaeus · 2023-10-06T09:16:07Z

generator/config/expressions.php

+    'Regex' => ['scalar' => true, 'types' => [BSON\Regex::class]],
+    'Constant' => ['scalar' => true, 'types' => ['mixed']],
+    'Binary' => ['scalar' => true, 'types' => ['string', BSON\Binary::class]],
+


Any reason these are capitalised? If we're supporting all BSON types here, there's no reason to change the capitalisation of names.

alcaeus · 2023-10-06T09:18:01Z

generator/config/expressions.php

    ],
    DateFieldPath::class => typeFieldPath(ResolvesToDate::class),
+    ResolvesToTimestamp::class => [
+        'implements' => [ResolvesToInt::class],
+        'types' => ['int', BSON\Int64::class],


This should include the BSON\Timestamp classs.

alcaeus · 2023-10-06T09:23:39Z

src/Builder/BuilderEncoder.php

@@ -32,7 +32,10 @@ class BuilderEncoder implements Encoder
     */
    public function canEncode($value): bool
    {
-        return $value instanceof Pipeline || $value instanceof StageInterface || $value instanceof ExpressionInterface;
+        return $value instanceof Pipeline


We should definitely consider splitting this class.

GromNaN · 2023-10-06T08:30:37Z

.github/workflows/tests.yml

-          - "7.4"
-          - "8.0"
+          #- "7.4"
+          #- "8.0"


To be reverted by PHPLIB-1268

Did you come to a solution to still run PHPLIB against 7.4+ but limit the aggregation builder code to 8.1+?

GromNaN · 2023-10-06T08:31:28Z

generator/composer.json

@@ -18,6 +18,9 @@
        "mongodb/mongodb": "@dev",
        "nette/php-generator": "^4",
        "symfony/console": "^6.3",
+        "symfony/css-selector": "^6.3",
+        "symfony/dom-crawler": "^6.3",


Remove when the scrape command is removed.

GromNaN · 2023-10-06T10:31:42Z

generator/src/Definition/OperatorDefinition.php

+        };
+
+        // Convert arguments to ArgumentDefinition objects
+        // Optional arguments must be after required arguments


The optional argument have to move to the end, as PHP doesn't support optional arguments before required ones.

That doesn't matter for "object" encoding, but this is an issue for "array" encoding since the order of arguments matter.

$slice have an optional argument in the middle of required arguments.

To be reworked in PHPLIB-1274

$slice have an optional argument in the middle of required arguments.

This sadly dates back to server version 3.2. I hope we've learned not to make such mistakes in the future.

Can we ask the server team to allow an object here?

They certainly can't change the array order due to BC reasons.

Argument lists for operators are typically lists, so I don't imagine using an object would be consistent. Even if such a change was made, we'd still have to support the legacy array format for older server versions, which in turns means we'd need to implement version checking here. AFAIK, all of the query syntax is modeled irrespective of the server version, so I'd rather not open that can of worms.

I expect $slice will just need to be handled specially.

GromNaN · 2023-10-06T11:47:23Z

generator/config/aggregation-stages/collStats.yaml

+    -
+        name: config
+        type:
+            - object


I did not modelize the full object. Let's use object function for that. People will have to read the documentation anyway.
I could have made it a variadic map.

Aye, this agrees with what we do elsewhere for certain operations (e.g. ModifyCollection).

GromNaN · 2023-10-06T11:48:20Z

generator/config/aggregation-stages/densify.yaml

+    -
+        name: range
+        type:
+            - Range


range model is a plain object for now. Not modelized.

For cases where we are choosing not to model the object, can we incorporate links in the YAML so that we get @see references in the generated code? Then, if folks happen to view the generated operator file they can be directed to the server documentation.

I added a @see comment with the link to the doc on each operator class header, and factory method.

GromNaN · 2023-10-06T11:52:26Z

generator/config/aggregation-operators/firstN.yaml

@@ -0,0 +1,23 @@
+# $schema: ../schema.json
+name: $firstN


I have a name conflict with $firstN that is both aggregation accumulator and array operator. Same for $lastN.

Both of these fall under aggregation, correct? And not the query namespace.

The API for both of these operators looks similar, but I assume they are distinct and there's nothing to say other operators and accumulators with the same name might differ more significantly. In this case, would it make sense to organize accumulators under their own namespace?

I actually notice that there are two groupings for accumulators in the docs:

Accumulators ($group, $bucket, $bucketAuto, $setWindowFields)

Accumulators (in Other Stages)

It's hard to predict how we might encounter naming conflicts in the future, but do you think it'd be worth organizing the many aggregation operators by separate namespaces to group them logically?

We could also do the same for Query and Projection operators, which have far fewer categories, if you like.

If we over-categorize, it could be confusing for developers, who will have to guess which category we've put the operator they want to use.
Also, I don't want to duplicate Yaml files for operator what would be in multiple categories, like $avg or $count.

Is this file modeling the array operator or accumulator? The link and description fields below suggest the array operator , but I see type: [ Accumulator ] as well.

Is this file modeling the array operator or accumulator? The link and description fields below suggest the array operator , but I see type: [ Accumulator ] as well.

I can see how over-categorization could be problematic (assuming users need to reference class names and don't use factory methods). What is your proposed solution?

GromNaN · 2023-10-09T08:53:45Z

src/Builder/BuilderEncoder.php

+    private function recursiveEncode(mixed $value): mixed
+    {
+        if (is_array($value)) {
+            foreach ($value as $key => $val) {
+                $value[$key] = $this->recursiveEncode($val);
+            }
+
+            return $value;
+        }
+
+        if ($value instanceof stdClass) {
+            foreach (get_object_vars($value) as $key => $val) {
+                $value->{$key} = $this->recursiveEncode($val);
+            }
+        }
+
+        return $this->encodeIfSupported($value);


This function is all about the ArrayCodec and ObjectCodec that were described in the codec architecture doc.
https://github.com/mongodb/mongo-php-library/pull/1125/files#diff-8bbcff6724826a3ca2b160e0718dbee5520987bbaa22e78eda96d0d448cad5e8R27

jmikola · 2023-10-06T20:17:51Z

.github/workflows/tests.yml

-          - "7.4"
-          - "8.0"
+          #- "7.4"
+          #- "8.0"


Did you come to a solution to still run PHPLIB against 7.4+ but limit the aggregation builder code to 8.1+?

jmikola · 2023-10-06T20:18:35Z

.idea/jsonSchemas.xml

@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="UTF-8"?>


Interestingly, this got picked up as a generated file in GitHub:

Some generated files are not rendered by default.

I realize it's temporary, but that doesn't seem right.

jmikola · 2023-10-06T20:20:36Z

examples/aggregation-builder.php

@@ -48,15 +50,15 @@ function toJSON(object $document): string
        totalCount: Aggregation::sum(1),
        evenCount: Aggregation::sum(
            Aggregation::mod(
-                Expression::fieldPath('randomValue'),
+                Expression::numberFieldPath('randomValue'),


Is numberFieldPath required over fieldPath since the code would expect ResolvesToNumber here? Do users still have the choice of passing an arbitrary field path as a string?

Looking at this now, I can see how these might be annoying if users must use type in the query builder.

That's a request from @alcaeus in the Technical Design doc.

To complete the type experience, there are also typed fieldPath helpers (e.g. stringFieldPath) which communicate to the type system that the field path resolves to the given type. This can again be used by an ORM to verify that the field will actually resolve to the given type (if known).

jmikola · 2023-10-10T20:49:24Z

tests/Builder/BuilderEncoderTest.php

        );

        $expected = [
            [
                '$match' => [
                    '$or' => [
-                        ['score' => ['$gt' => 70, '$lt' => 90]],
+                        // same as ['score' => ['$gt' => 70, '$lt' => 90]],
+                        ['score' => [['$gt' => 70], ['$lt' => 90]]],


How are these equivalent? One is matching a field against an array, which I expect would be an exact match. Consider:

> db.foo.insertOne({score:10}) > db.foo.insertOne({score:80}) > db.foo.insertOne({score:[{$gt:70},{$lt:90}]}) > db.foo.aggregate([{$match:{$or:[{score:[{$gt:70},{$lt:90}]}]}}]) [ { _id: ObjectId("6525b7d5f411a41fea45873b"), score: [ { '$gt': 70 }, { '$lt': 90 } ] } ] > db.foo.aggregate([{$match:{$or:[{score:{$gt:70,$lt:90}}]}}]) [ { _id: ObjectId("6525b859f411a41fea45873c"), score: 80 } ]

The document with {score: 10} is never matched, as it doesn't satisfy both range operators. In the case where the range operators are passed separately in an array, the server performs an equality match.

It was too easy... to be correct. I reworked the implementation to generate the expected result.

src/Builder/Type/QueryInterface.php

jmikola · 2023-10-10T21:09:22Z

src/Builder/Stage/UnsetStage.php

+     */
+    public function __construct(FieldPath|string ...$field)
+    {
+        if (\count($field) < 1) {


Is it expected that generated files reference global functions this way? It's inconsistent with what we do in the hand-written sources, so if it's easily configurable it may be worth changing.

Added $namespace->addUseFunction calls to add this. The code generator don't know about the function body, so it cannot be detected.

GromNaN · 2023-10-16T15:39:09Z

Replaced by mongodb/mongo-php-builder#1

GromNaN commented Oct 4, 2023

View reviewed changes

Create Yaml files from Google Sheet

6fe9809

GromNaN force-pushed the PHPLIB-1254 branch from d76fbdf to 6fe9809 Compare October 4, 2023 18:55

Import all operators and stages

204e88f

GromNaN commented Oct 5, 2023

View reviewed changes

generator/src/Definition/OperatorDefinition.php Outdated Show resolved Hide resolved

GromNaN added 7 commits October 5, 2023 11:17

Add @no-named-argument to list variadic

b3c6279

Improve typing for variadics

59f360c

Add link and description to classes and factory

64ee28e

Temporary disable benchmarks and old PHP version from CI

05c5ee5

Fix & encoding

83aa77a

Fix schema

9a36d17

Introduce MongoDB\object() function

c0de34d

GromNaN force-pushed the PHPLIB-1254 branch from 692f1b3 to c0de34d Compare October 5, 2023 21:32

GromNaN added 7 commits October 6, 2023 00:03

Fix single encoding + optional value in array

70f8671

Fix psalm + accumulator

38c7750

Case insensitive sort of methods

5733833

Fix $sum accumulator test

6faa7f8

Ignore psalm issues

27c9f21

Improve doc for object function

502ed6b

Fix methods order

3f175e7

GromNaN marked this pull request as ready for review October 6, 2023 07:39

GromNaN requested review from alcaeus and jmikola October 6, 2023 07:41

alcaeus reviewed Oct 6, 2023

View reviewed changes

GromNaN commented Oct 6, 2023

View reviewed changes

GromNaN added 2 commits October 6, 2023 14:03

Remove tests on PHP 8.0

d89d024

Rename mixed to any and remove incorrect generic type from list

2129f88

GromNaN commented Oct 9, 2023

View reviewed changes

Improve operator classification and add all BSON types

06c5294

jmikola reviewed Oct 10, 2023

View reviewed changes

GromNaN added 18 commits October 11, 2023 00:37

Rename Expression Operators and write architecture document

a5840c0

Implement Regex factory

99e2266

Remove generated warning on non-generated files

636aa4a

Add use function for generated code

5bc3cd8

Refinement of query type with a new filter type

bdfbcc5

Move Projection operators to a dedicated namespace

c7bc14c

Move Optional & Encode enum to the Type namespace

35dbd63

Move Accumulator and Window operators in a dedicated namespace

00b3cd3

Remove category from Yaml files and remove scrape command

dd28e4a

Add some errors for incorrect types

58fd4d0

Rename filter to fieldQuery

13d29a8

exclude accumulator from QA

e51b16f

Make FieldPath implement ResolveToAny

c0790e0

Remove unused packages

960eaa4

Make operator classes read-only

120f530

Add tests on Pipeline class

57555b9

Rename PhpObject enum

fee0df6

Use BSON\Type and BSON\Javascript classes, remove duplicate resolveToInt

ad48be4

GromNaN mentioned this pull request Oct 16, 2023

Init project mongodb/mongo-php-builder#1

Merged

GromNaN closed this Oct 16, 2023

GromNaN deleted the PHPLIB-1254 branch October 16, 2023 15:41

	"$comment": "Specifies how operator parameters are encoded.",
	"$comment": "array: parameters are encoded as an array of values in the order they are defined by the spec",
	"$comment": "object: parameters are encoded as an object with keys matching the parameter names",
	"$comment": "single: get the single parameter value",
	"$comment": "group: specific for $group stage",

	/**
	* Create a new stdClass instance with the provided properties.
	*
	* @psalm-suppress MoreSpecificReturnType
	* @psalm-suppress LessSpecificReturnStatement
	*/
	function object(mixed ...$values): stdClass
	{
	return (object) $values;
	}

PHPLIB-1254 Create Yaml for all operators and stages #1180

PHPLIB-1254 Create Yaml for all operators and stages #1180

Conversation

GromNaN commented Oct 4, 2023 • edited Loading

Operators and stages specifications

Types

GromNaN left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

jmikola Oct 6, 2023 • edited by GromNaN 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

GromNaN Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola Oct 6, 2023 • 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

GromNaN commented Oct 16, 2023

GromNaN commented Oct 4, 2023 •

edited

Loading

GromNaN left a comment •

edited

Loading

GromNaN Oct 6, 2023 •

edited

Loading

jmikola Oct 6, 2023 •

edited by GromNaN

Loading

GromNaN Oct 6, 2023 •

edited

Loading

jmikola Oct 6, 2023 •

edited

Loading

jmikola Oct 6, 2023 •

edited

Loading