Skip to content

Commit

Permalink
Fix bug in TopKSelector call to Arrays.sort.
Browse files Browse the repository at this point in the history
Fixes #5691
Fixes #5692

RELNOTES=n/a
PiperOrigin-RevId: 393150511
  • Loading branch information
Liulietong authored and Google Java Core Libraries committed Aug 26, 2021
1 parent d2c202a commit f79d923
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,16 @@
package com.google.common.collect;

import static com.google.common.truth.Truth.assertThat;
import static java.util.Collections.sort;

import com.google.common.math.IntMath;
import com.google.common.primitives.Ints;
import java.math.RoundingMode;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.List;
import java.util.Random;
import junit.framework.TestCase;

/**
Expand Down Expand Up @@ -119,4 +122,36 @@ public int compare(Integer o1, Integer o2) {
assertThat(top.topK()).containsExactlyElementsIn(Collections.nCopies(k, 0));
assertThat(compareCalls[0]).isAtMost(10L * n * IntMath.log2(k, RoundingMode.CEILING));
}

public void testExceedMaxIteration() {
/*
* Bug #5692 occurred when TopKSelector called Arrays.sort incorrectly. Test data that would
* trigger a problematic call to Arrays.sort is hard to construct by hand, so we searched for
* one among randomly generated inputs. To reach the Arrays.sort call, we need to pass an input
* that requires many iterations of partitioning inside trim(). So, to construct our random
* inputs, we concatenated 10 sorted lists together.
*/

int k = 10000;
Random random = new Random(1629833645599L);

// target list to be sorted using TopKSelector
List<Integer> target = new ArrayList<>();
for (int i = 0; i < 9; i++) {
List<Integer> sortedArray = new ArrayList();
for (int j = 0; j < 10000; j++) {
sortedArray.add(random.nextInt());
}
sort(sortedArray, Ordering.natural());
target.addAll(sortedArray);
}

TopKSelector<Integer> top = TopKSelector.least(k, Ordering.natural());
for (int value : target) {
top.offer(value);
}

sort(target, Ordering.natural());
assertEquals(top.topK(), target.subList(0, k));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ private void trim() {
iterations++;
if (iterations >= maxIterations) {
// We've already taken O(k log k), let's make sure we don't take longer than O(k log k).
Arrays.sort(buffer, left, right, comparator);
Arrays.sort(buffer, left, right + 1, comparator);
break;
}
}
Expand Down
35 changes: 35 additions & 0 deletions guava-tests/test/com/google/common/collect/TopKSelectorTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,16 @@
package com.google.common.collect;

import static com.google.common.truth.Truth.assertThat;
import static java.util.Collections.sort;

import com.google.common.math.IntMath;
import com.google.common.primitives.Ints;
import java.math.RoundingMode;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.List;
import java.util.Random;
import junit.framework.TestCase;

/**
Expand Down Expand Up @@ -119,4 +122,36 @@ public int compare(Integer o1, Integer o2) {
assertThat(top.topK()).containsExactlyElementsIn(Collections.nCopies(k, 0));
assertThat(compareCalls[0]).isAtMost(10L * n * IntMath.log2(k, RoundingMode.CEILING));
}

public void testExceedMaxIteration() {
/*
* Bug #5692 occurred when TopKSelector called Arrays.sort incorrectly. Test data that would
* trigger a problematic call to Arrays.sort is hard to construct by hand, so we searched for
* one among randomly generated inputs. To reach the Arrays.sort call, we need to pass an input
* that requires many iterations of partitioning inside trim(). So, to construct our random
* inputs, we concatenated 10 sorted lists together.
*/

int k = 10000;
Random random = new Random(1629833645599L);

// target list to be sorted using TopKSelector
List<Integer> target = new ArrayList<>();
for (int i = 0; i < 9; i++) {
List<Integer> sortedArray = new ArrayList();
for (int j = 0; j < 10000; j++) {
sortedArray.add(random.nextInt());
}
sort(sortedArray, Ordering.natural());
target.addAll(sortedArray);
}

TopKSelector<Integer> top = TopKSelector.least(k, Ordering.natural());
for (int value : target) {
top.offer(value);
}

sort(target, Ordering.natural());
assertEquals(top.topK(), target.subList(0, k));
}
}
2 changes: 1 addition & 1 deletion guava/src/com/google/common/collect/TopKSelector.java
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ private void trim() {
iterations++;
if (iterations >= maxIterations) {
// We've already taken O(k log k), let's make sure we don't take longer than O(k log k).
Arrays.sort(buffer, left, right, comparator);
Arrays.sort(buffer, left, right + 1, comparator);
break;
}
}
Expand Down

0 comments on commit f79d923

Please sign in to comment.