alamb commented on PR #11550: URL: https://github.com/apache/datafusion/pull/11550#issuecomment-2243508166
> I had tried to directly modify Expr before, but ends up builder API. I couldn't remember the reason > My guess is that it has to do with all of the other functions returning Expr rather than Result<Expr> and to fit neatly in with the rest of the code base I think another reason was that it might be easier to understand what APIs could be called wnen for example, if we created `Expr::order_by` there are many Exprs this doesn't make sense for (like `Expr::Like` for example). By making a builder then the APIs used for creating aggregate expressions can be in a single trait and documented with examples, etc > But I would like to optimize for user experience on these APIs, if directly mutating Expr is actually easier and straightforward to use I agree with this (though I do think if we change the API we should try to avoid any more churn than necessary). I would tend to favor @timsaucer 's opinion on what is easier / more intuitive. I think I am too close to DataFusion / Rust these days to really know what is easier from a "someone who is new to the datafusion codebase" perspective -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
