Fix aggregate rows key error

[johnc1231]

Fixes #8316

I honestly don't know really know how this env/agg_env stuff works, I just know that this makes the tests pass. It's possible that this is an improvement but not a fully correct substitution rule, would appreciate if you could check it / tell me how you figured out what it's supposed to be.

[tpoterba]

hmm, I think this is wrong. I think it should be:

        return AggArrayPerElement(_subst(ir.array, env2=agg_env, agg_env2=None),
                                  ir.element_name,
                                  ir.index_name,
                                  _subst(ir.agg_ir, env2=_subst(delete(env, ir.index_name), delete(agg_env, ir.element_name),
                                  ir.is_scan)

[johnc1231]

Can you explain to me how env vs agg_env works? Like, why does ir.array's env2 become an agg_env?

[tpoterba]

we have three variable scopes: eval, scan, and agg. Suppose we have a node like TableAggregate, that supports aggregation:

TableAggregate
  table ir
  ApplyBinaryPrimOp
    +
    Ref x
    ApplyAggOp(Sum(), ... seqOpArgs = Seq( Ref y )

In the above, the x variable is in the eval scope, and y is in the agg scope. When we're traversing nodes with data structures that track variables defined/referenced, agg nodes need to do extra work to move variables between scopes. ApplyAggOp takes the agg env coming into it, and uses that agg env as the evaluation env for its seqOp args.

AggArrayPerElement uses the agg environment for its array argument, and passes down both an eval and agg env to its agg_ir child.

[tpoterba]

does this help?

[johnc1231]

I think I'm following, though I don't know what the index_name is for in the code you wrote above.

[johnc1231]

Also, in this line:

_subst(ir.agg_ir, env2=_subst(delete(env, ir.index_name), delete(agg_env, ir.element_name)

you're calling _subst on an env instead of an ir

[johnc1231]

@tpoterba

[tpoterba]

index_name is a variable exposed in the evaluation scope of agg_ir (the array element index) that can be used in the aggregation result operation.

I think in the above, I added an extra _subst around the env which can be deleted

Fixed

[tpoterba]

this subst shouldn't delete anything, I think. The array uses the aggregation environment, but AggArrayPerElement only binds index_name in the body.

[tpoterba]

we use UIDs everywhere so it's kinda possible that this can never cause trigger an error, but should fix it

Fixed