Copilot commented on code in PR #18734:
URL: https://github.com/apache/datafusion/pull/18734#discussion_r2530928828
##########
datafusion/execution/src/memory_pool/mod.rs:
##########
@@ -599,4 +649,57 @@ mod tests {
assert_eq!(r2.size(), 25);
assert_eq!(pool.reserved(), 28);
}
+
+ #[test]
+ fn test_human_readable_count() {
+ // Test small numbers (< 1000) - should display as-is
+ assert_eq!(human_readable_count(0), "0");
+ assert_eq!(human_readable_count(1), "1");
+ assert_eq!(human_readable_count(999), "999");
+
+ // Test thousands (K)
+ assert_eq!(human_readable_count(1_000), "1.00 K");
+ assert_eq!(human_readable_count(10_100), "10.10 K");
+ assert_eq!(human_readable_count(1_532), "1.53 K");
+ assert_eq!(human_readable_count(99_999), "100.00 K");
+
+ // Test millions (M)
+ assert_eq!(human_readable_count(1_000_000), "1.00 M");
+ assert_eq!(human_readable_count(1_532_000), "1.53 M");
+ assert_eq!(human_readable_count(99_000_000), "99.00 M");
+ assert_eq!(human_readable_count(123_456_789), "123.5 M");
+
+ // Test billions (B)
+ assert_eq!(human_readable_count(1_000_000_000), "1.00 B");
+ assert_eq!(human_readable_count(1_532_000_000), "1.53 B");
+ assert_eq!(human_readable_count(999_999_999_999), "1000.0 B");
Review Comment:
[nitpick] The test expects `999_999_999_999` (just under 1 trillion) to
format as "1000.0 B" (1000 billion). This creates an awkward display where the
value exceeds the typical range for the unit (B for billions). Consider
updating the threshold logic to switch to the next unit (T) when the formatted
value would be >= 1000 in the current unit.
For example, values >= 999.95 billion could round to "1000.0 B" - it would
be clearer to display this as "1.00 T" instead.
##########
datafusion/execution/src/memory_pool/mod.rs:
##########
@@ -599,4 +649,57 @@ mod tests {
assert_eq!(r2.size(), 25);
assert_eq!(pool.reserved(), 28);
}
+
+ #[test]
+ fn test_human_readable_count() {
+ // Test small numbers (< 1000) - should display as-is
+ assert_eq!(human_readable_count(0), "0");
+ assert_eq!(human_readable_count(1), "1");
+ assert_eq!(human_readable_count(999), "999");
+
+ // Test thousands (K)
+ assert_eq!(human_readable_count(1_000), "1.00 K");
+ assert_eq!(human_readable_count(10_100), "10.10 K");
+ assert_eq!(human_readable_count(1_532), "1.53 K");
+ assert_eq!(human_readable_count(99_999), "100.00 K");
+
+ // Test millions (M)
+ assert_eq!(human_readable_count(1_000_000), "1.00 M");
+ assert_eq!(human_readable_count(1_532_000), "1.53 M");
+ assert_eq!(human_readable_count(99_000_000), "99.00 M");
+ assert_eq!(human_readable_count(123_456_789), "123.5 M");
+
+ // Test billions (B)
+ assert_eq!(human_readable_count(1_000_000_000), "1.00 B");
+ assert_eq!(human_readable_count(1_532_000_000), "1.53 B");
+ assert_eq!(human_readable_count(999_999_999_999), "1000.0 B");
+
+ // Test trillions (T)
+ assert_eq!(human_readable_count(1_000_000_000_000), "1.00 T");
+ assert_eq!(human_readable_count(42_000_000_000_000), "42.00 T");
+ }
+
+ #[test]
+ fn test_human_readable_duration() {
+ // Test nanoseconds (< 1µs)
+ assert_eq!(human_readable_duration(0), "0ns");
+ assert_eq!(human_readable_duration(1), "1ns");
+ assert_eq!(human_readable_duration(999), "999ns");
+
+ // Test microseconds (1µs to < 1ms)
+ assert_eq!(human_readable_duration(1_000), "1.00µs");
+ assert_eq!(human_readable_duration(1_234), "1.23µs");
+ assert_eq!(human_readable_duration(999_999), "1000.00µs");
Review Comment:
[nitpick] The test expects `999_999` nanoseconds to format as "1000.00µs"
instead of "1.00ms". This creates an awkward display where the value exceeds
the typical range for the unit. Consider updating the threshold logic to switch
to the next unit when the formatted value would be >= 1000 in the current unit.
Similar issue occurs on line 698 with `999_999_999` nanoseconds formatting
as "1000.00ms" instead of "1.00s".
##########
datafusion/execution/src/memory_pool/mod.rs:
##########
@@ -599,4 +649,57 @@ mod tests {
assert_eq!(r2.size(), 25);
assert_eq!(pool.reserved(), 28);
}
+
+ #[test]
+ fn test_human_readable_count() {
+ // Test small numbers (< 1000) - should display as-is
+ assert_eq!(human_readable_count(0), "0");
+ assert_eq!(human_readable_count(1), "1");
+ assert_eq!(human_readable_count(999), "999");
+
+ // Test thousands (K)
+ assert_eq!(human_readable_count(1_000), "1.00 K");
+ assert_eq!(human_readable_count(10_100), "10.10 K");
+ assert_eq!(human_readable_count(1_532), "1.53 K");
+ assert_eq!(human_readable_count(99_999), "100.00 K");
+
+ // Test millions (M)
+ assert_eq!(human_readable_count(1_000_000), "1.00 M");
+ assert_eq!(human_readable_count(1_532_000), "1.53 M");
+ assert_eq!(human_readable_count(99_000_000), "99.00 M");
+ assert_eq!(human_readable_count(123_456_789), "123.5 M");
+
+ // Test billions (B)
+ assert_eq!(human_readable_count(1_000_000_000), "1.00 B");
+ assert_eq!(human_readable_count(1_532_000_000), "1.53 B");
+ assert_eq!(human_readable_count(999_999_999_999), "1000.0 B");
+
+ // Test trillions (T)
+ assert_eq!(human_readable_count(1_000_000_000_000), "1.00 T");
+ assert_eq!(human_readable_count(42_000_000_000_000), "42.00 T");
+ }
+
+ #[test]
+ fn test_human_readable_duration() {
+ // Test nanoseconds (< 1µs)
+ assert_eq!(human_readable_duration(0), "0ns");
+ assert_eq!(human_readable_duration(1), "1ns");
+ assert_eq!(human_readable_duration(999), "999ns");
+
+ // Test microseconds (1µs to < 1ms)
+ assert_eq!(human_readable_duration(1_000), "1.00µs");
+ assert_eq!(human_readable_duration(1_234), "1.23µs");
+ assert_eq!(human_readable_duration(999_999), "1000.00µs");
+
+ // Test milliseconds (1ms to < 1s)
+ assert_eq!(human_readable_duration(1_000_000), "1.00ms");
+ assert_eq!(human_readable_duration(11_295_377), "11.30ms");
+ assert_eq!(human_readable_duration(1_234_567), "1.23ms");
+ assert_eq!(human_readable_duration(999_999_999), "1000.00ms");
Review Comment:
[nitpick] Similar to line 692, this test expects `999_999_999` nanoseconds
to format as "1000.00ms" instead of "1.00s", which creates an awkward display
exceeding the typical range for the milliseconds unit.
```suggestion
assert_eq!(human_readable_duration(999_999_999), "1.00s");
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
