CooperativeExec incorrectly implements maintains_input_order #16994

Activity

1. pepijnve

   added

   bugSomething isn't workingSomething isn't working

   on Jul 31, 2025
2. pepijnve

   added a commit that references this issue on Jul 31, 2025

   apache#16994 Ensure CooperativeExec#maintains_input_order returns a V…

   ...

   ee9ccf6
3. pepijnve commented on Jul 31, 2025

   

   pepijnve

   on Jul 31, 2025

   Last edited by pepijnve

   ContributorAuthor

   More actions

   I came up with this as unit test, but that pretty much duplicates the implementation. Should I add this test case or is it not worth it?

   What might be useful is to have more general test logic that takes in an ExecutionPlan and verifies (to the extent possible) that the post conditions required by the trait are met. For instance that the length of the various Vecs is correct. This can't check for correctness of the values (i.e. true or false in this case), but it can check for wellformedness of the result.

   #[test]
   fn maintains_input_order() -> Result<()> {
       let schema = Arc::new(Schema::empty());
       let exec = LazyMemoryExec::try_new(schema, vec![])?;
       let coop = CooperativeExec::new(Arc::new(exec));
       let maintains_input_order = coop.maintains_input_order();
       assert_eq!(maintains_input_order, vec![true;coop.children().len()]);
       Ok(())
   }
   
4. alamb

   added

   regressionSomething that used to work no longer doesSomething that used to work no longer does

   on Jul 31, 2025
5. alamb commented on Jul 31, 2025

   

   alamb

   on Jul 31, 2025

   Contributor

   More actions

   What might be useful is to have more general test logic that takes in an ExecutionPlan and verifies (to the extent possible) that the post conditions required by the trait are met. For instance that the length of the various Vecs is correct. This can't check for correctness of the values (i.e. true or false in this case), but it can check for wellformedness of the result.

   @wiedld started adding something like this with the ExecutionPlan::check_invariants:

   https://docs.rs/datafusion/latest/datafusion/physical_plan/trait.ExecutionPlan.html#method.check_invariants

   Maybe we can use that mechanism?
6. alamb commented on Jul 31, 2025

   

   alamb

   on Jul 31, 2025

   Contributor

   More actions

   Or we can extend here https://github.com/apache/datafusion/blob/c6d55207161e400e53645d5ee7d7bf16cd024c2f/datafusion/core/src/physical_planner.rs#L2356-L2355 with some basic checks
7. pepijnve

   added 3 commits that reference this issue on Aug 1, 2025

   apache#16994 Extend default ExecutionPlan invariant checks

   ...

   6232474

   apache#16994 Extend default ExecutionPlan invariant checks

   ...

   f3d78cf

   apache#16994 Extend default ExecutionPlan invariant checks

   ...

   74b153d
8. alamb

   added a commit that references this issue on Aug 4, 2025

   #16994 Ensure CooperativeExec#maintains_input_order returns a Vec of …

   ...

   2968331
9. alamb

   closed this as completedin #16995on Aug 4, 2025
10. pepijnve

    added a commit that references this issue on Aug 7, 2025

    apache#16994 Ensure CooperativeExec#maintains_input_order returns a V…

    ...

    2f11409
11. xudong963

    added a commit that references this issue on Aug 7, 2025

    #16994 Ensure CooperativeExec#maintains_input_order returns a Vec of …

    ...

    f6ec4c3
12. pepijnve

    added a commit that references this issue on Aug 11, 2025

    apache#16994 Ensure CooperativeExec#maintains_input_order returns a V…

    ...

    ef3e35b
13. hknlof

    added a commit that references this issue on Aug 20, 2025

    apache#16994 Ensure CooperativeExec#maintains_input_order returns a V…

    ...

    ddeb54a