milenkovicm commented on PR #22415: URL: https://github.com/apache/datafusion/pull/22415#issuecomment-4508988671
> Right now nothing in DF actually uses `apply_expressions` so it does not fail. The original intention was to use it for serializing dynamic filters, but #22011 ended up going with typed getters instead. I suspect it'll get used for making Dynamic Filters and BufferExec work in #21350, and there's also interest in a read/write pair of expressions on the plan (#20009). if so, would it be safer if you have added new trait which must be implemented by the operators which actually allow introspection of the expressions? currently we have a method which we cant really explain what it may or may not be used for, and we're forcing everybody to implement it. If nothing is using it do we want to push everybody to implement it just now? > So as of today, returning `Ok(Continue)` and skipping all expressions is actually safe. But I don't think implementing it partially (visiting some expressions but not others) is safe since the API is intended to be generic (the intention is not only for dynamic filters) and any future caller can assume you've visited everything you own. generic is never great in public API, talking from experience. @alamb would you reconsider adding default implementation just for now until we're sure its going to be useful? we can always force users to implement it in the next release cycle once the design settles down (or remove it, with minor impact) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
