From 16360a0846931cd3c4acff59d2e1242270b69fe4 Mon Sep 17 00:00:00 2001 From: daguimu Date: Wed, 25 Mar 2026 21:13:38 +0800 Subject: [PATCH] fix: MapComparator integer overflow causes SQL stat sort to fail The Long/Date/Number comparisons in MapComparator used (int)(a - b) which overflows for large values, violating the Comparator contract. This causes Collections.sort to produce unpredictable results that don't change between ascending and descending, making the SQL monitoring page sort appear broken. Replace with Long.compare/Double.compare which handle all value ranges correctly. Fixes #6624 Co-Authored-By: Claude Opus 4.6 (1M context) --- .../com/alibaba/druid/util/MapComparator.java | 6 +- .../druid/util/MapComparatorOverflowTest.java | 151 ++++++++++++++++++ 2 files changed, 154 insertions(+), 3 deletions(-) create mode 100644 core/src/test/java/com/alibaba/druid/util/MapComparatorOverflowTest.java diff --git a/core/src/main/java/com/alibaba/druid/util/MapComparator.java b/core/src/main/java/com/alibaba/druid/util/MapComparator.java index 985830e253..808f1c77ab 100644 --- a/core/src/main/java/com/alibaba/druid/util/MapComparator.java +++ b/core/src/main/java/com/alibaba/druid/util/MapComparator.java @@ -34,7 +34,7 @@ public MapComparator(K orderByKey, boolean isDesc) { } private int compare(Number o1, Number o2) { - return (int) (o1.doubleValue() - o2.doubleValue()); + return Double.compare(o1.doubleValue(), o2.doubleValue()); } private int compare(String o1, String o2) { @@ -42,7 +42,7 @@ private int compare(String o1, String o2) { } private int compare(Date o1, Date o2) { - return (int) (o1.getTime() - o2.getTime()); + return Long.compare(o1.getTime(), o2.getTime()); } @Override @@ -99,7 +99,7 @@ private int compare_0(Map o1, Map o2) { } if (v1 instanceof Long) { - return (int) (((Long) v1).longValue() - ((Number) v2).longValue()); + return Long.compare(((Long) v1).longValue(), ((Number) v2).longValue()); } if (v1 instanceof Number) { diff --git a/core/src/test/java/com/alibaba/druid/util/MapComparatorOverflowTest.java b/core/src/test/java/com/alibaba/druid/util/MapComparatorOverflowTest.java new file mode 100644 index 0000000000..77d2609a95 --- /dev/null +++ b/core/src/test/java/com/alibaba/druid/util/MapComparatorOverflowTest.java @@ -0,0 +1,151 @@ +package com.alibaba.druid.util; + +import org.junit.jupiter.api.Test; + +import java.util.*; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * Fix MapComparator sorting failures due to integer overflow in Long/Date comparisons. + *

+ * The previous implementation used {@code (int)(longA - longB)} which overflows for large + * values, violating the Comparator contract and causing incorrect sort order. + * This manifests as the SQL monitoring page sort not working (descending has no effect). + * + * @see Issue #6624 + */ +public class MapComparatorOverflowTest { + @Test + public void test_long_sort_ascending_large_values() { + List> list = new ArrayList<>(); + long[] values = {Long.MAX_VALUE, 1L, Long.MAX_VALUE / 2, 100L, Long.MAX_VALUE - 1}; + for (long v : values) { + Map m = new HashMap<>(); + m.put("k", v); + list.add(m); + } + + Collections.sort(list, new MapComparator<>("k", false)); + + long prev = Long.MIN_VALUE; + for (Map m : list) { + long val = (Long) m.get("k"); + assertTrue(val >= prev, "ASC sort failed: " + val + " < " + prev); + prev = val; + } + } + + @Test + public void test_long_sort_descending_large_values() { + List> list = new ArrayList<>(); + long[] values = {Long.MAX_VALUE, 1L, Long.MAX_VALUE / 2, 100L, Long.MAX_VALUE - 1}; + for (long v : values) { + Map m = new HashMap<>(); + m.put("k", v); + list.add(m); + } + + Collections.sort(list, new MapComparator<>("k", true)); + + long prev = Long.MAX_VALUE; + for (Map m : list) { + long val = (Long) m.get("k"); + assertTrue(val <= prev, "DESC sort failed: " + val + " > " + prev); + prev = val; + } + } + + @Test + public void test_long_sort_desc_differs_from_asc() { + // Reproduce the exact issue #6624: desc sort should produce different order than asc + List> data = new ArrayList<>(); + for (long v : new long[]{500L, 100L, 300L, 200L, 400L}) { + Map m = new HashMap<>(); + m.put("MaxTimespan", v); + data.add(m); + } + + List> ascList = new ArrayList<>(data); + Collections.sort(ascList, new MapComparator<>("MaxTimespan", false)); + + List> descList = new ArrayList<>(data); + Collections.sort(descList, new MapComparator<>("MaxTimespan", true)); + + // ASC first should be smallest + assertEquals(100L, ascList.get(0).get("MaxTimespan")); + // DESC first should be largest + assertEquals(500L, descList.get(0).get("MaxTimespan")); + + // ASC and DESC should be reverse of each other + for (int i = 0; i < data.size(); i++) { + assertEquals(ascList.get(i).get("MaxTimespan"), + descList.get(data.size() - 1 - i).get("MaxTimespan")); + } + } + + @Test + public void test_long_overflow_values() { + // Values that previously caused int overflow: (int)(3000000000 - 1) = -1294967297 + MapComparator asc = new MapComparator<>("k", false); + + Map big = new HashMap<>(); + big.put("k", 3000000000L); + Map small = new HashMap<>(); + small.put("k", 1L); + + assertTrue(asc.compare(big, small) > 0, "3B should be > 1"); + assertTrue(asc.compare(small, big) < 0, "1 should be < 3B"); + } + + @Test + public void test_date_sort_large_time_difference() { + MapComparator asc = new MapComparator<>("k", false); + MapComparator desc = new MapComparator<>("k", true); + + Map recent = new HashMap<>(); + recent.put("k", new Date(System.currentTimeMillis())); + Map old = new HashMap<>(); + old.put("k", new Date(0L)); // epoch + + assertTrue(asc.compare(recent, old) > 0, "ASC: recent should be > epoch"); + assertTrue(desc.compare(recent, old) < 0, "DESC: recent should be < epoch"); + } + + @Test + public void test_number_small_difference_precision() { + // Previously: (int)(100.5 - 100.0) = (int)0.5 = 0 (treated as equal) + MapComparator asc = new MapComparator<>("k", false); + + Map a = new HashMap<>(); + a.put("k", 100.5); + Map b = new HashMap<>(); + b.put("k", 100.0); + + assertTrue(asc.compare(a, b) > 0, "100.5 should be > 100.0"); + } + + @Test + public void test_random_long_sort_consistency() { + // Ensure sort doesn't throw IllegalArgumentException from TimSort + // due to Comparator contract violation (transitivity) + Random random = new Random(42); + List> list = new ArrayList<>(); + for (int i = 0; i < 100; i++) { + Map m = new HashMap<>(); + m.put("k", random.nextLong() & Long.MAX_VALUE); + list.add(m); + } + + assertDoesNotThrow(() -> + Collections.sort(list, new MapComparator<>("k", true))); + + // Verify the result is actually sorted descending + long prev = Long.MAX_VALUE; + for (Map m : list) { + long val = (Long) m.get("k"); + assertTrue(val <= prev, "Random DESC sort failed at " + val); + prev = val; + } + } +}