fix: remap nested TopK filter expressions before their children - #26031
efegokdemir wants to merge 1 commit into
Conversation
| # specific language governing permissions and limitations | ||
| # under the License. | ||
|
|
||
| statement ok |
There was a problem hiding this comment.
The end-to-end SLT provides strong coverage for #26029. Since remap_children is also shared by HashJoin and projection/scan pushdown, would it be valuable to add a focused unit test that registers both a parent expression and one of its descendants, while also checking that an unmatched parent still allows descendant remapping? Non-blocking—the reported wrong-result path is already covered by the SLT.
rgbuilds
left a comment
There was a problem hiding this comment.
I traced the parent-first remapping behavior and the shared DynamicFilterPhysicalExpr consumers. transform_down with Jump correctly replaces the largest matching expression, preserves descendant remapping when the parent does not match, and continues processing sibling subtrees. The focused and broader TopK, dynamic-filter, Parquet-pushdown, physical-expression, formatting, and Clippy checks pass locally. No blocking issues found.
Which issue does this PR close?
Rationale for this change
TopK dynamic filtering can return incorrect rows when one sort expression contains another sort key. Child-first remapping changes the parent before it can be matched.
What changes are included in this PR?
Remap matching parent expressions before their descendants and stop traversing each replacement. Add the reported query as a SQL logic test.
What is the testing strategy for this PR?
cargo test --profile=ci --test sqllogictests -- topk_dynamic_filter_remap.sltcargo test --profile=ci -p datafusion-physical-expr --lib(1685 passed, 2 ignored)cargo fmt --all --checkcargo clippy --profile ci -p datafusion-physical-expr --lib -- -D warnings-A clippy::ignore-without-reasonfor an existing ignored test lacking a reason inplanner.rs.The broader
topk.sltfile could not be validated because this shallow checkout does not include thetestingsubmodule data.Are there any user-facing changes?
Yes. Queries matching this case now return the correct TopK rows; there is no API change.