Mango match_failures/2 function - #6080
Conversation
5404017 to
200cbda
Compare
nickva
left a comment
nickva
left a comment
There was a problem hiding this comment.
Looks great. Almost ready to merge!
Just a few more comments, a way to generalize the error a bit maybe, and a comment about always validating mango VDUs to remove a footgun from the user.
Also wondering if we can re-run the perf tests with the latest shape since we more clauses to handle.
200cbda to
384cd8e
Compare
384cd8e to
15efda6
Compare
nickva
left a comment
nickva
left a comment
There was a problem hiding this comment.
Good work! It's almost ready to merge. Just a few minor comments about docs and main thing that's interesting to look at is the performance. We changed quite a bit since the initial shape so it would be interesting to see if it impact performance. Ideally for the non-verbose case it would stay the same or be insignificant.
| .. versionadded:: TODO | ||
|
|
||
| When set to ``true``, the ``validate_doc_update`` field will be | ||
| validated when design documents are updated. For ``javascript`` design |
There was a problem hiding this comment.
For "javascript" (default) language only. "query" is always validated.
There was a problem hiding this comment.
Thanks for spotting this, I'll correct it in the next push.
| "converting a Mango VDU to JavaScript updates its effects", | ||
| "deleting a Mango VDU removes its effects", | ||
| "Mango VDU rejects a doc if any existing ddoc fails to match", | ||
| "invalid Mango VDU is detected on doc update", |
There was a problem hiding this comment.
This test was removed but we left the title in it seems. Maybe it can be a test of a "query" VDU language but the function is a javascript string one (or just any string) or other type.
There was a problem hiding this comment.
There is another test in this suite that checks query VDUs are validated, regardless of the config setting, so I'm removing this title.
| % bad_path in which case matching fails. | ||
| match({[{Field, Cond}]}, Value, Cmp) -> | ||
| match({[{Field, Cond}]}, Value, #ctx{verbose = Verb, path = Path} = Ctx) -> | ||
| InnerPath = extend_path(Field, Path), |
There was a problem hiding this comment.
If we extend even for non-verbose case we should benchmark a few case which exercise this path with verbose=false (default) with before and after the PR
There was a problem hiding this comment.
I've run the couple of benchmarks I put in before doing this work, against the main branch, against this branch as-is, and after a change to only modify the Ctx if Verb = true; these are the results (higher is better, each column is a set of readings and the mean value):
apache/main:
and: normal | and: verbose | allMatch: normal | allMatch: verbose
------------- | --------------- | ------------------- | -----------------
3373312 | n/a | 2116779 | n/a
3506853 | | 2116407 |
3177151 | | 1887538 |
3381903 | | 1948357 |
3615163 | | 2080386 |
------------- | --------------- | ------------------- | -----------------
3410876 | | 2029893
this branch:
and: normal | and: verbose | allMatch: normal | allMatch: verbose
------------- | --------------- | ------------------- | -----------------
3219631 | 601439 | 3085928 | 647014
3077505 | 594021 | 3105148 | 642489
3088902 | 591180 | 3163921 | 647481
3551507 | 687608 | 3034273 | 673736
3208026 | 598050 | 3069272 | 662754
------------- | --------------- | ------------------- | -----------------
3229114 | 614459 | 3091708 | 654694
with InnerCtx optimisation:
and: normal | and: verbose | allMatch: normal | allMatch: verbose
------------- | --------------- | ------------------- | -----------------
3534877 | 618352 | 3385474 | 624247
3786131 | 686324 | 3426165 | 692269
3582253 | 662680 | 3417704 | 779013
3779435 | 666856 | 3419047 | 754569
3634909 | 657130 | 3471058 | 673272
------------- | --------------- | ------------------- | -----------------
3663521 | 658268 | 3423889 | 704674
Without the optimisation, $and runs ~5% slower and $allMatch ~50% faster. With a change to how InnerCtx is handled, $and is ~7% faster than main while $allMatch is almost 70% faster.
It makes sense that $and would have been impacted by this b/c {x: {$and: [A, B, C]}} normalises to {$and: [{x: A}, {x: B}, {x: C}]} i.e. field accesses are pushed down the tree and get duplicated as a result. So, optimisations to field accesses can have a major impact.
The updates I'm pushing include a new commit that only changes the Ctx value if Verb = true.
| % can't be pushed down through its corresponding operator, | ||
| % e.g. {x: {$allMatch: S}}. Negation inside the $allMatch | ||
| % should still be normalized. | ||
| norm_negations({[{Field, Cond}]}) when is_list(Field) -> |
There was a problem hiding this comment.
This is a behavior change in indexing/filtering? And I think it applies to non-verbose case too? It's a bug fix but we could still call it out (maybe someone relied on the bug for indexing/filtering).
There was a problem hiding this comment.
My understanding is this should strictly be considered a bug fix, i.e. normalisation should not in general alter the meaning of an expression, but it does remove certain forms that match would otherwise have to consider. One such effect is that $nor was expected to be fully removed from the expression and match does not have to handle it at all, but this was not the case if $nor appeared inside an $allMatch expression since normalisation did not recurse into it.
So the behaviour change is that expressions that did not work at all should now work. I'm not aware of any kind of expression that previously worked, and now has a different effect because of this change, such that applications might be relying on the old behaviour and would break after this.
Am I missing something?
Rather than returning a boolean to indicate just success or failure, `mango_selector:match/2` now returns a list of "failures" describing the ways in which the selector failed to match the input. If this list is empty, the match was a success.
We will need to pass other things around between `match` calls as well the current `Cmp` function, so here we replace this argument with a `#ctx` record that intially just contains a `cmp` field.
To give detailed feedback to the caller, the `#ctx` argument to `mango_selector:match/3` now records the path that was taken to reach each value, and this path is added to the `#failure` records. Each path segment is either a binary, if it represents an object property, or an integer if it represents an array index. Items are pushed on the front of `#ctx.path` as this is faster than pushing onto the back of a list. This list can then be reversed once the final list of failures has been generated, before the failures are presented to the caller.
Collecting detailed `#failure` records rather than a boolean true/false
when evaluating selectors imposes a performance penalty, so we would
like to only do this when a selector is used for a VDU, not when it is
used for indexing/filtering.
To this end we introduce "verbose" mode signalled via the `#ctx.verbose`
field, and each branch of `mango_selector:match/3` now has 3 distinct
versions:
- `#ctx{verbose = false}`: this is the original version that returns
true/false, taken when a selector is used for Mango queries.
- `#ctx{verbose = true, negate = false}`: verbose mode, when the
operator is not negated by an enclosing `$not` operator. Returns a
list of `#failure` records which may be empty.
- `#ctx{verbose = true, negate = true}`: verbose mode, when the operator
is negated by an enclosing `$not` operator. Returns a list of
`#failure` records.
The different negation modes are needed because, in order to generate
meaningful failure messages, we need to record whether an operator was
negated. The behaviour of combinators like `$and`, `$or`, `$allMatch`
and `$elemMatch` means not all `$not` operators can be normalized out of
the selector before evaluation. Instead, when we encounter a `$not`
during evaluation, we flip the `#ctx.negate` field before evaluating the
inner operator.
Until now, document updates rejected by a Mango VDU returned an opaque "forbidden" message to the client. This commit adds a detailed list of failures, obtained by converting the `#failure` records returned by `mango_selector:match/3` into human-readable messages.
…ct if the doc is newly created
Currently, when a design doc is updated, we validate the `map` and `reduce` fields, but not `validate_doc_update`. Instead, trying to update any other doc while an invalid `validate_doc_update` exists will trigger an error. This comment makes VDU validation more 'eager' by performing it when the ddoc itself is updated. Normal doc writes will still trigger an error if an invalid `validate_doc_update` already exists, but now we try to prevent this happening by validating VDUs when they are first created.
…keyMapMatch") should be considered successful when applied to values of the wrong type
…lements when reporting VDU failures
… non-verbose mode
15efda6 to
48f039a
Compare
2 participants
This PR has the same content as #5858, it is just in a branch in the
apacherepo to see if this resolves problems with CI.