feat(sql): add format_price function for finance #4960
Conversation
|
|
|
Hi @nwoolmer, I'm new on open source, this is my first issue. I'd like to help in more issues. I implement the format_price to #4620, But in the documentation https://www.cmegroup.com/education/courses/introduction-to-treasuries/calculating-us-treasury-pricing.html can have the third digit in fraction part ( The Default for US Treasury is quarter of 32nd of a point). But you didn't put that in the signature described in #4620, isn't that expected? |
Hi @thalesreisbr, thank you for the contribution, and this is a very good question. |
| return new FormatPrice(args.getQuick(0), args.getQuick(1)); | ||
| } | ||
|
|
||
| private static class FormatPrice extends StrFunction implements BinaryFunction { |
There was a problem hiding this comment.
it is better to have a VARCHAR function. This is a UTF8 encoded string, that avoids UTF16-to-UTF8 reencoding on the wire.
| double fractionalPart = decimalPrice - wholePart; | ||
| int fractionalTicks = (int) Math.round(fractionalPart * tickSize); | ||
|
|
||
| return String.format("%d-%02d", wholePart, fractionalTicks); |
There was a problem hiding this comment.
while this is a conventional Java way to format primitive numerics as a string, we are trying to avoid this method across our codebase to avoid generating string objects on the critical path.
Instead, please create a sink class member of Utf8StringSink type and print primitive values into that sink.
null values should not be printed at all, thus resulting in an empty sink. Hope this makes sense?
| public class FormatPriceFunctionFactoryTest extends AbstractFunctionFactoryTest { | ||
|
|
||
| @Test | ||
| public void testFormatPriceHappyPath() throws Exception { |
There was a problem hiding this comment.
could you add tests that produce multi-row result set? for example
select format_price(rnd_double() * 100, 16) from long_sequence(50) order by 1that would also sort the values of your function to a more complete exercise.
| assertQuery("format_price\n100-16\n", "select format_price(100.5, 32)"); | ||
| assertQuery("format_price\n101-00\n", "select format_price(101.0, 32)"); | ||
| assertQuery("format_price\n101-00\n", "select format_price(101, 32)"); | ||
| assertQuery("format_price\n99-12\n", "select format_price(99.375, 32)"); |
There was a problem hiding this comment.
please add tests that exercise different values of the tick size
| } | ||
|
|
||
| private static class FormatPrice extends StrFunction implements BinaryFunction { | ||
| private final Function decimal_form; |
There was a problem hiding this comment.
in Java we use camelCase variable names, e.g. decimalForm
|
Hey @thalesreisbr , are you still interested in this or would you like us to sort it out? |
Hi @nwoolmer, I would like to take this issue if possible |
|
Let me get back to you on Monday! |
Implements format_price to #4620