ast: index a lookup into a collection in base data - #9235
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
39cbbc7 to
0616dd4
Compare
|
|
||
| Authored by @sueun-dev | ||
|
|
||
| ### Rule indexing sees a lookup into a collection in base data |
There was a problem hiding this comment.
Could this be folded into Rule indexing improvements above in it's entirety?
There was a problem hiding this comment.
Totally. Thought this was already gone 🙈
| @@ -439,6 +572,18 @@ type valueMapper struct { | |||
| } | |||
|
|
|||
| // refID identifies one of the references an index is built on. | |||
There was a problem hiding this comment.
I'd wager this should remain being attached to type refID int32 below.
| // evaluation was about to do -- the lookup being what the rule tests. | ||
| // | ||
| // Whether a caller stops is the caller's own business, not IndexResult.EarlyExit's: | ||
| // partial evaluation evaluates every candidate under a ruleset that permits stopping. |
There was a problem hiding this comment.
I'll admit the PE note here gave me a hint without needing to look for implementations of IndexEveryCandidateEvaluated; but that said, should we be keeping implementation specific info out of the indexer?
There was a problem hiding this comment.
Yeah I suppose so... I mean, the check is deliberately added to that PE behaves the way we want -- it used to be something like "does it EE?" but then it turned out that under PE, EE is ignored. So it took a turn towards "every candidate is evaluated" as the more-or-less direct opposite of EE 🔀
|
|
||
| // resolve answers for a reference, asking the resolver the first time only. | ||
| func (c *resolveCache) resolve(resolver ValueResolver, ref Ref) (Value, error) { | ||
| if c.keyOK && RefEqual(c.keyRef, ref) { |
There was a problem hiding this comment.
Are we banking on ref staying somewhat stable for a lookup, and not oscillating between values?
There was a problem hiding this comment.
Yes. This is also only for data refs -- they won't change within a single index lookup.
| } | ||
| // Base data read from JSON holds no set, but a store keeping ast.Value can, | ||
| // and so can a `with` statement replacing the collection. | ||
| case Set: |
There was a problem hiding this comment.
Couldn't key also be a number, for which we can do direct indexing into arrays; or am I missing some nuance here?
There was a problem hiding this comment.
Our array's Get would end up being a bounds check (like for objects, or sets, we don't check the value, only its existence, even if it's false). So to help with indexing, we'd only differentiate rule bodies referencing arrays of different lengths. That seems very uncommon. Let's revisit if someone raises a need for this. Sounds good?
`data.groups.g1.members[input.subject]` was recorded as "this reference must be
defined", so a ruleset of one rule per group had nothing to discriminate on. The
lookup is now recorded as what it asks -- the last element has to be a key of the
collection at the ground prefix -- and gather asks the collection: one hash lookup
into an object or a set, nothing stored per member, nothing to bound or configure.
`value in collection` asks for values rather than keys, which base data answers
only by being walked, and is left alone.
Only asked where every candidate is evaluated anyway. A ruleset whose definitions
agree on their value lets a caller stop at the first that holds, and gather would
have to ask about all of them to exclude any -- the work evaluation was about to
do, the lookup being what the rule tests. A resolver says its caller does not stop
by implementing IndexEveryCandidateEvaluated, as partial evaluation never does.
before after
partial eval, 100 rules 187577 ns/op 61043 ns/op
partial eval, 500 rules 658317 ns/op 173877 ns/op
500 rules, no early exit 356424 ns/op 149821 ns/op
500 rules, early exit 352908 ns/op 359272 ns/op
The index carries 0.50 MB against 0.46 MB for 500 rules over 50 groups of 20000
members each -- a million members, none of them on the trie.
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
0616dd4 to
c97b18f
Compare
The prefix and suffix matching merged earlier had no entry at all, and the indexer had collected two of its own. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
c97b18f to
30eb843
Compare
|
Benchmark comparison |
2 participants
data.groups.g1.members[input.subject]was recorded as "this reference must be defined", so a ruleset of one rule per group had nothing to discriminate on. The lookup is now recorded as what it asks -- the last element has to be a key of the collection at the ground prefix -- and gather asks the collection: one hash lookup into an object or a set, nothing stored per member, nothing to bound or configure.value in collectionasks for values rather than keys, which base data answers only by being walked, and is left alone.Note
Only asked where every candidate is evaluated anyway. A ruleset whose definitions agree on their value lets a caller stop at the first that holds (= EE), and
gather()would have to ask about all of them to exclude any -- the work evaluation was about to do, the lookup being what the rule tests. A resolver says its caller does not stop by implementing IndexEveryCandidateEvaluated, as partial evaluation never does.The index carries 0.50 MB against 0.46 MB for 500 rules over 50 groups of 20000 members each -- a million members, none of them on the trie.
Update I've pushed a second commit to brush up the CHANGELOG.md section on RI: the prefix/suffix was missing from it, and the other two sections took too much space. Now it's one RI section with a couple of related points, and a href to the docs.