You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/05/08 08:10:00 UTC
[GitHub] [druid] FrankChen021 opened a new pull request #10635: Add SQL functions to format numbers into human readable format
FrankChen021 opened a new pull request #10635:
URL: https://github.com/apache/druid/pull/10635
This PR implements requirements described in #10584
In this PR, 3 SQL functions and 3 corresponding native functions are provided to format numbers to different style.
| SQL/Native function | Description |
| --- | --- |
| human_readable_binary_byte_format(value[, precision]) | Returns the value in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. Supported unit suffix: `B`, `KiB`, `MiB`, `GiB`, `TiB`, `PiB`, `EiB`. `precision` must be in the range of [0,3] (default: 2). |
| human_readable_decimal_byte_format(value[, precision]) | Returns the value in human-readable [SI](https://en.wikipedia.org/wiki/Binary_prefix) format. Supported unit suffix: `B`, `KB`, `MB`, `GB`, `TB`, `PB`, `EB`. `precision` must be in the range of [0,3] (default: 2). |
| human_readable_decimal_format(value[, precision]) | Returns the value in human-readable SI format. Supported unit suffix: `K`, `M`, `G`, `T`, `P`, `E`. `precision` must be in the range of [0,3] (default: 2). |
The reason why 3 functions are provided to user instead of 1 function with different argument of format is that we think it's more simpler for a user to call these function. However in internal implementation, there's only one public format function exposed for all 3 format.
We've tested this feature in our clusters, and it works as we expect.
Web Console now looks like
<hr>
This PR has:
- [X] been self-reviewed.
- [X] added documentation for new or modified features or behaviors.
- [X] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [X] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [X] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [X] been tested in a test Druid cluster.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r537244165
##########
File path: core/src/test/java/org/apache/druid/java/util/common/HumanReadableBytesTest.java
##########
@@ -395,6 +395,107 @@ public void testBytesRange()
Assert.assertEquals("value must be in the range of [0, 5]", message);
}
+ @Test
+ public void testFormatInBinaryByte()
+ {
+ Assert.assertEquals("-8.00 EiB", HumanReadableBytes.format(Long.MIN_VALUE, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("-8.000 EiB", HumanReadableBytes.format(Long.MIN_VALUE, 3, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+
+ Assert.assertEquals("-2.00 GiB", HumanReadableBytes.format(Integer.MIN_VALUE, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("-32.00 KiB", HumanReadableBytes.format(Short.MIN_VALUE, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("-128.00 B", HumanReadableBytes.format(Byte.MIN_VALUE, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("-1.00 B", HumanReadableBytes.format(-1, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("0.00 B", HumanReadableBytes.format(0, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.00 B", HumanReadableBytes.format(1, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.00 KiB", HumanReadableBytes.format(1024L, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.00 MiB", HumanReadableBytes.format(1024L * 1024, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.00 GiB", HumanReadableBytes.format(1024L * 1024 * 1024, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.00 TiB", HumanReadableBytes.format(1024L * 1024 * 1024 * 1024, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.00 PiB", HumanReadableBytes.format(1024L * 1024 * 1024 * 1024 * 1024, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("8.00 EiB", HumanReadableBytes.format(Long.MAX_VALUE, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ }
+
+ @Test
+ public void testPrecisionInBinaryFormat()
+ {
+ Assert.assertEquals("1 KiB", HumanReadableBytes.format(1500, 0, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.5 KiB", HumanReadableBytes.format(1500, 1, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.46 KiB", HumanReadableBytes.format(1500, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.465 KiB", HumanReadableBytes.format(1500, 3, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ }
+
+ @Test
+ public void testPrecisionInDecimalFormat()
+ {
+ Assert.assertEquals("1 KB", HumanReadableBytes.format(1456, 0, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.5 KB", HumanReadableBytes.format(1456, 1, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.46 KB", HumanReadableBytes.format(1456, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.456 KB", HumanReadableBytes.format(1456, 3, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ }
+
+ @Test
+ public void testFormatInDecimalByte()
+ {
+ Assert.assertEquals("1.00 B", HumanReadableBytes.format(1, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.00 KB", HumanReadableBytes.format(1000L, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.00 MB", HumanReadableBytes.format(1000L * 1000, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.00 GB", HumanReadableBytes.format(1000L * 1000 * 1000, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.00 TB", HumanReadableBytes.format(1000L * 1000 * 1000 * 1000, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.00 PB", HumanReadableBytes.format(1000L * 1000 * 1000 * 1000 * 1000, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("9.22 EB", HumanReadableBytes.format(Long.MAX_VALUE, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+
+ Assert.assertEquals("100.00 KB", HumanReadableBytes.format(99999, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("99.999 KB", HumanReadableBytes.format(99999, 3, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+
+ Assert.assertEquals("999.9 PB", HumanReadableBytes.format(999_949_999_999_999_999L, 1, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("999.95 PB", HumanReadableBytes.format(999_949_999_999_999_999L, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("999.949 PB", HumanReadableBytes.format(999_949_999_999_999_999L, 3, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ }
+
+ @Test
+ public void testFormatInDecimal()
+ {
+ Assert.assertEquals("1.00 ", HumanReadableBytes.format(1, 2, HumanReadableBytes.UnitSystem.DECIMAL, true));
Review comment:
This special case has been handled in the latest commit.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-739679792
> Do you like how the web console auto compiles in the function docs? :-p
Yes, it's very useful and convenient. I like it.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r539206931
##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -3275,4 +3276,86 @@ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
return l.stream();
}
}
+
+ abstract class SizeFormatFunc implements Function
+ {
+ protected abstract HumanReadableBytes.UnitSystem getUnitSystem();
+
+ /**
+ * Evaluate given expression
+ * By default, 'precision' is 2 and 'hasSpace' is false
+ */
+ @Override
+ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
+ {
+ final long bytes = args.get(0).eval(bindings).asLong();
Review comment:
Hi @clintropolis , null and type handling has been improved in the latest commit. Please check it at your convenience.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r547515782
##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -519,6 +525,203 @@ public void testLeast()
assertExpr("least(1, null, 'A')", "1");
}
+ @Test
+ public void testSizeFormat()
+ {
+ assertExpr("binary_byte_format(-1024)", "-1.00 KiB");
+ assertExpr("binary_byte_format(1024)", "1.00 KiB");
+ assertExpr("binary_byte_format(1024*1024)", "1.00 MiB");
+ assertExpr("binary_byte_format(1024*1024*1024)", "1.00 GiB");
+ assertExpr("binary_byte_format(1024*1024*1024*1024)", "1.00 TiB");
+ assertExpr("binary_byte_format(1024*1024*1024*1024*1024)", "1.00 PiB");
+
+ assertExpr("decimal_byte_format(-1000)", "-1.00 KB");
+ assertExpr("decimal_byte_format(1000)", "1.00 KB");
+ assertExpr("decimal_byte_format(1000*1000)", "1.00 MB");
+ assertExpr("decimal_byte_format(1000*1000*1000)", "1.00 GB");
+ assertExpr("decimal_byte_format(1000*1000*1000*1000)", "1.00 TB");
+
+ assertExpr("decimal_format(-1000)", "-1.00 K");
+ assertExpr("decimal_format(1000)", "1.00 K");
+ assertExpr("decimal_format(1000*1000)", "1.00 M");
+ assertExpr("decimal_format(1000*1000*1000)", "1.00 G");
+ assertExpr("decimal_format(1000*1000*1000*1000)", "1.00 T");
+ }
+
+ @Test
+ public void testSizeFormatWithDifferentPrecision()
+ {
+ assertExpr("binary_byte_format(1024, 0)", "1 KiB");
+ assertExpr("binary_byte_format(1024*1024, 1)", "1.0 MiB");
+ assertExpr("binary_byte_format(1024*1024*1024, 2)", "1.00 GiB");
+ assertExpr("binary_byte_format(1024*1024*1024*1024, 3)", "1.000 TiB");
+
+ assertExpr("decimal_byte_format(1234, 0)", "1 KB");
+ assertExpr("decimal_byte_format(1234*1000, 1)", "1.2 MB");
+ assertExpr("decimal_byte_format(1234*1000*1000, 2)", "1.23 GB");
+ assertExpr("decimal_byte_format(1234*1000*1000*1000, 3)", "1.234 TB");
+
+ assertExpr("decimal_format(1234, 0)", "1 K");
+ assertExpr("decimal_format(1234*1000,1)", "1.2 M");
+ assertExpr("decimal_format(1234*1000*1000,2)", "1.23 G");
+ assertExpr("decimal_format(1234*1000*1000*1000,3)", "1.234 T");
+ }
+
+ @Test
+ public void testSizeFormatWithEdgeCases()
+ {
+ //a nonexist value is null which is treated as 0
+ assertExpr("binary_byte_format(nonexist)", "0 B");
+
+ //f = 12.34
+ assertExpr("binary_byte_format(f)", "12 B");
+
+ //nan is Double.NaN
+ assertExpr("binary_byte_format(nan)", "0 B");
+
+ //inf = Double.POSITIVE_INFINITY
+ assertExpr("binary_byte_format(inf)", "8.00 EiB");
+
+ //inf = Double.NEGATIVE_INFINITY
+ assertExpr("binary_byte_format(-inf)", "-8.00 EiB");
+
+ // o = 0
+ assertExpr("binary_byte_format(o)", "0 B");
+
+ // od = 0D
+ assertExpr("binary_byte_format(od)", "0 B");
+
+ // of = 0F
+ assertExpr("binary_byte_format(of)", "0 B");
+ }
+
+ @Test
+ public void testSizeForatInvalidArgumentType()
+ {
+ try {
+ //x = "foo"
+ Parser.parse("binary_byte_format(x)", ExprMacroTable.nil())
+ .eval(bindings);
+
+ //must not go to here
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Function[binary_byte_format] needs a number as its first argument", e.getMessage());
+ }
+
+ try {
+ //x = "foo"
+ Parser.parse("binary_byte_format(1024, x)", ExprMacroTable.nil())
+ .eval(bindings);
+
+ //must not go to here
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Function[binary_byte_format] needs an integer as its second argument", e.getMessage());
+ }
+
+ try {
+ //of = 0F
+ Parser.parse("binary_byte_format(1024, of)", ExprMacroTable.nil())
+ .eval(bindings);
+
+ //must not go to here
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Function[binary_byte_format] needs an integer as its second argument", e.getMessage());
+ }
+
+ try {
+ //of = 0F
+ Parser.parse("binary_byte_format(1024, nonexist)", ExprMacroTable.nil())
+ .eval(bindings);
+
+ //must not go to here
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Function[binary_byte_format] needs an integer as its second argument", e.getMessage());
+ }
+ }
+
+ @Test
+ public void testSizeFormatInvalidPrecision()
+ {
+ try {
+ Parser.parse("binary_byte_format(1024, maxLong)", ExprMacroTable.nil())
+ .eval(bindings);
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Given precision[9223372036854775807] of Function[binary_byte_format] must be in the range of [0,3]", e.getMessage());
+ }
+
+ try {
+ Parser.parse("binary_byte_format(1024, minLong)", ExprMacroTable.nil())
+ .eval(bindings);
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Given precision[-9223372036854775808] of Function[binary_byte_format] must be in the range of [0,3]", e.getMessage());
+ }
+
+ try {
+ Parser.parse("binary_byte_format(1024, -1)", ExprMacroTable.nil())
+ .eval(bindings);
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Given precision[-1] of Function[binary_byte_format] must be in the range of [0,3]", e.getMessage());
+ }
+
+ try {
+ Parser.parse("binary_byte_format(1024, 4)", ExprMacroTable.nil())
+ .eval(bindings);
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Given precision[4] of Function[binary_byte_format] must be in the range of [0,3]", e.getMessage());
+ }
+ }
+
+ @Test
+ public void testSizeFormatInvalidArgumentSize()
+ {
+ expectedException.expect(IAE.class);
+ expectedException.expectMessage("Function[binary_byte_format] needs 1 or 2 arguments");
+ Parser.parse("binary_byte_format(1024, 2, 3)", ExprMacroTable.nil())
+ .eval(bindings);
+ }
+
+ @Test
+ public void testSizeFormatWithNoDefaultValueForNull()
+ {
+ NullHandling.updateForTests(false);
Review comment:
All unit tests are run with both values of `druid.generic.useDefaultValueForNull`, so it isn't necessary to explicitly configure it. What we typically do is try to just write the test to check for the mode and adjust the expectation accordingly, e.g. to use another example from this file
```
assertExpr("lpad(x, 2, '')", NullHandling.replaceWithDefault() ? null : "fo");
```
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r551727213
##########
File path: core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
##########
@@ -519,6 +525,203 @@ public void testLeast()
assertExpr("least(1, null, 'A')", "1");
}
+ @Test
+ public void testSizeFormat()
+ {
+ assertExpr("binary_byte_format(-1024)", "-1.00 KiB");
+ assertExpr("binary_byte_format(1024)", "1.00 KiB");
+ assertExpr("binary_byte_format(1024*1024)", "1.00 MiB");
+ assertExpr("binary_byte_format(1024*1024*1024)", "1.00 GiB");
+ assertExpr("binary_byte_format(1024*1024*1024*1024)", "1.00 TiB");
+ assertExpr("binary_byte_format(1024*1024*1024*1024*1024)", "1.00 PiB");
+
+ assertExpr("decimal_byte_format(-1000)", "-1.00 KB");
+ assertExpr("decimal_byte_format(1000)", "1.00 KB");
+ assertExpr("decimal_byte_format(1000*1000)", "1.00 MB");
+ assertExpr("decimal_byte_format(1000*1000*1000)", "1.00 GB");
+ assertExpr("decimal_byte_format(1000*1000*1000*1000)", "1.00 TB");
+
+ assertExpr("decimal_format(-1000)", "-1.00 K");
+ assertExpr("decimal_format(1000)", "1.00 K");
+ assertExpr("decimal_format(1000*1000)", "1.00 M");
+ assertExpr("decimal_format(1000*1000*1000)", "1.00 G");
+ assertExpr("decimal_format(1000*1000*1000*1000)", "1.00 T");
+ }
+
+ @Test
+ public void testSizeFormatWithDifferentPrecision()
+ {
+ assertExpr("binary_byte_format(1024, 0)", "1 KiB");
+ assertExpr("binary_byte_format(1024*1024, 1)", "1.0 MiB");
+ assertExpr("binary_byte_format(1024*1024*1024, 2)", "1.00 GiB");
+ assertExpr("binary_byte_format(1024*1024*1024*1024, 3)", "1.000 TiB");
+
+ assertExpr("decimal_byte_format(1234, 0)", "1 KB");
+ assertExpr("decimal_byte_format(1234*1000, 1)", "1.2 MB");
+ assertExpr("decimal_byte_format(1234*1000*1000, 2)", "1.23 GB");
+ assertExpr("decimal_byte_format(1234*1000*1000*1000, 3)", "1.234 TB");
+
+ assertExpr("decimal_format(1234, 0)", "1 K");
+ assertExpr("decimal_format(1234*1000,1)", "1.2 M");
+ assertExpr("decimal_format(1234*1000*1000,2)", "1.23 G");
+ assertExpr("decimal_format(1234*1000*1000*1000,3)", "1.234 T");
+ }
+
+ @Test
+ public void testSizeFormatWithEdgeCases()
+ {
+ //a nonexist value is null which is treated as 0
+ assertExpr("binary_byte_format(nonexist)", "0 B");
+
+ //f = 12.34
+ assertExpr("binary_byte_format(f)", "12 B");
+
+ //nan is Double.NaN
+ assertExpr("binary_byte_format(nan)", "0 B");
+
+ //inf = Double.POSITIVE_INFINITY
+ assertExpr("binary_byte_format(inf)", "8.00 EiB");
+
+ //inf = Double.NEGATIVE_INFINITY
+ assertExpr("binary_byte_format(-inf)", "-8.00 EiB");
+
+ // o = 0
+ assertExpr("binary_byte_format(o)", "0 B");
+
+ // od = 0D
+ assertExpr("binary_byte_format(od)", "0 B");
+
+ // of = 0F
+ assertExpr("binary_byte_format(of)", "0 B");
+ }
+
+ @Test
+ public void testSizeForatInvalidArgumentType()
+ {
+ try {
+ //x = "foo"
+ Parser.parse("binary_byte_format(x)", ExprMacroTable.nil())
+ .eval(bindings);
+
+ //must not go to here
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Function[binary_byte_format] needs a number as its first argument", e.getMessage());
+ }
+
+ try {
+ //x = "foo"
+ Parser.parse("binary_byte_format(1024, x)", ExprMacroTable.nil())
+ .eval(bindings);
+
+ //must not go to here
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Function[binary_byte_format] needs an integer as its second argument", e.getMessage());
+ }
+
+ try {
+ //of = 0F
+ Parser.parse("binary_byte_format(1024, of)", ExprMacroTable.nil())
+ .eval(bindings);
+
+ //must not go to here
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Function[binary_byte_format] needs an integer as its second argument", e.getMessage());
+ }
+
+ try {
+ //of = 0F
+ Parser.parse("binary_byte_format(1024, nonexist)", ExprMacroTable.nil())
+ .eval(bindings);
+
+ //must not go to here
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Function[binary_byte_format] needs an integer as its second argument", e.getMessage());
+ }
+ }
+
+ @Test
+ public void testSizeFormatInvalidPrecision()
+ {
+ try {
+ Parser.parse("binary_byte_format(1024, maxLong)", ExprMacroTable.nil())
+ .eval(bindings);
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Given precision[9223372036854775807] of Function[binary_byte_format] must be in the range of [0,3]", e.getMessage());
+ }
+
+ try {
+ Parser.parse("binary_byte_format(1024, minLong)", ExprMacroTable.nil())
+ .eval(bindings);
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Given precision[-9223372036854775808] of Function[binary_byte_format] must be in the range of [0,3]", e.getMessage());
+ }
+
+ try {
+ Parser.parse("binary_byte_format(1024, -1)", ExprMacroTable.nil())
+ .eval(bindings);
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Given precision[-1] of Function[binary_byte_format] must be in the range of [0,3]", e.getMessage());
+ }
+
+ try {
+ Parser.parse("binary_byte_format(1024, 4)", ExprMacroTable.nil())
+ .eval(bindings);
+ Assert.assertTrue(false);
+ }
+ catch (IAE e) {
+ Assert.assertEquals("Given precision[4] of Function[binary_byte_format] must be in the range of [0,3]", e.getMessage());
+ }
+ }
+
+ @Test
+ public void testSizeFormatInvalidArgumentSize()
+ {
+ expectedException.expect(IAE.class);
+ expectedException.expectMessage("Function[binary_byte_format] needs 1 or 2 arguments");
+ Parser.parse("binary_byte_format(1024, 2, 3)", ExprMacroTable.nil())
+ .eval(bindings);
+ }
+
+ @Test
+ public void testSizeFormatWithNoDefaultValueForNull()
+ {
+ NullHandling.updateForTests(false);
Review comment:
MySQL ships a similar function [format_bytes](https://dev.mysql.com/doc/refman/5.7/en/sys-format-bytes.html)
I struggled the two different approaches at first. And at last I chose to do it by 3 different functions. The reasons are,
1. different function names are more meaningful than different arguments for one function. Since there're 3 different unit systems in this PR, how to name them in a short enough way and without ambiguity is a big challenge. For example, `FORMAT(number, 'si')`, `FORMAT(number, 'dec')`, `si` and `dec` are standard abbreviation and short enough but they're hard to understand; `FORMAT(number, 'binary_byte')`, it's clear enough, but it's not so simple compared to `binary_byte_format(number)`
2. at the underlying layer, there are always different format functions, and if we provide one function at the user side, we have to do some checks on the format specifier and dispatch calls to those different functions. It's a little bit simple if different functions are provided.
But as you mentioned, there are also some drawbacks in this way. If the standard is to keep consistent with other databases or keep less numbers of functions exposed to users, maybe we need to combine these functions together.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-885371850
Hi @gianm , do you have any other comments ?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-757354535
I, for one, don't have a strong opinion. `decimal_format` here is equivalent to https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/text/CompactNumberFormat.html which I liked. So `decimal_format` could be `compact_format`/`compact_number`
The rest of the methods can be
`compact_decimal_byte_format`/`compact_decimal_byte_number`/`compact_number_decimal_byte`/`compact_number_to_decimal_byte`
and
`compact_binary_byte_format`/`compact_binary_byte_number`/`compact_number_binary_byte`/`compact_number_to_binary_byte`
`compact_number` / `compact_number_to_decimal_byte` / `compact_number_to_binary_byte` seems least ambiguous to me. That's all I can think of :)
Btw Java also has DecimalFormat which formats the number to any pattern use passes but doesn't compact it as such.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-749307337
The branch has been rebased onto the latest master to resolve conflicts with master branch. And do you have any more comments ? @clintropolis
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 closed pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 closed pull request #10635:
URL: https://github.com/apache/druid/pull/10635
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-870351762
@FrankChen021 Just one minor comment on the doc. Rest looks good to me. Thank you for your patience.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r538058342
##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -3275,4 +3276,86 @@ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
return l.stream();
}
}
+
+ abstract class SizeFormatFunc implements Function
+ {
+ protected abstract HumanReadableBytes.UnitSystem getUnitSystem();
+
+ /**
+ * Evaluate given expression
+ * By default, 'precision' is 2 and 'hasSpace' is false
+ */
+ @Override
+ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
+ {
+ final long bytes = args.get(0).eval(bindings).asLong();
Review comment:
Since we are calling `asLong` without checking `isNumericNull` of the `ExprEval`, we are ignoring the value of `druid.generic.useDefaultValueForNull` here, which I think is incorrect.
Thinking out loud, how should this function behave with non-long inputs?
The way this is currently implement:
* Inputs of `ExprType.DOUBLE` will be cast to a `ExprType.LONG` before conversion.
* For `ExprType.STRING` inputs, if they are number-ish strings, they will be parsed into long values, but if not `asLong` will always be 0.
I don't know that this behavior is incorrect, I just wanted to call it out to think about it.
I do think we want to check for `isNumericNull` and return `ExprEval.of(null)` if `NullHandling.sqlCompatible()` is set, for any input types.
I see in the SQL operator it looks like it strictly validates that the inputs are numeric, while Druid native expressions have traditionally been a bit fast and loose about the inputs they accept and tend to be rather forgiving, so perhaps this is ok that the behavior here doesn't quite match.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r537243650
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -16153,6 +16161,44 @@ public void testTimeStampAddConversion() throws Exception
);
}
+ public void testSizeFormatFunction() throws Exception
+ {
+ testQuery(
+ "SELECT m1, "
+ + "BINARY_BYTE_FORMAT(45678),"
+ + "BINARY_BYTE_FORMAT(m1*12345),"
+ + "BINARY_BYTE_FORMAT(m1*12345, 0), "
+ + "DECIMAL_BYTE_FORMAT(m1*12345), "
+ + "DECIMAL_FORMAT(m1*12345) "
+ + "FROM numfoo WHERE f1 = 0.1 LIMIT 1",
+ ImmutableList.of(
+ newScanQueryBuilder()
+ .dataSource(CalciteTests.DATASOURCE3)
+ .intervals(querySegmentSpec(Filtration.eternity()))
+ .virtualColumns(expressionVirtualColumn("v0", "'44.61KiB'", ValueType.STRING),
Review comment:
This is a special case. I also thought it should a function call expression here. But because the given argument is a constant, this function call has been calculated during SQL parsing phase before converting to native sql. I've added some comments in the latest commit here.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r660434509
##########
File path: docs/misc/math-expr.md
##########
@@ -258,3 +258,12 @@ supported features:
* math functions: `abs`, `acos`, `asin`, `atan`, `cbrt`, `ceil`, `cos`, `cosh`, `cot`, `exp`, `expm1`, `floor`, `getExponent`, `log`, `log10`, `log1p`, `nextUp`, `rint`, `signum`, `sin`, `sinh`, `sqrt`, `tan`, `tanh`, `toDegrees`, `toRadians`, `ulp`, `atan2`, `copySign`, `div`, `hypot`, `max`, `min`, `nextAfter`, `pow`, `remainder`, `scalb` are supported for numeric types
* time functions: `timestamp_floor` (with constant granularity argument) is supported for numeric types
* other: `parse_long` is supported for numeric and string types
+
+
+## Other functions
+
+| function | description |
+| --- | --- |
+| human_readable_binary_byte_format(value[, precision]) | Format a number in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. `precision` must be in the range of [0,3] (default: 2). For example:<li> human_readable_binary_byte_format(1048576) returns `1.00 MiB`</li><li>human_readable_binary_byte_format(1048576, 1) returns `1.000 MiB`</li> |
Review comment:
Thanks for your quick review. I found this bug and fixed it.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-898177797
@FrankChen021 can you fix up conflicts? I think this is ready to merge otherwise
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-757354535
I, for one, don't have a strong opinion. `decimal_format` here is equivalent to https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/text/CompactNumberFormat.html which I liked. So `decimal_format` could be `compact_format`/`compact_number`
The rest of the methods can be
`compact_decimal_byte_format`/`compact_decimal_byte_number`/`compact_number_decimal_byte`/`compact_number_to_decimal_byte`
and
`compact_binary_byte_format`/`compact_binary_byte_number`/`compact_number_binary_byte`/`compact_number_to_binary_byte`
`compact_number` / `compact_number_to_decimal_byte` / `compact_number_to_binary_byte` seems least ambiguous to me. That's all I can think of :)
Btw Java also has DecimalFormat which formats the number to any pattern use passes but doesn't compact it as such.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-757192914
>
> * 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?
Naming is really a big challenge in the world of programming :) . There're some ways listed as follows
* Alternative 1 - One function with different format specifier
format(number, 'human_readable_bin_byte')
format(number, 'human_readable_dec_byte')
format(number, 'human_readable_decimal')
It looks like the parameters are still too long.
* Alternative 2 - One function with abbreviated format specifier
format(number, 'si') -- si is the standard word for binary byte.
format(number, 'iec') -- iec is the standard word for decimal byte
format(number, 'dec')
If we want to design one function, the challenge is how to give nice names for format specifiers. But it also leaves the extensibility in the future to support other kinds of number format. Do you have nicer format specifier names ?
* Alternative 3 - based on @gianm 's suggestion
human_readable_format_bin_byte
human_readable_format_dec_byte
human_readable_format_dec
I think I would prefer the 3rd here. What do you think ? @gianm @clintropolis @abhishekagarwal87 ?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-835196575
This PR was accidentally closed, re-open again for review.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r538058342
##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -3275,4 +3276,86 @@ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
return l.stream();
}
}
+
+ abstract class SizeFormatFunc implements Function
+ {
+ protected abstract HumanReadableBytes.UnitSystem getUnitSystem();
+
+ /**
+ * Evaluate given expression
+ * By default, 'precision' is 2 and 'hasSpace' is false
+ */
+ @Override
+ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
+ {
+ final long bytes = args.get(0).eval(bindings).asLong();
Review comment:
Since we are calling `asLong` without checking `isNumericNull` of the `ExprEval`, we are ignoring the value of `druid.generic.useDefaultValueForNull` here, which I think is incorrect and this should return null instead.
Thinking out loud, how should this function behave with non-long inputs?
The way this is currently implement:
* Inputs of `ExprType.DOUBLE` will be cast to a `ExprType.LONG` before conversion.
* For `ExprType.STRING` inputs, if they are number-ish strings, they will be parsed into long values, but if not `asLong` will always be 0.
I don't know that this behavior is incorrect, I just wanted to call it out to think about it.
I do think we want to check for `isNumericNull` and return `ExprEval.of(null)` if `NullHandling.sqlCompatible()` is set, for any input types.
I see in the SQL operator it looks like it strictly validates that the inputs are numeric, while Druid native expressions have traditionally been a bit fast and loose about the inputs they accept and tend to be rather forgiving, so perhaps this is ok that the behavior here doesn't quite match.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r656099852
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/HumanReadableFormatOperatorConversion.java
##########
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.calcite.expression.builtin;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.sql.SqlCallBinding;
+import org.apache.calcite.sql.SqlFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlOperandCountRange;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.type.SqlOperandCountRanges;
+import org.apache.calcite.sql.type.SqlOperandTypeChecker;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.sql.calcite.expression.DruidExpression;
+import org.apache.druid.sql.calcite.expression.OperatorConversions;
+import org.apache.druid.sql.calcite.expression.SqlOperatorConversion;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+
+public class HumanReadableFormatOperatorConversion implements SqlOperatorConversion
+{
+ public static final SqlOperatorConversion BINARY_BYTE_FORMAT = new HumanReadableFormatOperatorConversion("human_readable_binary_byte_format");
+ public static final SqlOperatorConversion DECIMAL_BYTE_FORMAT = new HumanReadableFormatOperatorConversion("human_readable_decimal_byte_format");
+ public static final SqlOperatorConversion DECIMAL_FORMAT = new HumanReadableFormatOperatorConversion("human_readable_decimal_format");
+
+ private final String name;
+ private final SqlFunction sqlFunction;
+
+ private HumanReadableFormatOperatorConversion(String name)
+ {
+ this.sqlFunction = OperatorConversions
+ .operatorBuilder(StringUtils.toUpperCase(name))
+ .operandTypeChecker(new HumanReadableFormatOperandTypeChecker())
+ .functionCategory(SqlFunctionCategory.STRING)
+ .returnTypeNonNull(SqlTypeName.VARCHAR)
Review comment:
this should use the recently added `returnTypeCascadeNullable` since it returns null if the input is null (see #11327)
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r537244973
##########
File path: core/src/main/java/org/apache/druid/java/util/common/HumanReadableBytes.java
##########
@@ -208,4 +208,108 @@ private static long parseInner(String rawNumber)
throw new IAE("Invalid format or out of range of long: %s", rawNumber);
}
}
+
+ public enum UnitSystem
+ {
+ /**
+ * also known as IEC format
+ * eg: B, KiB, MiB, GiB ...
+ */
+ BINARY_BYTE,
+
+ /**
+ * also known as SI format
+ * eg: B, KB, MB ...
+ */
+ DECIMAL_BYTE,
+
+ /**
+ * simplified SI format without 'B' indicator
+ * eg: K, M, G ...
+ */
+ DECIMAL
+ }
+
+ /**
+ * Returns a human-readable string version of input value
+ *
+ * @param bytes input value. Negative value is also allowed
+ * @param precision [0,3]
+ * @param unitSystem which unit system is adopted to format the input value, see {@link UnitSystem}
+ * @param hasSpace if it's true, there's an extra space between the number and the unit suffix
+ */
+ public static String format(long bytes, int precision, UnitSystem unitSystem, boolean hasSpace)
Review comment:
We intended to expose this parameter at the SQL/native query level at first, but found that this parameter is not so vital. So we still keep it in the underlying implementation but hide it from the SQL level.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r538155122
##########
File path: docs/querying/sql.md
##########
@@ -563,6 +563,9 @@ The [DataSketches extension](../development/extensions-core/datasketches-extensi
|`COALESCE(value1, value2, ...)`|Returns the first value that is neither NULL nor empty string.|
|`NVL(expr,expr-for-null)`|Returns 'expr-for-null' if 'expr' is null (or empty string for string type).|
|`BLOOM_FILTER_TEST(<expr>, <serialized-filter>)`|Returns true if the value is contained in a Base64-serialized bloom filter. See the [Bloom filter extension](../development/extensions-core/bloom-filter.html) documentation for additional details.|
+|`BINARY_BYTE_FORMAT(value, [precision])`|Returns the value in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. Supported unit suffix: `B`, `KiB`, `MiB`, `GiB`, `TiB`, `PiB`, `EiB`. `precision` must be in the range of [0,3] (default: 2).|
+|`DECIMAL_BYTE_FORMAT(value, [precision])`|Returns the value in human-readable [SI](https://en.wikipedia.org/wiki/Binary_prefix) format. Supported unit suffix: `B`, `KB`, `MB`, `GB`, `TB`, `PB`, `EB`. `precision` must be in the range of [0,3] (default: 2).|
+|`DECIMAL_FORMAT(value, [precision])`|Returns the value in human-readable SI format. Supported unit suffix: `K`, `M`, `G`, `T`, `P`, `E`. `precision` must be in the range of [0,3] (default: 2).|
Review comment:
yeah, i guess in the native expression version of this document the section is called math functions or something like that
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-870351762
@FrankChen021 Just one minor comment on the doc. Rest looks good to me. Thank you for your patience.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r656893902
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/HumanReadableFormatOperatorConversion.java
##########
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.calcite.expression.builtin;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.sql.SqlCallBinding;
+import org.apache.calcite.sql.SqlFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlOperandCountRange;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.type.SqlOperandCountRanges;
+import org.apache.calcite.sql.type.SqlOperandTypeChecker;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.sql.calcite.expression.DruidExpression;
+import org.apache.druid.sql.calcite.expression.OperatorConversions;
+import org.apache.druid.sql.calcite.expression.SqlOperatorConversion;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+
+public class HumanReadableFormatOperatorConversion implements SqlOperatorConversion
+{
+ public static final SqlOperatorConversion BINARY_BYTE_FORMAT = new HumanReadableFormatOperatorConversion("human_readable_binary_byte_format");
+ public static final SqlOperatorConversion DECIMAL_BYTE_FORMAT = new HumanReadableFormatOperatorConversion("human_readable_decimal_byte_format");
+ public static final SqlOperatorConversion DECIMAL_FORMAT = new HumanReadableFormatOperatorConversion("human_readable_decimal_format");
+
+ private final String name;
+ private final SqlFunction sqlFunction;
+
+ private HumanReadableFormatOperatorConversion(String name)
+ {
+ this.sqlFunction = OperatorConversions
+ .operatorBuilder(StringUtils.toUpperCase(name))
+ .operandTypeChecker(new HumanReadableFormatOperandTypeChecker())
+ .functionCategory(SqlFunctionCategory.STRING)
+ .returnTypeNonNull(SqlTypeName.VARCHAR)
Review comment:
Fixed
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r537281394
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -16153,6 +16161,44 @@ public void testTimeStampAddConversion() throws Exception
);
}
+ public void testSizeFormatFunction() throws Exception
+ {
+ testQuery(
+ "SELECT m1, "
+ + "BINARY_BYTE_FORMAT(45678),"
+ + "BINARY_BYTE_FORMAT(m1*12345),"
+ + "BINARY_BYTE_FORMAT(m1*12345, 0), "
+ + "DECIMAL_BYTE_FORMAT(m1*12345), "
+ + "DECIMAL_FORMAT(m1*12345) "
+ + "FROM numfoo WHERE f1 = 0.1 LIMIT 1",
+ ImmutableList.of(
+ newScanQueryBuilder()
+ .dataSource(CalciteTests.DATASOURCE3)
+ .intervals(querySegmentSpec(Filtration.eternity()))
+ .virtualColumns(expressionVirtualColumn("v0", "'44.61KiB'", ValueType.STRING),
Review comment:
👍 Thanks for clarifying
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r537243778
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/SizeFormatOperatorConversion.java
##########
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.calcite.expression.builtin;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.sql.SqlCallBinding;
+import org.apache.calcite.sql.SqlFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlOperandCountRange;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.type.SqlOperandCountRanges;
+import org.apache.calcite.sql.type.SqlOperandTypeChecker;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.sql.calcite.expression.DruidExpression;
+import org.apache.druid.sql.calcite.expression.OperatorConversions;
+import org.apache.druid.sql.calcite.expression.SqlOperatorConversion;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+
+public class SizeFormatOperatorConversion implements SqlOperatorConversion
+{
+ public static final SqlOperatorConversion BINARY_BYTE_FORMAT = new SizeFormatOperatorConversion("binary_byte_format");
+ public static final SqlOperatorConversion DECIMAL_BYTE_FORMAT = new SizeFormatOperatorConversion("decimal_byte_format");
+ public static final SqlOperatorConversion DECIMAL_FORMAT = new SizeFormatOperatorConversion("decimal_format");
+
+ private final String name;
+ private final SqlFunction sqlFunction;
+
+ private SizeFormatOperatorConversion(String name)
+ {
+ this.sqlFunction = OperatorConversions
+ .operatorBuilder(StringUtils.toUpperCase(name))
+ .operandTypeChecker(new StringFormatOperandTypeChecker())
+ .functionCategory(SqlFunctionCategory.STRING)
+ .returnTypeNonNull(SqlTypeName.VARCHAR)
+ .build();
+
+ this.name = name;
+ }
+
+ @Override
+ public SqlOperator calciteOperator()
+ {
+ return sqlFunction;
+ }
+
+ @Override
+ public DruidExpression toDruidExpression(
+ final PlannerContext plannerContext,
+ final RowSignature rowSignature,
+ final RexNode rexNode
+ )
+ {
+ return OperatorConversions.convertCall(plannerContext, rowSignature, rexNode, name);
+ }
+
+ private static class StringFormatOperandTypeChecker implements SqlOperandTypeChecker
+ {
+ @Override
+ public boolean checkOperandTypes(SqlCallBinding callBinding, boolean throwOnFailure)
+ {
+ final RelDataType firstArgType = callBinding.getOperandType(0);
Review comment:
Fixed
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-740309406
Hi @asdf2014 @gianm @jihoonson @suneet-s , Could you help to review 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-898235998
@clintropolis Fixed. Let's wait to see if there's any CI problems.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r660356714
##########
File path: docs/misc/math-expr.md
##########
@@ -258,3 +258,12 @@ supported features:
* math functions: `abs`, `acos`, `asin`, `atan`, `cbrt`, `ceil`, `cos`, `cosh`, `cot`, `exp`, `expm1`, `floor`, `getExponent`, `log`, `log10`, `log1p`, `nextUp`, `rint`, `signum`, `sin`, `sinh`, `sqrt`, `tan`, `tanh`, `toDegrees`, `toRadians`, `ulp`, `atan2`, `copySign`, `div`, `hypot`, `max`, `min`, `nextAfter`, `pow`, `remainder`, `scalb` are supported for numeric types
* time functions: `timestamp_floor` (with constant granularity argument) is supported for numeric types
* other: `parse_long` is supported for numeric and string types
+
+
+## Other functions
+
+| function | description |
+| --- | --- |
+| human_readable_binary_byte_format(value[, precision]) | Format a number in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. For example, human_readable_binary_byte_format(1048576) returns `1.00 MiB`. `precision` must be in the range of [0,3] (default: 2). |
Review comment:
@FrankChen021 can you add one example with a custom precision value? Maybe, one of these examples can be modified itself.
##########
File path: docs/misc/math-expr.md
##########
@@ -258,3 +258,12 @@ supported features:
* math functions: `abs`, `acos`, `asin`, `atan`, `cbrt`, `ceil`, `cos`, `cosh`, `cot`, `exp`, `expm1`, `floor`, `getExponent`, `log`, `log10`, `log1p`, `nextUp`, `rint`, `signum`, `sin`, `sinh`, `sqrt`, `tan`, `tanh`, `toDegrees`, `toRadians`, `ulp`, `atan2`, `copySign`, `div`, `hypot`, `max`, `min`, `nextAfter`, `pow`, `remainder`, `scalb` are supported for numeric types
* time functions: `timestamp_floor` (with constant granularity argument) is supported for numeric types
* other: `parse_long` is supported for numeric and string types
+
+
+## Other functions
+
+| function | description |
+| --- | --- |
+| human_readable_binary_byte_format(value[, precision]) | Format a number in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. `precision` must be in the range of [0,3] (default: 2). For example:<li> human_readable_binary_byte_format(1048576) returns `1.00 MiB`</li><li>human_readable_binary_byte_format(1048576, 1) returns `1.000 MiB`</li> |
Review comment:
```suggestion
| human_readable_binary_byte_format(value[, precision]) | Format a number in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. `precision` must be in the range of [0,3] (default: 2). For example:<li> human_readable_binary_byte_format(1048576) returns `1.00 MiB`</li><li>human_readable_binary_byte_format(1048576, 1) returns `1.0 MiB`</li> |
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r537280742
##########
File path: core/src/main/java/org/apache/druid/java/util/common/HumanReadableBytes.java
##########
@@ -208,4 +208,108 @@ private static long parseInner(String rawNumber)
throw new IAE("Invalid format or out of range of long: %s", rawNumber);
}
}
+
+ public enum UnitSystem
+ {
+ /**
+ * also known as IEC format
+ * eg: B, KiB, MiB, GiB ...
+ */
+ BINARY_BYTE,
+
+ /**
+ * also known as SI format
+ * eg: B, KB, MB ...
+ */
+ DECIMAL_BYTE,
+
+ /**
+ * simplified SI format without 'B' indicator
+ * eg: K, M, G ...
+ */
+ DECIMAL
+ }
+
+ /**
+ * Returns a human-readable string version of input value
+ *
+ * @param bytes input value. Negative value is also allowed
+ * @param precision [0,3]
+ * @param unitSystem which unit system is adopted to format the input value, see {@link UnitSystem}
+ * @param hasSpace if it's true, there's an extra space between the number and the unit suffix
+ */
+ public static String format(long bytes, int precision, UnitSystem unitSystem, boolean hasSpace)
Review comment:
IMO it's more of a dead param then and should be removed. It can be added back if druid ever exposes `hasSpace` to user in future.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-783123959
@FrankChen021 very sorry for the delayed response, I got a bit busy and haven't had a chance to get back to this PR until now. I think I agree that we shouldn't try to do a unified format function right now, I didn't mean to hoist the responsibility onto this PR (but am glad we had the discussion). I think the 3rd option of using the names for the functions @gianm suggested sgtm.
I think we can get this merged after the conflicts fixed and naming adjustments made. I still think you could adjust the tests to just expect the correct response based on the value of `druid.generic.useDefaultValueForNull` since travis runs both ways, which would make `NullHandling.updateForTests` not necessary [(this comment)](https://github.com/apache/druid/pull/10635#discussion_r547515782), but thats more of a nitpick on my end, and am +1 either way.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r660433584
##########
File path: docs/misc/math-expr.md
##########
@@ -258,3 +258,12 @@ supported features:
* math functions: `abs`, `acos`, `asin`, `atan`, `cbrt`, `ceil`, `cos`, `cosh`, `cot`, `exp`, `expm1`, `floor`, `getExponent`, `log`, `log10`, `log1p`, `nextUp`, `rint`, `signum`, `sin`, `sinh`, `sqrt`, `tan`, `tanh`, `toDegrees`, `toRadians`, `ulp`, `atan2`, `copySign`, `div`, `hypot`, `max`, `min`, `nextAfter`, `pow`, `remainder`, `scalb` are supported for numeric types
* time functions: `timestamp_floor` (with constant granularity argument) is supported for numeric types
* other: `parse_long` is supported for numeric and string types
+
+
+## Other functions
+
+| function | description |
+| --- | --- |
+| human_readable_binary_byte_format(value[, precision]) | Format a number in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. `precision` must be in the range of [0,3] (default: 2). For example:<li> human_readable_binary_byte_format(1048576) returns `1.00 MiB`</li><li>human_readable_binary_byte_format(1048576, 1) returns `1.000 MiB`</li> |
Review comment:
```suggestion
| human_readable_binary_byte_format(value[, precision]) | Format a number in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. `precision` must be in the range of [0,3] (default: 2). For example:<li> human_readable_binary_byte_format(1048576) returns `1.00 MiB`</li><li>human_readable_binary_byte_format(1048576, 1) returns `1.0 MiB`</li> |
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r535985635
##########
File path: core/src/test/java/org/apache/druid/java/util/common/HumanReadableBytesTest.java
##########
@@ -395,6 +395,107 @@ public void testBytesRange()
Assert.assertEquals("value must be in the range of [0, 5]", message);
}
+ @Test
+ public void testFormatInBinaryByte()
+ {
+ Assert.assertEquals("-8.00 EiB", HumanReadableBytes.format(Long.MIN_VALUE, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("-8.000 EiB", HumanReadableBytes.format(Long.MIN_VALUE, 3, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+
+ Assert.assertEquals("-2.00 GiB", HumanReadableBytes.format(Integer.MIN_VALUE, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("-32.00 KiB", HumanReadableBytes.format(Short.MIN_VALUE, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("-128.00 B", HumanReadableBytes.format(Byte.MIN_VALUE, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("-1.00 B", HumanReadableBytes.format(-1, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("0.00 B", HumanReadableBytes.format(0, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.00 B", HumanReadableBytes.format(1, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.00 KiB", HumanReadableBytes.format(1024L, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.00 MiB", HumanReadableBytes.format(1024L * 1024, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.00 GiB", HumanReadableBytes.format(1024L * 1024 * 1024, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.00 TiB", HumanReadableBytes.format(1024L * 1024 * 1024 * 1024, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.00 PiB", HumanReadableBytes.format(1024L * 1024 * 1024 * 1024 * 1024, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("8.00 EiB", HumanReadableBytes.format(Long.MAX_VALUE, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ }
+
+ @Test
+ public void testPrecisionInBinaryFormat()
+ {
+ Assert.assertEquals("1 KiB", HumanReadableBytes.format(1500, 0, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.5 KiB", HumanReadableBytes.format(1500, 1, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.46 KiB", HumanReadableBytes.format(1500, 2, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ Assert.assertEquals("1.465 KiB", HumanReadableBytes.format(1500, 3, HumanReadableBytes.UnitSystem.BINARY_BYTE, true));
+ }
+
+ @Test
+ public void testPrecisionInDecimalFormat()
+ {
+ Assert.assertEquals("1 KB", HumanReadableBytes.format(1456, 0, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.5 KB", HumanReadableBytes.format(1456, 1, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.46 KB", HumanReadableBytes.format(1456, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.456 KB", HumanReadableBytes.format(1456, 3, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ }
+
+ @Test
+ public void testFormatInDecimalByte()
+ {
+ Assert.assertEquals("1.00 B", HumanReadableBytes.format(1, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.00 KB", HumanReadableBytes.format(1000L, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.00 MB", HumanReadableBytes.format(1000L * 1000, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.00 GB", HumanReadableBytes.format(1000L * 1000 * 1000, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.00 TB", HumanReadableBytes.format(1000L * 1000 * 1000 * 1000, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("1.00 PB", HumanReadableBytes.format(1000L * 1000 * 1000 * 1000 * 1000, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("9.22 EB", HumanReadableBytes.format(Long.MAX_VALUE, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+
+ Assert.assertEquals("100.00 KB", HumanReadableBytes.format(99999, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("99.999 KB", HumanReadableBytes.format(99999, 3, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+
+ Assert.assertEquals("999.9 PB", HumanReadableBytes.format(999_949_999_999_999_999L, 1, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("999.95 PB", HumanReadableBytes.format(999_949_999_999_999_999L, 2, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ Assert.assertEquals("999.949 PB", HumanReadableBytes.format(999_949_999_999_999_999L, 3, HumanReadableBytes.UnitSystem.DECIMAL_BYTE, true));
+ }
+
+ @Test
+ public void testFormatInDecimal()
+ {
+ Assert.assertEquals("1.00 ", HumanReadableBytes.format(1, 2, HumanReadableBytes.UnitSystem.DECIMAL, true));
Review comment:
in this particular scenario, should space be appended?
##########
File path: core/src/main/java/org/apache/druid/java/util/common/HumanReadableBytes.java
##########
@@ -208,4 +208,108 @@ private static long parseInner(String rawNumber)
throw new IAE("Invalid format or out of range of long: %s", rawNumber);
}
}
+
+ public enum UnitSystem
+ {
+ /**
+ * also known as IEC format
+ * eg: B, KiB, MiB, GiB ...
+ */
+ BINARY_BYTE,
+
+ /**
+ * also known as SI format
+ * eg: B, KB, MB ...
+ */
+ DECIMAL_BYTE,
+
+ /**
+ * simplified SI format without 'B' indicator
+ * eg: K, M, G ...
+ */
+ DECIMAL
+ }
+
+ /**
+ * Returns a human-readable string version of input value
+ *
+ * @param bytes input value. Negative value is also allowed
+ * @param precision [0,3]
+ * @param unitSystem which unit system is adopted to format the input value, see {@link UnitSystem}
+ * @param hasSpace if it's true, there's an extra space between the number and the unit suffix
+ */
+ public static String format(long bytes, int precision, UnitSystem unitSystem, boolean hasSpace)
Review comment:
is the `hasSpace` customizable from outside? May be it can be just hard-coded to true.
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -16153,6 +16161,44 @@ public void testTimeStampAddConversion() throws Exception
);
}
+ public void testSizeFormatFunction() throws Exception
+ {
+ testQuery(
+ "SELECT m1, "
+ + "BINARY_BYTE_FORMAT(45678),"
+ + "BINARY_BYTE_FORMAT(m1*12345),"
+ + "BINARY_BYTE_FORMAT(m1*12345, 0), "
+ + "DECIMAL_BYTE_FORMAT(m1*12345), "
+ + "DECIMAL_FORMAT(m1*12345) "
+ + "FROM numfoo WHERE f1 = 0.1 LIMIT 1",
+ ImmutableList.of(
+ newScanQueryBuilder()
+ .dataSource(CalciteTests.DATASOURCE3)
+ .intervals(querySegmentSpec(Filtration.eternity()))
+ .virtualColumns(expressionVirtualColumn("v0", "'44.61KiB'", ValueType.STRING),
Review comment:
should this be?
```suggestion
.virtualColumns(expressionVirtualColumn("v0", "binary_date_format(45678)", ValueType.STRING),
```
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/SizeFormatOperatorConversion.java
##########
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.calcite.expression.builtin;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.sql.SqlCallBinding;
+import org.apache.calcite.sql.SqlFunction;
+import org.apache.calcite.sql.SqlFunctionCategory;
+import org.apache.calcite.sql.SqlOperandCountRange;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.type.SqlOperandCountRanges;
+import org.apache.calcite.sql.type.SqlOperandTypeChecker;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.sql.calcite.expression.DruidExpression;
+import org.apache.druid.sql.calcite.expression.OperatorConversions;
+import org.apache.druid.sql.calcite.expression.SqlOperatorConversion;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+
+public class SizeFormatOperatorConversion implements SqlOperatorConversion
+{
+ public static final SqlOperatorConversion BINARY_BYTE_FORMAT = new SizeFormatOperatorConversion("binary_byte_format");
+ public static final SqlOperatorConversion DECIMAL_BYTE_FORMAT = new SizeFormatOperatorConversion("decimal_byte_format");
+ public static final SqlOperatorConversion DECIMAL_FORMAT = new SizeFormatOperatorConversion("decimal_format");
+
+ private final String name;
+ private final SqlFunction sqlFunction;
+
+ private SizeFormatOperatorConversion(String name)
+ {
+ this.sqlFunction = OperatorConversions
+ .operatorBuilder(StringUtils.toUpperCase(name))
+ .operandTypeChecker(new StringFormatOperandTypeChecker())
+ .functionCategory(SqlFunctionCategory.STRING)
+ .returnTypeNonNull(SqlTypeName.VARCHAR)
+ .build();
+
+ this.name = name;
+ }
+
+ @Override
+ public SqlOperator calciteOperator()
+ {
+ return sqlFunction;
+ }
+
+ @Override
+ public DruidExpression toDruidExpression(
+ final PlannerContext plannerContext,
+ final RowSignature rowSignature,
+ final RexNode rexNode
+ )
+ {
+ return OperatorConversions.convertCall(plannerContext, rowSignature, rexNode, name);
+ }
+
+ private static class StringFormatOperandTypeChecker implements SqlOperandTypeChecker
+ {
+ @Override
+ public boolean checkOperandTypes(SqlCallBinding callBinding, boolean throwOnFailure)
+ {
+ final RelDataType firstArgType = callBinding.getOperandType(0);
Review comment:
shouldn't there be a check on `precision` as well?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r538150512
##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -3275,4 +3276,86 @@ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
return l.stream();
}
}
+
+ abstract class SizeFormatFunc implements Function
+ {
+ protected abstract HumanReadableBytes.UnitSystem getUnitSystem();
+
+ /**
+ * Evaluate given expression
+ * By default, 'precision' is 2 and 'hasSpace' is false
+ */
+ @Override
+ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
+ {
+ final long bytes = args.get(0).eval(bindings).asLong();
Review comment:
good point. I'll make some improvement here.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-738713331
Hi @abhishekagarwal87 , Thanks for your review. I will handle your comments on next Monday.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r554228178
##########
File path: docs/querying/sql.md
##########
@@ -563,6 +563,9 @@ The [DataSketches extension](../development/extensions-core/datasketches-extensi
|`COALESCE(value1, value2, ...)`|Returns the first value that is neither NULL nor empty string.|
|`NVL(expr,expr-for-null)`|Returns 'expr-for-null' if 'expr' is null (or empty string for string type).|
|`BLOOM_FILTER_TEST(<expr>, <serialized-filter>)`|Returns true if the value is contained in a Base64-serialized bloom filter. See the [Bloom filter extension](../development/extensions-core/bloom-filter.html) documentation for additional details.|
+|`BINARY_BYTE_FORMAT(value, [precision])`|Returns the value in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. Supported unit suffix: `B`, `KiB`, `MiB`, `GiB`, `TiB`, `PiB`, `EiB`. `precision` must be in the range of [0,3] (default: 2).|
+|`DECIMAL_BYTE_FORMAT(value, [precision])`|Returns the value in human-readable [SI](https://en.wikipedia.org/wiki/Binary_prefix) format. Supported unit suffix: `B`, `KB`, `MB`, `GB`, `TB`, `PB`, `EB`. `precision` must be in the range of [0,3] (default: 2).|
+|`DECIMAL_FORMAT(value, [precision])`|Returns the value in human-readable SI format. Supported unit suffix: `K`, `M`, `G`, `T`, `P`, `E`. `precision` must be in the range of [0,3] (default: 2).|
Review comment:
I think they make sense in the numeric section, because they accept numbers. It's OK that they don't return numbers. It's analogous to `TIME_FORMAT`, which still belongs in the time section.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-743124794
The failure of CI has nothing to do with changes in this PR. @clintropolis could you re-trigger CI ?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-757192914
>
> * 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?
Naming is really a big challenge in the world of programming :) . There're some ways listed as follows
* Alternative 1 - One function with different format specifier
format(number, 'human_readable_bin_byte')
format(number, 'human_readable_dec_byte')
format(number, 'human_readable_decimal')
It looks like the parameters are still too long.
* Alternative 2 - One function with abbreviated format specifier
format(number, 'si') -- si is the standard word for binary byte.
format(number, 'iec') -- iec is the standard word for decimal byte
format(number, 'dec')
If we want to design one function, the challenge is how to give nice names for format specifiers. But it also leaves the extensibility in the future to support other kinds of number format. Do you have nicer format specifier names ?
* Alternative 3 - based on @gianm 's suggestion
human_readable_format_bin_byte
human_readable_format_dec_byte
human_readable_format_dec
I think I would prefer the 3rd here. What do you think ? @gianm @clintropolis @abhishekagarwal87 ?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-783127765
@clintropolis Thanks for your response. I didn't got enough time to handle comments you left in the past month either. I will submit some commits to fix the conflicts, naming style and tests ASAP.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-739455997
Do you like how the web console auto compiles in the function docs? :-p
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #10635:
URL: https://github.com/apache/druid/pull/10635
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r660434509
##########
File path: docs/misc/math-expr.md
##########
@@ -258,3 +258,12 @@ supported features:
* math functions: `abs`, `acos`, `asin`, `atan`, `cbrt`, `ceil`, `cos`, `cosh`, `cot`, `exp`, `expm1`, `floor`, `getExponent`, `log`, `log10`, `log1p`, `nextUp`, `rint`, `signum`, `sin`, `sinh`, `sqrt`, `tan`, `tanh`, `toDegrees`, `toRadians`, `ulp`, `atan2`, `copySign`, `div`, `hypot`, `max`, `min`, `nextAfter`, `pow`, `remainder`, `scalb` are supported for numeric types
* time functions: `timestamp_floor` (with constant granularity argument) is supported for numeric types
* other: `parse_long` is supported for numeric and string types
+
+
+## Other functions
+
+| function | description |
+| --- | --- |
+| human_readable_binary_byte_format(value[, precision]) | Format a number in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. `precision` must be in the range of [0,3] (default: 2). For example:<li> human_readable_binary_byte_format(1048576) returns `1.00 MiB`</li><li>human_readable_binary_byte_format(1048576, 1) returns `1.000 MiB`</li> |
Review comment:
Thanks for your quick review. I found this bug and fixed it.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #10635:
URL: https://github.com/apache/druid/pull/10635#issuecomment-869463226
Hi @abhishekagarwal87 @gianm , do you have any other comments ?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r660356714
##########
File path: docs/misc/math-expr.md
##########
@@ -258,3 +258,12 @@ supported features:
* math functions: `abs`, `acos`, `asin`, `atan`, `cbrt`, `ceil`, `cos`, `cosh`, `cot`, `exp`, `expm1`, `floor`, `getExponent`, `log`, `log10`, `log1p`, `nextUp`, `rint`, `signum`, `sin`, `sinh`, `sqrt`, `tan`, `tanh`, `toDegrees`, `toRadians`, `ulp`, `atan2`, `copySign`, `div`, `hypot`, `max`, `min`, `nextAfter`, `pow`, `remainder`, `scalb` are supported for numeric types
* time functions: `timestamp_floor` (with constant granularity argument) is supported for numeric types
* other: `parse_long` is supported for numeric and string types
+
+
+## Other functions
+
+| function | description |
+| --- | --- |
+| human_readable_binary_byte_format(value[, precision]) | Format a number in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. For example, human_readable_binary_byte_format(1048576) returns `1.00 MiB`. `precision` must be in the range of [0,3] (default: 2). |
Review comment:
@FrankChen021 can you add one example with a custom precision value? Maybe, one of these examples can be modified itself.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r538058342
##########
File path: core/src/main/java/org/apache/druid/math/expr/Function.java
##########
@@ -3275,4 +3276,86 @@ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
return l.stream();
}
}
+
+ abstract class SizeFormatFunc implements Function
+ {
+ protected abstract HumanReadableBytes.UnitSystem getUnitSystem();
+
+ /**
+ * Evaluate given expression
+ * By default, 'precision' is 2 and 'hasSpace' is false
+ */
+ @Override
+ public ExprEval apply(List<Expr> args, Expr.ObjectBinding bindings)
+ {
+ final long bytes = args.get(0).eval(bindings).asLong();
Review comment:
Thinking out loud, how should this function behave with non-long inputs? The way this is currently implement:
* Inputs of `ExprType.DOUBLE` will be cast to a `ExprType.LONG` before conversion.
* For `ExprType.STRING` inputs, if they are number-ish strings, they will be parsed into long values, but if not will always be 0 if they are not parseable as numbers, since we are calling `asLong` without checking `isNumericNull` of the `ExprEval`, so we are ignoring the value of `druid.generic.useDefaultValueForNull`.
I don't know that this behavior is incorrect, I just wanted to call it out to think about it. I do think we want to check for `isNumericNull` and return `ExprEval.of(null)` if `NullHandling.sqlCompatible()` is set.
I see in the SQL operator it looks like it strictly validates that the inputs are numeric, while Druid native expressions have traditionally been a bit fast and loose about the inputs they accept and tend to be rather forgiving, so perhaps this is ok that the behavior here doesn't quite match.
##########
File path: docs/querying/sql.md
##########
@@ -563,6 +563,9 @@ The [DataSketches extension](../development/extensions-core/datasketches-extensi
|`COALESCE(value1, value2, ...)`|Returns the first value that is neither NULL nor empty string.|
|`NVL(expr,expr-for-null)`|Returns 'expr-for-null' if 'expr' is null (or empty string for string type).|
|`BLOOM_FILTER_TEST(<expr>, <serialized-filter>)`|Returns true if the value is contained in a Base64-serialized bloom filter. See the [Bloom filter extension](../development/extensions-core/bloom-filter.html) documentation for additional details.|
+|`BINARY_BYTE_FORMAT(value, [precision])`|Returns the value in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. Supported unit suffix: `B`, `KiB`, `MiB`, `GiB`, `TiB`, `PiB`, `EiB`. `precision` must be in the range of [0,3] (default: 2).|
+|`DECIMAL_BYTE_FORMAT(value, [precision])`|Returns the value in human-readable [SI](https://en.wikipedia.org/wiki/Binary_prefix) format. Supported unit suffix: `B`, `KB`, `MB`, `GB`, `TB`, `PB`, `EB`. `precision` must be in the range of [0,3] (default: 2).|
+|`DECIMAL_FORMAT(value, [precision])`|Returns the value in human-readable SI format. Supported unit suffix: `K`, `M`, `G`, `T`, `P`, `E`. `precision` must be in the range of [0,3] (default: 2).|
Review comment:
Should these docs live with the 'numeric' functions? I suppose here is ok too...
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r538148740
##########
File path: docs/querying/sql.md
##########
@@ -563,6 +563,9 @@ The [DataSketches extension](../development/extensions-core/datasketches-extensi
|`COALESCE(value1, value2, ...)`|Returns the first value that is neither NULL nor empty string.|
|`NVL(expr,expr-for-null)`|Returns 'expr-for-null' if 'expr' is null (or empty string for string type).|
|`BLOOM_FILTER_TEST(<expr>, <serialized-filter>)`|Returns true if the value is contained in a Base64-serialized bloom filter. See the [Bloom filter extension](../development/extensions-core/bloom-filter.html) documentation for additional details.|
+|`BINARY_BYTE_FORMAT(value, [precision])`|Returns the value in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. Supported unit suffix: `B`, `KiB`, `MiB`, `GiB`, `TiB`, `PiB`, `EiB`. `precision` must be in the range of [0,3] (default: 2).|
+|`DECIMAL_BYTE_FORMAT(value, [precision])`|Returns the value in human-readable [SI](https://en.wikipedia.org/wiki/Binary_prefix) format. Supported unit suffix: `B`, `KB`, `MB`, `GB`, `TB`, `PB`, `EB`. `precision` must be in the range of [0,3] (default: 2).|
+|`DECIMAL_FORMAT(value, [precision])`|Returns the value in human-readable SI format. Supported unit suffix: `K`, `M`, `G`, `T`, `P`, `E`. `precision` must be in the range of [0,3] (default: 2).|
Review comment:
I see all existing numeric functions are all about mathematical computation, the input parameter and output result are all type of integer or float. The new functions in this PR are a little bit different from those, I don't whether it's suitable to put them in 'numeric function' section, so I put them in a separated section.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r554228178
##########
File path: docs/querying/sql.md
##########
@@ -563,6 +563,9 @@ The [DataSketches extension](../development/extensions-core/datasketches-extensi
|`COALESCE(value1, value2, ...)`|Returns the first value that is neither NULL nor empty string.|
|`NVL(expr,expr-for-null)`|Returns 'expr-for-null' if 'expr' is null (or empty string for string type).|
|`BLOOM_FILTER_TEST(<expr>, <serialized-filter>)`|Returns true if the value is contained in a Base64-serialized bloom filter. See the [Bloom filter extension](../development/extensions-core/bloom-filter.html) documentation for additional details.|
+|`BINARY_BYTE_FORMAT(value, [precision])`|Returns the value in human-readable [IEC](https://en.wikipedia.org/wiki/Binary_prefix) format. Supported unit suffix: `B`, `KiB`, `MiB`, `GiB`, `TiB`, `PiB`, `EiB`. `precision` must be in the range of [0,3] (default: 2).|
+|`DECIMAL_BYTE_FORMAT(value, [precision])`|Returns the value in human-readable [SI](https://en.wikipedia.org/wiki/Binary_prefix) format. Supported unit suffix: `B`, `KB`, `MB`, `GB`, `TB`, `PB`, `EB`. `precision` must be in the range of [0,3] (default: 2).|
+|`DECIMAL_FORMAT(value, [precision])`|Returns the value in human-readable SI format. Supported unit suffix: `K`, `M`, `G`, `T`, `P`, `E`. `precision` must be in the range of [0,3] (default: 2).|
Review comment:
I think they make sense in the numeric section, because they accept numbers. It's OK that they don't return numbers. It's analogous to `TIME_FORMAT`, which still belongs in the time section.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a change in pull request #10635: Add SQL functions to format numbers into human readable format
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #10635:
URL: https://github.com/apache/druid/pull/10635#discussion_r537365555
##########
File path: core/src/main/java/org/apache/druid/java/util/common/HumanReadableBytes.java
##########
@@ -208,4 +208,108 @@ private static long parseInner(String rawNumber)
throw new IAE("Invalid format or out of range of long: %s", rawNumber);
}
}
+
+ public enum UnitSystem
+ {
+ /**
+ * also known as IEC format
+ * eg: B, KiB, MiB, GiB ...
+ */
+ BINARY_BYTE,
+
+ /**
+ * also known as SI format
+ * eg: B, KB, MB ...
+ */
+ DECIMAL_BYTE,
+
+ /**
+ * simplified SI format without 'B' indicator
+ * eg: K, M, G ...
+ */
+ DECIMAL
+ }
+
+ /**
+ * Returns a human-readable string version of input value
+ *
+ * @param bytes input value. Negative value is also allowed
+ * @param precision [0,3]
+ * @param unitSystem which unit system is adopted to format the input value, see {@link UnitSystem}
+ * @param hasSpace if it's true, there's an extra space between the number and the unit suffix
+ */
+ public static String format(long bytes, int precision, UnitSystem unitSystem, boolean hasSpace)
Review comment:
OK. It's been deleted.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org