address comments

Author: matriv

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/CompositeAggsRowSet.java

@@ -49,7 +49,7 @@ class CompositeAggsRowSet extends ResultRowSet<BucketExtractor> {
 
             // If the limit is -1 then we have a local sorting (sort on aggregate function) that requires all the buckets
             // to be processed so we stop only when all data is exhausted.
-            int remainingLimit = limit == -1 ? limit : limit - size >= 0 ? limit - size : 0;
+            int remainingLimit = (limit == -1) ? limit : ((limit - size) >= 0 ? (limit - size) : 0);
 
             // if the computed limit is zero, or the size is zero it means either there's nothing left or the limit has been reached
             // note that a composite agg might be valid but return zero groups (since these can be filtered with HAVING/bucket selector)

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java

@@ -210,10 +210,10 @@ private void consumeRowSet(RowSet rowSet) {
             for (boolean hasRows = rrs.hasCurrentRow(); hasRows; hasRows = rrs.advanceRow()) {
                 List<Object> row = new ArrayList<>(rrs.columnCount());
                 rrs.forEachResultColumn(row::add);
-                // if the queue overflows and no limit was specified, bail out
+                // if the queue overflows and no limit was specified, throw an error
                 if (data.insertWithOverflow(new Tuple<>(row, counter.getAndIncrement())) != null && noLimit) {
                     onFailure(new SqlIllegalArgumentException(
-                        "The default limit [{}] for aggregate sorting has been reached; please specify a LIMIT", data.size()));
+                        "The default limit [{}] for aggregate sorting has been reached; please specify a LIMIT", MAXIMUM_SIZE));
                 }
             }
         }

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/QuerierTests.java

@@ -65,4 +65,53 @@ public void testAggSorting_TwoFields() {
             assertEquals(49 - (i / 2) + ((i % 2) * 50), results.get(i).get(1));
         }
     }
+
+    @SuppressWarnings("rawtypes")
+    public void testAggSorting_ThreeFieldsWithRandomization() {
+        // Initialize comparators for fields (columns)
+        int noColumns = randomIntBetween(3, 10);
+        List<Tuple<Integer, Comparator>> tuples = new ArrayList<>(noColumns);
+        boolean[] ordering = new boolean[noColumns];
+        for (int j = 0; j < noColumns; j++) {
+            boolean order = randomBoolean();
+            ordering[j] = order;
+            tuples.add(new Tuple<>(j, order ? Comparator.naturalOrder() : Comparator.reverseOrder()));
+        }
+
+        // Insert random no of documents (rows) with random 0/1 values for each field
+        int noDocs = randomIntBetween(10, 50);
+        int queueSize = randomIntBetween(4, noDocs / 2);
+        List<List<Integer>> expected = new ArrayList<>(noDocs);
+        Querier.AggSortingQueue queue = new AggSortingQueue(queueSize, tuples);
+        for (int i = 0; i < noDocs; i++) {
+            List<Integer> values = new ArrayList<>(noColumns);
+            for (int j = 0; j < noColumns; j++) {
+                values.add(randomBoolean() ? 1 : 0);
+            }
+            queue.insertWithOverflow(new Tuple<>(values, i));
+            expected.add(values);
+        }
+
+        List<List<?>> results = queue.asList();
+        assertEquals(queueSize, results.size());
+        expected.sort((o1, o2) -> {
+            for (int j = 0; j < noColumns; j++) {
+                if (ordering[j]) { // asc
+                    if (o1.get(j) < o2.get(j)) {
+                        return -1;
+                    } else if (o1.get(j) > o2.get(j)) {
+                        return 1;
+                    }
+                } else { // desc
+                    if (o1.get(j) < o2.get(j)) {
+                        return 1;
+                    } else if (o1.get(j) > o2.get(j)) {
+                        return -1;
+                    }
+                }
+            }
+            return 0;
+        });
+        assertEquals(expected.subList(0, queueSize), results);
+    }
 }