Skip to content

Conversation

timsaucer
Copy link

I think all of the work on the case builder seems to be because we cannot simply clone the underlying CaseBuilder. This approach basically caches the required values and then calls CaseBuider as needed. I think it greatly simplifies the code. I also think some of the unit tests were not producing the expected results, so I update them.

Please let me know what you think @kosiew

@timsaucer
Copy link
Author

Also apache/datafusion#17927 should clean this up nicely.

@kosiew
Copy link
Owner

kosiew commented Oct 6, 2025

apache/datafusion#17927 is a good idea.

In the meantime, I think we should exclude CaseBuilder from apache#1253.
Instead we can add a follow-up task to make freeze CaseBuilder after apache/datafusion#17927 arrives because there is behaviour change.
The new implementation mutates a cloned copy and returns it, leaving the original builder untouched:

pub fn when(&self, when: PyExpr, then: PyExpr) -> PyCaseBuilder {
    println!("when called {self:?}");
    let mut case_builder = self.clone();
    case_builder.when.push(when.into());
    case_builder.then.push(then.into());

    case_builder
}

Before apache#1253 we wrote the updated builder back into self, so callers could safely ignore the return value (builder.when(...); builder.otherwise(...)) and still observe the new arm.

With the new implementation those calls silently do nothing, so builder.end() would only see the original branches.
This will create bugs in user code that relied on the pre-apache#1253 behaviour.

@timsaucer
Copy link
Author

Before apache#1253 we wrote the updated builder back into self, so callers could safely ignore the return value (builder.when(...); builder.otherwise(...)) and still observe the new arm.

Yes, I recognize this would be slightly different behavior. I believe usage expecting mutate in place on the case builder object is incorrect usage. That usage, while possible, does not match the documentation. I think we can mark this as a breaking change. The new behavior does match the documentation.

@kosiew kosiew merged commit 6e384ba into kosiew:frozen-1250 Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants