Introduce Count unit by kokosing · Pull Request #8 · airlift/units

kokosing · 2018-10-31T13:52:58Z

Introduce Count unit

dain · 2018-11-01T20:49:03Z

src/main/java/io/airlift/units/Count.java

+        return new Count(size, unit).convertToMostSuccinctCount();
+    }
+
+    private final double value;


Do we want an inexact value for this? IIRC, a single long would get us to 4000 quadrillion.

I was rather thinking about case where something could have fraction part (like water or mass). Though I do not have strong opinion. Would you like me to change this?

I'm not sure. I can't think of any case where I would want to use this for non-integral counts. For water or mass, I assume we would introduce Volume and Mass units. Maybe @electrum or @martint have thoughts.

Notice that DataSize is also using double. Using double here is useful when doing a coversions between different units, like from KB to MB (as fractional part appears). The same happens for Count. When long is used then Count become precise more precise, but not all conversion are possible.

See #9.

Oh good point.

If were going to keep this pattern, I think we should make it clear that the value is inexact.

I renamed this class to Quantity so it now it clear that it may contain fractional part. Also now, it is a bit more general.

kokosing · 2018-11-05T06:19:44Z

Automation failure does not seem related.

kokosing · 2018-11-05T06:21:27Z

@electrum Would you mind to take a look?

kokosing · 2019-01-04T08:19:49Z

@electrum, @dain Ping, it blocks: prestodb/presto#11443

dain

I only skimmed the high level, and it looks good to me. But, I think the needs more eyes due to the difficult in changing this in the future. Specifically, I think @electrum and @martint should read this over.

dain · 2019-01-04T21:57:22Z

src/main/java/io/airlift/units/Count.java

+    // call allocates a new array at each call.
+    private static final Unit[] COUNT_UNITS = Unit.values();
+
+    public static Count succinct(long bytes)


bytes :)

Maybe rename this to count?

dain · 2019-01-04T21:59:39Z

src/main/java/io/airlift/units/Count.java

+        return new Count(size, unit).convertToMostSuccinctCount();
+    }
+
+    private final double value;


If were going to keep this pattern, I think we should make it clear that the value is inexact.

dain · 2019-01-04T22:02:24Z

src/main/java/io/airlift/units/Count.java

+    {
+        //This order is important, it should be in increasing magnitude.
+        SINGLE(1L, ""),
+        THOUSAND(1000L, "k"),


Do we want to use a lowercase k here?

I took from here: https://en.wikipedia.org/wiki/Power_of_10

They mention both ways. Changed.

kokosing

Comments addressed

kokosing · 2019-01-14T14:30:57Z

src/main/java/io/airlift/units/Count.java

+    // call allocates a new array at each call.
+    private static final Unit[] COUNT_UNITS = Unit.values();
+
+    public static Count succinct(long bytes)


kokosing · 2019-01-14T14:31:43Z

src/main/java/io/airlift/units/Count.java

+        return new Count(size, unit).convertToMostSuccinctCount();
+    }
+
+    private final double value;


I renamed this class to Quantity so it now it clear that it may contain fractional part. Also now, it is a bit more general.

kokosing · 2019-01-14T14:33:37Z

src/main/java/io/airlift/units/Count.java

+    {
+        //This order is important, it should be in increasing magnitude.
+        SINGLE(1L, ""),
+        THOUSAND(1000L, "k"),


I took from here: https://en.wikipedia.org/wiki/Power_of_10

They mention both ways. Changed.

kokosing · 2019-01-14T14:36:54Z

think the needs more eyes due to the difficult in changing this in the future

Why is so? I think it will be difficult to change if we get a vast usage. I hope that along the way the usage is growing we will fix all the issues.

kokosing · 2019-01-30T09:36:39Z

@electrum @martint ping

martint · 2019-02-05T03:34:40Z

I'm ok with having something like this in airlift, but please let's not call it a "count unit". There's no unit involved here (e.g., kilogram, meters, dollars, etc). What we're talking about is a scaled count, or a count with scale.

martint · 2019-02-05T03:34:57Z

src/main/java/io/airlift/units/Count.java

+        return Double.hashCode(value);
+    }
+
+    public enum Unit


This is not a unit. It's a scale or magnitude.

kokosing · 2019-02-05T10:35:35Z

but please let's not call it a "count unit"

To me it is some kind of "unit" as it describes certain physical property. Anyway, I liked Magnitude much better, thanks for pointing this.

kokosing · 2019-02-05T10:35:47Z

Addressed comments.

electrum

Some initial comments.

I'm also not sure the parsing and formatting semantics are right. If we don't have fractions, then six digits are required to accurately represent anything. For example, 123456 cannot be converted to K without using fractions or losing precision. But we could have 123.456K without losing accuracy (since we are doing base 10 magnitudes).

electrum · 2019-02-25T05:09:49Z

src/main/java/io/airlift/units/Count.java

