Repository navigation
Conversation
5404017 to
200cbda
Compare
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
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.
| % 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.
There was a problem hiding this comment.
That's a good find and a great performance optimization
| % 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?
There was a problem hiding this comment.
Yeah it's a bug fix, definitely. I think it might affect for example a partial index, if we skipped documents before and now they are added to the index we'd get an inconsistent index. Before they excluded some docs now they'll get extra. The same could be said about the a selector as a _changes filter. There we would have skipped and not replicated some documents but now it could be true so we'd allow extra documents through.
There was a problem hiding this comment.
An example of a filtered replication which change results based on the pr.
A mitigating factor for a continuous replication might be that I think replication id would change so old checkpoints won't be found it would be rewound. A completed replication obviously won't be re-run automatically.
A similar setup might affect partial index selector https://docs.couchdb.org/en/stable/ddocs/mango.html#find-partial-indexes
#!/bin/sh
http DELETE $DB/src
http DELETE $DB/tgt
http PUT $DB/src
http PUT $DB/tgt
http PUT $DB/src/doc1 name=a thingers:='[{"type":"veg"},{"type":"fruit"}]'
http PUT $DB/src/doc2 name=b thingers:='[{"type":"veg"},{"type":"rock"}]'
# thingers are all neither tools nor rocks
http PUT $DB/_replicator
http POST $DB/_replicate source=${DB}/src target=${DB}/tgt selector:='{"thingers": {"$allMatch": {"$nor": [{"type":"tool"}, {"type":"rock"}]}}}'
http $DB/tgt/_all_docs
### before VDU mango PR
# {
# "offset": 0,
# "rows": [],
# "total_rows": 0
# }
### after VDU mango PR
# {
# "offset": 0,
# "rows": [
# {
# "id": "doc1",
# "key": "doc1",
# "value": {
# "rev": "1-3d2d5bea046d1cc1e3ffb40b2de8411e"
# }
# }
# ],
# "total_rows": 1
# }15efda6 to
48f039a
Compare
48f039a to
190b164
Compare
When Mango selectors are normalized prior to being executed, one effect
is that negations are pushed down the expression tree or removed
altogether if a double-negation can be replaced with a simpler
equivalent expression.
In most cases this would result in the `$nor` operator being removed
entirely so that the evaluator never had to consider it. However there
are cases where this did not happen, e.g. inside `$allMatch`:
{
"x": {
"$allMatch": {
"y": {
"$nor": [
{ "$lt": 3 },
{ "$gt": 5 }
]
}
}
}
}
This expression would be unchanged by normalization and would fail to
evaluate when indexing. As part of the enhancements for returning
detailed match failure information to support using Mango for
`validate_doc_update`, we now normalize this to:
{
"x": {
"$allMatch": {
"$and": [
{ "y": { "$gte": 3 } },
{ "y": { "$lte": 5 } }
]
}
}
}
This means that some selectors that previously failed to match documents
in `_find` calls, filtered replication, etc, will now match. Previously,
a selector with `$nor` nested inside `$allMatch` would never match
anything, whereas now it will correctly match documents that meet the
selector's requirements.
Therefore, `_find` calls may return docs that they did not previously
include, and filtered replication may copy docs that were previously
filtered out. Any doc that *was* matched by a selector until now will
continue to match after this change.
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
190b164 to
ffd7a74
Compare
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.