nit: comparing by name isn't wrong here, but usually we use an AttributeSet, where .contains works by NameId. That'd avoid the dance where we start with a Set<Attribute> (not the same!) and then create a Set<String> from it.

I think this remark is still valid. In fact, I think this is a bug.

Because we use transformDown, we won't just traverse until we hit the TopN; we will traverse the whole plan and update every project we find.

And even if we only traverse until we hit the TopN, there may be projections on the way that don't have access to the renamed attributes (because another plan dropped them earlier).

I think we can do this instead, in two passes:

1. pass: update the topn (and place the eval over it)
2. pass: transformUp the plan and update only those projections whose inputs contain the renamed attributes, so these projections don't drop them.

Please, let's update the precise definition of this interface. I gave a suggestion above. This interface is implicitly "any command that we can hoist topn and limits past", but currently it says "can be commutated with limits" - which is both too much (LIMIT X | MY_COMMAND has to be the same as local LIMIT X | MY_COMMAND | LIMIT X) and too little (doesn't say anything about topns).

Rather than testing the whole optimizer chain, you can construct a specific logical plan and test the application of a single rule pass.

Alternatively, you can invent a test-only logical plan node that implements CardinalityPreserving and place a projection under it. The projection won't travel upwards because there's no pushdown rule for the new plan node.

That would be a failure since if somebody drops our attributes we can no longer carry them to topN copy. Unless there is a possibility of nodes dropping attributes and then magically un-dropping them, the verification step below should catch it I think?

The verification step will fail later downstream with a rather generic server error in this case ("optimized incorrectly due to missing attributes"). It'd be a tad nicer to check or assert that we're not creating a broken plan to begin with.

Consider an invented plan node NewProjection, which is silly because it does the same as a Project without being one. (Less silly would be plans that always output the same, fixed attributes; we don't have such plans, yet, but we could!)

If you have TopN BY field1, field2 | NewProjection | Project | remote Enrich, then placing the EVAL $$renamed$field1 = field1, $$renamed$field2 = field2 after TopN will not be sufficient, because the renamed attributes get dropped by NewProjection. If we now go and just stuff the $$renamed$fieldX attributes into the output of the Project - the remote Enrich will think "yay! I got the attributes I needed to pass to the new TopN!" but, in fact, our plan will just be deeply broken. And plan verification will catch this, but it'd be nicer to not create a broken plan to begin with.

This can be prevented by checking (or at least, asserting) that the InputSet of the Project still contains $$renamed$fieldX. For this to be correct, we need to transformUp (not down!) to propagate the renamed attributes through all the projects that may still be in the plan.

This is quite defensive, but it'd also tackle the problem that we didn't stop updating projects even below the original topn.

But your solution of just stopping early works, too, of course! So, please consider this a nit as of now.