gianm commented on pull request #10635: URL: https://github.com/apache/druid/pull/10635#issuecomment-757031980
My 2ยข on the naming thing: I like having 3 separate functions, because I think a 1-function model works best if you have a good spec for format strings, which isn't what this patch is about. An example of a good spec is: https://www.postgresql.org/docs/current/functions-formatting.html#FUNCTIONS-FORMATTING-NUMERIC-TABLE If you just have a few different mutually exclusive options, like we do here, I think it's more SQL-y to have different functions. In the future, we might introduce a postgresql-style number formatting spec, but we don't have to do it today. A couple of things though: - IMO the name `decimal_format` is misleading. It sounds like a function that will format a number to a specific amount of decimals. Maybe instead we could call it `human_readable_decimal_format`. For consistency it may then be nice to call the others `human_readable_binary_byte_format` and `human_readable_decimal_byte_format`. The function names are long, but I think that's probably better than being misleading. Any thoughts @FrankChen021, @abhishekagarwal87, @clintropolis? - Please include examples in the documentation of what the formatted numbers will look like. I'm just writing this as a comment instead of a review, since I didn't read the code. ---------------------------------------------------------------- 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]
