Fix test for invalid rate in FormatUtils.formatDataRate() #15337

metadaddy · 2022-12-07T17:54:40Z

Description

FormatUtils.formatDataRate() contains the following code:

long rate = Math.round(dataSize.toBytes() / duration.getValue(SECONDS));
if (Double.isNaN(rate) || Double.isInfinite(rate)) {
    rate = 0;
}

The test can never evaluate as true, since both functions will return false for any long.

The correct test is:

if (rate == Long.MAX_VALUE) {
    rate = 0;
}

It's not necessary to test for Long.MIN_VALUE, since neither dataSize nor duration can be negative.

Unit Test

I added a new class, TestFormatUtils to test both this fix and the rest of FormatUtils. The tests for this change are:

assertEquals(FormatUtils.formatDataRate(DataSize.ofBytes(1), Duration.valueOf("0s"), false), "0B");
assertEquals(FormatUtils.formatDataRate(DataSize.ofBytes(1), Duration.valueOf("0s"), true), "0B/s");

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

Related Issue

Fixes #13093

client/trino-cli/src/main/java/io/trino/cli/FormatUtils.java

findepi · 2022-12-07T21:35:21Z

client/trino-cli/src/main/java/io/trino/cli/FormatUtils.java

@@ -115,7 +115,7 @@ else if (longForm) {
    public static String formatDataRate(DataSize dataSize, Duration duration, boolean longForm)
    {
        long rate = Math.round(dataSize.toBytes() / duration.getValue(SECONDS));
-        if (Double.isNaN(rate) || Double.isInfinite(rate)) {
+        if (rate == Long.MAX_VALUE) {


I am not sure there is a reason to special case the long max value.
Maybe we should remove the if block altogether?

Math.round returns Long.MAX_VALUE for positive infinity and Long.MIN_VALUE for negative infinity. Double.NaN returns 0. The check looks correct to me since dataSize.toBytes() can only return positive values, making negative infinity impossible.

The check against Long.MAX_VALUE preserves the intended semantics (and mirrors those of formatCountRate()) - if the rate calculation results in 'infinity', then set it to zero.

Are we printing "oh, so big a rate that I can't handle it" and "nothing is happening" the same way?

Yes - this is how formatCountRate works. I'm merely fixing the check here so it works as intended.

I'm pretty sure the only way to get infinity (and thus max long) is if the duration is effectively zero. In that case I think printing a zero rate is fine. It will quickly update to a real value.

findepi · 2022-12-07T21:36:49Z

client/trino-cli/src/test/java/io/trino/cli/TestFormatUtils.java

+    }
+
+    @Test
+    public void testPluralize()


that's awesome you're adding this test class.
note that it covers lot more than just the fix itself

what about introducing the test class as a prep commit
and then only doing the change (that should be reflected with only a small, related test change)

@findepi Two PRs, or one PR containing two commits?

I'd do 1 PR containing two commits.

client/trino-cli/src/test/java/io/trino/cli/TestFormatUtils.java

findepi · 2022-12-07T21:38:31Z

client/trino-cli/src/test/java/io/trino/cli/TestFormatUtils.java

+    @Test
+    public void testFormatCount()
+    {
+        assertEquals(FormatUtils.formatCount(1L), "1");


nit: you can import FormatUtils method statically

i know this will make it less clear that we're invking FormatUtils methods
but it will improve readability overall

metadaddy · 2022-12-08T02:01:53Z

Thanks for the reviews! I've made the suggested changes; @findepi - just let me know whether you're looking for two PRs, or one PR with two commits and I'll do another push.

metadaddy · 2022-12-08T02:41:07Z

@findepi I refactored to two commits and force-pushed them. Let me know if you need two PRs.

findepi · 2022-12-08T09:51:33Z

client/trino-cli/src/test/java/io/trino/cli/TestFormatUtils.java

@@ -275,6 +275,9 @@ public void testFormatDataRate()
        assertEquals(formatDataRate(DataSize.ofBytes(864000), Duration.valueOf("10d"), true), "1B/s");
        assertEquals(formatDataRate(DataSize.ofBytes(8640000), Duration.valueOf("10d"), false), "10B");
        assertEquals(formatDataRate(DataSize.ofBytes(8640000), Duration.valueOf("10d"), true), "10B/s");
+
+        assertEquals(formatDataRate(DataSize.ofBytes(1), Duration.valueOf("0s"), false), "0B");


If we eg transferred 1 MB within "zero time" (which isn't impossible given clock granularity), why would we want to print 0?

I don't know; I'm simply honoring the intent of the original code written by @electrum

metadaddy · 2022-12-08T17:16:07Z

Force-pushed the fix for the failing maven check (import static io.trino.cli.FormatUtils.*)

mosabua · 2024-01-11T23:18:44Z

@findepi @metadaddy @pettyjamesm can you continue to collaborate on this PR please?

github-actions · 2024-09-04T17:06:24Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-09-05T17:04:33Z

Does this PR need anything besides a merge @findepi and @pettyjamesm ?

pettyjamesm · 2024-09-05T20:40:21Z

Does this PR need anything besides a merge @findepi and @pettyjamesm ?

Nothing on my end, @findepi had some feedback and conversations going after I had approved it- so I defer to him on merge readiness, although if he’s not available I can pick it up and re-review.

github-actions · 2024-09-30T17:38:00Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-10-17T21:16:29Z

Could you rebase @metadaddy .. maybe we can find someone to look and get this merged after.

metadaddy · 2024-10-18T01:25:02Z

Hi @mosabua - I rebased, as requested 👍🏻

dain · 2024-10-18T20:16:22Z

client/trino-cli/src/main/java/io/trino/cli/FormatUtils.java

@@ -115,7 +115,7 @@ else if (longForm) {
    public static String formatDataRate(DataSize dataSize, Duration duration, boolean longForm)
    {
        long rate = Math.round(dataSize.toBytes() / duration.getValue(SECONDS));
-        if (Double.isNaN(rate) || Double.isInfinite(rate)) {
+        if (rate == Long.MAX_VALUE) {


I'm pretty sure the only way to get infinity (and thus max long) is if the duration is effectively zero. In that case I think printing a zero rate is fine. It will quickly update to a real value.

cla-bot bot added the cla-signed label Dec 7, 2022

metadaddy mentioned this pull request Dec 7, 2022

io.trino.cli.FormatUtils.formatDataRate incorrectly tests long rate with Double functions #13093

Closed

metadaddy requested a review from findepi December 7, 2022 17:55

findepi reviewed Dec 7, 2022

View reviewed changes

findepi requested a review from pettyjamesm December 7, 2022 21:38

pettyjamesm approved these changes Dec 7, 2022

View reviewed changes

metadaddy force-pushed the fix-formatdatarate-test branch from 5253041 to a3ff146 Compare December 8, 2022 02:39

findepi reviewed Dec 8, 2022

View reviewed changes

metadaddy force-pushed the fix-formatdatarate-test branch from a3ff146 to ed8e1a6 Compare December 8, 2022 17:15

findepi requested a review from electrum December 9, 2022 22:35

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare March 14, 2023 11:34

github-actions bot added the stale label Sep 4, 2024

github-actions bot removed the stale label Sep 6, 2024

github-actions bot added the stale label Sep 30, 2024

Fix check for invalid rate in FormatUtils.formatDataRate()

adb19f6

metadaddy force-pushed the fix-formatdatarate-test branch from ed8e1a6 to adb19f6 Compare October 18, 2024 00:48

github-actions bot removed the stale label Oct 18, 2024

dain approved these changes Oct 18, 2024

View reviewed changes

dain merged commit ff794d2 into trinodb:master Oct 18, 2024
40 checks passed

github-actions bot added this to the 463 milestone Oct 18, 2024

mosabua mentioned this pull request Oct 18, 2024

Add Trino 463 release notes #23810

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test for invalid rate in FormatUtils.formatDataRate() #15337

Fix test for invalid rate in FormatUtils.formatDataRate() #15337

metadaddy commented Dec 7, 2022

findepi Dec 7, 2022

pettyjamesm Dec 7, 2022

metadaddy Dec 8, 2022

findepi Dec 8, 2022

metadaddy Dec 8, 2022

dain Oct 18, 2024

findepi Dec 7, 2022

metadaddy Dec 8, 2022

findepi Dec 8, 2022

metadaddy Dec 8, 2022

findepi Dec 7, 2022

metadaddy Dec 8, 2022

metadaddy commented Dec 8, 2022

metadaddy commented Dec 8, 2022

findepi Dec 8, 2022

metadaddy Dec 8, 2022

metadaddy commented Dec 8, 2022

mosabua commented Jan 11, 2024

github-actions bot commented Sep 4, 2024

mosabua commented Sep 5, 2024

pettyjamesm commented Sep 5, 2024

github-actions bot commented Sep 30, 2024

mosabua commented Oct 17, 2024

metadaddy commented Oct 18, 2024

dain Oct 18, 2024

Fix test for invalid rate in FormatUtils.formatDataRate() #15337

Fix test for invalid rate in FormatUtils.formatDataRate() #15337

Conversation

metadaddy commented Dec 7, 2022

Description

Unit Test

Release notes

Related Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metadaddy commented Dec 8, 2022

metadaddy commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metadaddy commented Dec 8, 2022

mosabua commented Jan 11, 2024

github-actions bot commented Sep 4, 2024

mosabua commented Sep 5, 2024

pettyjamesm commented Sep 5, 2024

github-actions bot commented Sep 30, 2024

mosabua commented Oct 17, 2024

metadaddy commented Oct 18, 2024

Choose a reason for hiding this comment