findepi commented on PR #16781: URL: https://github.com/apache/datafusion/pull/16781#issuecomment-3077368850
> I think it could be reduced in size somewhat by adding `aliases` to the default implementations. i'd rather see stuff like aliases and documentation not be part of ScalardUDFImpl at all. The logical plan doesn't have to bear documentation of its constituents. But yeah, i can add aliases there. > My biggest concern is that in the future when we add new functions / modify existing ones we will forget to add a corresponding equals and hash implementations This is absolutely NOT improved by this PR. The problem remains unsolved. > To try and mitigage the issues, I suggest: > > 1. Add a comment to all the implementations added in this PR like `// Implement equals because there are fields other than name and signature` and `// implement hash because there are fields other than name and signature` I don't think it helps. Whenever you see equals/hash functions you know what they are for. The crux of the problem is -- you need to be aware of their existence in the ScalardUDFImpl to even implement them! I'd suggest _removing_ default implementations if we cannot make them correct. Not in this PR. -- 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