+    }
+
+    /**
+     * @return count with no bigger value than 1000 in succinct magnitude, fractional part is rounded


How is there a fractional part when value is a long?

When you pass 123456 in SINGLE then you will get 123.456K that will be rounded to 123K.

electrum · 2019-02-25T05:10:30Z

src/main/java/io/airlift/units/Count.java

+
+    public Count(long count, Magnitude magnitude)
+    {
+        checkArgument(!Double.isInfinite(count), "count is infinite");


Both these checks are not needed for long

electrum · 2019-02-25T05:12:23Z

src/main/java/io/airlift/units/Count.java

+
+    public enum Magnitude
+    {
+        //This order is important, it should be in increasing magnitude.


How about

// must be in increasing magnitude order

electrum · 2019-02-25T05:13:20Z

src/test/java/io/airlift/units/TestCount.java

+    }
+
+    @Test(dataProvider = "conversions")
+    public void testConversions(Count.Magnitude magnitude, Count.Magnitude toMagnitude, long factor)


Static import Magnitude

electrum · 2019-02-25T05:20:22Z

src/main/java/io/airlift/units/Count.java

+        QUADRILLION(1000_000_000_000_000L, "P");
+
+        private final long factor;
+        private final String magnitutdeString;


Typo magnitutdeString (also in constructor argument, message below, and getter)

electrum · 2019-02-25T05:26:39Z

src/main/java/io/airlift/units/Count.java

+
+    public Count convertTo(Magnitude magnitude)
+    {
+        requireNonNull(magnitude, "magnitude is null");


Move this check to getValue (or remove it since the caller will get an NPE anyway)

electrum · 2019-02-25T05:29:58Z

src/main/java/io/airlift/units/Count.java

+    @Override
+    public int hashCode()
+    {
+        return Double.hashCode(getValue(Magnitude.SINGLE));


Same comment as compareTo

electrum · 2019-02-25T05:31:12Z

src/main/java/io/airlift/units/Count.java

+import static java.lang.String.format;
+import static java.util.Objects.requireNonNull;
+
+public class Count


Make this final (DataSize should have been final as well)

electrum · 2019-02-25T05:31:14Z

src/main/java/io/airlift/units/Count.java

+    @Override
+    public int compareTo(Count o)
+    {
+        return Double.compare(getValue(Magnitude.SINGLE), o.getValue(Magnitude.SINGLE));


This double comparison is wrong. However, using getValue() won't work, since it can fail on overflow. Maybe we should have a toBigInteger() method and use that here.

electrum · 2019-02-25T05:34:17Z

src/main/java/io/airlift/units/Count.java

+            return (long) (floor(value)) + magnitude.getMagnitutdeString();
+        }
+
+        return format(Locale.ENGLISH, "%.2f%s", value, magnitude.getMagnitutdeString());


This formatting seems wrong for long.

kokosing · 2019-02-26T10:08:58Z

For example, 123456 cannot be converted to K without using fractions or losing precision.

Any conversion that loses a precision throws an exception, unless it is says it rounds like succinctRounded.

But we could have 123.456K without losing accuracy (since we are doing base 10 magnitudes).

Do you mean to always store value raw value (in magnitude SINGLE) and convert (format) it only when displaying.

kokosing · 2019-02-26T10:09:36Z

Comments addressed. Thank you for a review.

nineinchnick · 2022-02-23T11:40:15Z

src/main/java/io/airlift/units/Count.java

+        MILLION(1000_000L, "M"),
+        BILLION(1000_000_000L, "B"),
+        TRILION(1000_000_000_000L, "T"),
+        QUADRILLION(1000_000_000_000_000L, "P");


This is not consistent with using B for Billion. How about Q?

Suggested change

QUADRILLION(1000_000_000_000_000L, "P");

QUADRILLION(1000_000_000_000_000L, "Q");

dain reviewed Nov 1, 2018

View reviewed changes

kokosing mentioned this pull request Nov 5, 2018

Print cost metrics as data size prestodb/presto#11443

Closed

dain reviewed Jan 4, 2019

View reviewed changes

dain assigned electrum and martint Jan 4, 2019

dain requested review from electrum and martint January 4, 2019 22:06

kokosing commented Jan 14, 2019

View reviewed changes

kokosing mentioned this pull request Jan 25, 2019

Print cost metrics in units trinodb/trino#68

Merged

martint reviewed Feb 5, 2019

View reviewed changes

kokosing force-pushed the origin/master/001_count branch from f48c33b to 9f22896 Compare February 5, 2019 10:35

kokosing force-pushed the origin/master/001_count branch from 9f22896 to 86a3afd Compare February 8, 2019 08:27

electrum reviewed Feb 25, 2019

View reviewed changes

kokosing added 2 commits February 26, 2019 10:30

Make DataSize final

b7fb250

Introduce Count

b805a0a

kokosing force-pushed the origin/master/001_count branch from 86a3afd to b805a0a Compare February 26, 2019 09:53

nineinchnick reviewed Feb 23, 2022

View reviewed changes

	QUADRILLION(1000_000_000_000_000L, "P");
	QUADRILLION(1000_000_000_000_000L, "Q");

Conversation

kokosing commented Oct 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokosing commented Nov 5, 2018

Uh oh!

kokosing commented Nov 5, 2018

Uh oh!

kokosing commented Jan 4, 2019

Uh oh!

dain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokosing commented Jan 14, 2019

Uh oh!

kokosing commented Jan 30, 2019

Uh oh!

martint commented Feb 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokosing commented Feb 5, 2019

Uh oh!

kokosing commented Feb 5, 2019

Uh oh!

electrum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!