FilterExec should remap indices of parent dynamic filters

[jackkleeman]

Which issue does this PR close?

• Closes [Bug] [v52 regression] FilterExec parent dynamic filters have incorrect column indices #20285.

Rationale for this change

Parent dynamic filters should be remapped from input to output schema, otherwise the indices can be wrong

What changes are included in this PR?

• Use with_child to remap parent filters
• Add unit test that catches this issue

Are these changes tested?

Yes

Are there any user-facing changes?

No

[notashes]

nice fix! I was working on the same #20283! I'll close mine as this already contains tests!

[adriangb]

Great fix!

[adriangb]

Merging to get the fix in soon. @jackkleeman I wonder if we can reproduce this from an SLT test? Do you have a shape of query that you first noticed this with?

[jackkleeman]

I wonder if we can reproduce this from an SLT test? Do you have a shape of query that you first noticed this with?

I struggled because the datasources seem to do a column name remapping and are tolerant of this type of mismatch? it makes me wonder if i should be doing column name remapping too in my tableprovider

this shape should cover it select 1 from sys_invocation_status where partition_key is not null order by modified_at limit 5. ie, any filter on a column that isnt modified_at, and then a topk on modified_at. It needs dynamic filter pushdown, and the error won't occur until the dynamic filter is updated with a modified_at bound (the bound will be interpreted as partition_key > ).

[adriangb]

I struggled because the datasources seem to do a column name remapping and are tolerant of this type of mismatch? it makes me wonder if i should be doing column name remapping too in my tableprovider

I guess they are doing it to handle some cases and that makes them more resilient. In theory I'm not sure, maybe it's not needed. In practice it's probably a good safety net for you to do that as well.

[jackkleeman]

there might be an argument to disable that behaviour for the tests so that we catch these types of issues

[adriangb]

there might be an argument to disable that behaviour for the tests so that we catch these types of issues

IIRC it exists to adapt logical to physical schemas, which is a real thing / not paving over bugs. I'd guess it happens in https://github.com/apache/datafusion/blob/main/datafusion/physical-expr-adapter/src/schema_rewriter.rs.