gianm commented on pull request #10230: URL: https://github.com/apache/druid/pull/10230#issuecomment-670280819
@kaplanmaxe thank you for the contribution! There's another patch with a similar feature: #10084. I prefer the approach you took in this patch — of defining named functions — because it will be easier to integrate into the SQL layer. It is pretty quick to write a SQL operator that is backed by a native expr function. You also have a few more functions added (like bit shifts), which is great. Some comments: 1. Please add some tests. You could start by copying the tests from #10084 and then adding new ones for the additional functions you have. That patch has a pretty good range of tests for a variety of cases. It's a bit more complicated because it also modified the grammar. Your tests would mostly be going in "FunctionTest". 2. The functions should be named like other native expr functions, using `snake_case` rather than `camelCase`. 3. If you're interested in adding SQL functions too, or as a follow up, check out IPv4AddressMatchOperatorConversion for an example of how to implement a SQL function that maps cleanly onto a native expr function. It is mostly just declaring the proper type and operand checking info for the SQL layer. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
