ast/compile: use all vars from rule body for index candidates #3709
Conversation
| @@ -3674,6 +3679,11 @@ func TestCompilerBuildComprehensionIndexKeySet(t *testing.T) { | |||
| ] | |||
| } | |||
| `, | |||
| // Note: no comprehension index for line 7 (`z = [ ...`) | |||
| expected: exp{9: { | |||
There was a problem hiding this comment.
This expectation is new: the nested comprehension is now indexed because y, previously hidden in the surrounding body of the z comprehension, is no now taken into account.
There was a problem hiding this comment.
This looks correct to me. We can index the inner comprehension but not the outer one. There was an issue in the original indexing implementation that resulted in this test case/fix: #2497
| @@ -1772,8 +1772,8 @@ func (ci *ComprehensionIndex) String() string { | |||
|
|
|||
| func buildComprehensionIndices(dbg debug.Debug, arity func(Ref) int, candidates VarSet, rwVars map[Var]Var, node interface{}, result map[*Term]*ComprehensionIndex) uint64 { | |||
| var n uint64 | |||
| cpy := candidates.Copy() | |||
There was a problem hiding this comment.
I realized that nested bodies will see candidates from subsequent expressions because of how WalkBodies and the for-loop below interact... for example:
x = 7
y = {1 | ["foo" | ...]} # array comprehension nested inside set comprehension
z = 8In this case, the candidate set will contain x and z when the nested array comprehension body is processed.
This shouldn't be a problem in practice because if the nested array comprehension does not close over z then it won't matter. If it does close over z then z = 8 will be reordered before the outer set comprehension by an earlier stage.
ast/compile_test.go
Outdated
| @@ -3661,7 +3666,7 @@ func TestCompilerBuildComprehensionIndexKeySet(t *testing.T) { | |||
| wantDebug: 1, | |||
| }, | |||
| { | |||
| note: "skip: due to nested comprehension containing candidate", | |||
| note: "skip: due to nested comprehension containing candidate + indexed nested comprehension with key from rule body", | |||
There was a problem hiding this comment.
nit: "skip:" was intended for cases where no index gets built because it produce incorrect results, worsen performance, etc. Let's use a difference prefix ("mixed:"?) since an index is built now.
There was a problem hiding this comment.
nit: I'd also update the comment in the module that says 'y' disqualifies indexing ... to be a bit less ambiguous... it disqualifies indexing of the outer comprehension.
| @@ -3674,6 +3679,11 @@ func TestCompilerBuildComprehensionIndexKeySet(t *testing.T) { | |||
| ] | |||
| } | |||
| `, | |||
| // Note: no comprehension index for line 7 (`z = [ ...`) | |||
| expected: exp{9: { | |||
There was a problem hiding this comment.
This looks correct to me. We can index the inner comprehension but not the outer one. There was an issue in the original indexing implementation that resulted in this test case/fix: #2497
Before, we'd only looked at the vars preceding the comprehension in the body containing it. In the case of nested comprehensions, that would have excluded the rule body OUTSIDE of the nested body. Now, we'll accumulate candidates over multiple bodies -- capturing the ones that had been missing before. Fixes open-policy-agent#3579. Signed-off-by: Stephan Renatus <[email protected]>
|
Addressed the test comments |
srenatus
force-pushed
the
sr/issue-3579/comprehension-cache
branch
from
f88c9cc
to
7137b9a
Compare
…olicy-agent#3709) Before, we'd only looked at the vars preceding the comprehension in the body containing it. In the case of nested comprehensions, that would have excluded the rule body OUTSIDE of the nested body. Now, we'll accumulate candidates over multiple bodies -- capturing the ones that had been missing before. Fixes open-policy-agent#3579. Signed-off-by: Stephan Renatus <[email protected]>
…olicy-agent#3709) Before, we'd only looked at the vars preceding the comprehension in the body containing it. In the case of nested comprehensions, that would have excluded the rule body OUTSIDE of the nested body. Now, we'll accumulate candidates over multiple bodies -- capturing the ones that had been missing before. Fixes open-policy-agent#3579. Signed-off-by: Stephan Renatus <[email protected]> Signed-off-by: Dolev Farhi <[email protected]>
Before, we'd only looked at the vars preceding the comprehension in the body
containing it. In the case of nested comprehensions, that would have excluded
the rule body OUTSIDE of the nested body.
Now, we'll accumulate candidates over multiple bodies -- capturing the ones
that had been missing before.
Fixes #3579.
I had a more involved fix first, but making it work with the query compiler, I realised that moving the
candiates.Copy()does the trick, too.The change to the existing tests also covers the issue underlying #3579, I believe.