Use Scopes to resolve lambda argument references in ExpressionAnalyzer

[findepi]

Use Scope to resolve lambda arguments in ExpressionAnalyzer.

Fixes #9023, #7784

For #7790 following things are still needed:

• AggregationAnalyzer needs similar update
• This doesn't fix Wrong query results when lambda scope shadows column name #9025 (QueryPlanner?)

[sopel39]

Duplicate of #7790? I am not sure it is possible to use Scopes only in ExpressionAnalyzer without modifying other classes that resolve lambda arguments (I don't think arguments resolution would behave consistently then).

[findepi]

This is related to #7790 -- a step towards that direction.

I agree there is risk of inconsistency. However, there already is some inconsistency (#9025) even though scopes were used nowhere, so I think this still is a good step in a good direction.

[findepi]

@martint , did you have time to look into this?

[martint]

I did. The code looks good, but I need to think about the comment @sopel39 made above regarding inconsistencies. It'd be great if we can find concrete examples where it wouldn't work as expected (beyond the once you listed in #9025)

[findepi]

@sopel39 , did you have anything specific in mind?

[sopel39]

Resolution works differently than in TranslationMap (although it might be consistent. However this is a tricky code with edge cases). Why can't you simply make ExpressionAnalyzer.Visitor#visitIdentifier to behave like the one in TranslationMap#translateNamesToSymbols? This way we wouldn't introduce second (possibly inconsistent) resolution method for lambda arguments.

[findepi]

TranslationMap#translateNamesToSymbols already does it differently than ExpressionAnalyzer, see my today comments in #9025.

Can we have an example of a query that will work incorrectly after this change and used to work correctly or at least fail before?

[sopel39]

Can we have an example of a query that will work incorrectly after this change and used to work correctly or at least fail before?

I am not sure (I haven't deeply though about it), but this doesn't mean that there won't be any new regressions. It seems that it would be easier to make both TranslationMap and ExpressionAnalyzer to behave consistently for now (using current lambda resolution mechanism) instead of using yet another resolution method in ExpressionAnalyzer .

Then after #7398 we could switch completely to scopes for lambda resolution.

[findepi]

@sopel39 but the problem is in master they both do wrong (differently). After this PR, ExpressionAnalyzer will correctly resolve lambda references and captures within lambda and translation map needs to follow (I'm working on it currently) or abstain from any use of scopes (#7398).

I did a quick look into doing #7398, but the code seems both complex and 'there is room for improvement'-ish.

[sopel39]

translation map needs to follow

Yeah, but then you probably should use Scope for lambda arguments resolution in TranslationMap too (in order to be consistent). I am just not sure it's worth the work since Scopes would be removed from actual planning.

[sopel39]

but then you probably should use Scope for lambda arguments resolution in TranslationMap too

Or maybe couple of TODOs would do for now too. I don't have strong opinion either way.

[findepi]

@martint , @sopel39
i created follow-up PR that fixes TranslationMap and thus fixes #9025: #9090. This is not much code so it doesn't get us away from #7398 (it doesn't get us closer either).

[sopel39]

Yeah, but then you probably should use Scope for lambda arguments resolution in TranslationMap too

This is probably not true after all. I think in TranslationMap you can just check if node is column ref or lambda argument ref.

[findepi]

@sopel39 let's move the discussion related to #9090 there. Do you have any other comments relating this PR? Can @martint merge it?

[sopel39]

I haven't reviewed it yet (I don't know if I will have time). However, the approach to use Scope in ExpressionAnalzyzer seems legit. I would avoid using it in TranslationMap though. Generally, it appears to be easier to switch to Scope for lambda args resolution than I thought.

[findepi]

@sopel39 i don't think you need to review this thoroughly, as @martint already did.

[findepi]

Superseded by #9099