Test and fix for issue #16998: SortExec shares DynamicFilterPhysicalExpr across multiple executions

[robertream]

Summary

Fixes #16998: SortExec shares DynamicFilterPhysicalExpr across multiple executions

This PR addresses an issue where SortExec was sharing the same DynamicFilterPhysicalExpr across multiple invocations of with_new_children (e.g., from reset_plan_states), causing recursive queries to fail.

Changes

• Fixed SortExec::with_new_children: Now properly clones the DynamicFilterPhysicalExpr instead of sharing the same instance across multiple executions
• Added comprehensive unit test: test_sort_exec_filter_cloning_issue_16998 that reproduces the issue and verifies the fix

Problem Description

The issue occurred when recursive queries would repeatedly execute the same SortExec plan. The shared DynamicFilterPhysicalExpr would maintain state across executions, leading to incorrect results. For example:

with recursive r as (
  select 0 as k, 0 as v
    union all
    (
      select *
      from r
      order by v
      limit 1
    )
)
select *
from r
limit 5;

This query would return only 2 rows instead of the expected 5 rows.

Solution

The fix ensures each SortExec instance gets its own copy of the dynamic filter by:

1. Creating a new DynamicFilterPhysicalExpr instance with the same children expressions
2. Preserving the current expression state or falling back to a true literal
3. Ensuring independent state management across multiple executions

Test Verification

• Added test_sort_exec_filter_cloning_issue_16998 that specifically tests the recursive query scenario
• All existing sort tests continue to pass
• The test fails when the fix is reverted, confirming it properly detects the issue

🤖 Generated with Claude Code

[xudong963]

Thank you @robertream , I enabled the CI process, will have a look later. /cc @adriangb

[adriangb]

As you can see the current behavior was intentional and necessary for this functionality. IMO we should add a .cloned() or with_new_state() method to ExecutionPlan that defaults to with_new_children() and we call that from places that don't want to preserve state.

[robertream]

I was vibe coding this one as an experiment, so if this isn’t the right fix, feel free to throw it away, I'm only out tokens, not labor/ :)

…

On Fri, Aug 1, 2025 at 9:32 PM Adrian Garcia Badaracco < ***@***.***> wrote: *adriangb* left a comment (apache/datafusion#17016) <#17016 (comment)> As you can see the current behavior was intentional and necessary for this functionality. IMO we should add a .cloned() or with_new_state() method to ExecutionPlan that defaults to with_new_children() and we call that from places that don't want to preserve state. — Reply to this email directly, view it on GitHub <#17016 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFTGTNXNGNAJP47GELTKD3LQ5N5AVCNFSM6AAAAACC6EQJXSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCNBWGIYDKNJSGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[adriangb]

I am unfortunately AFK until Monday at which point I'll have a pileup of work stuff. So Monday evening or Tuesday I'll try something. My suggestion would be a new method that explicitly copies an ExecutionPlan resetting any state.

[adriangb]

@robertream I don't think I can push to your branch so I opened #17028 with my change / suggestions. Could you take a look? We should either incorporate feedback here or proceed with my PR. Either way I'll give you attribution in the commits there.

[robertream]

@robertream I don't think I can push to your branch so I opened #17028 with my change / suggestions. Could you take a look? We should either incorporate feedback here or proceed with my PR. Either way I'll give you attribution in the commits there.

Go with your PR, mine was part of an experiment