Skip to content

Commit 649f74d

Browse files
committed
optimize away one step for low row count
1 parent f51d65e commit 649f74d

File tree

2 files changed

+10
-12
lines changed

2 files changed

+10
-12
lines changed

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/approximate/Approximate.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -529,20 +529,20 @@ private ActionListener<Result> countListener(double sampleProbability, ActionLis
529529
return listener.delegateFailureAndWrap((countListener, countResult) -> {
530530
long rowCount = rowCount(countResult);
531531
logger.debug("countPlan result (p={}): {} rows", sampleProbability, rowCount);
532-
if (rowCount <= SAMPLE_ROW_COUNT_FOR_COUNT_ESTIMATION / 2 && sampleProbability < 1.0) {
532+
double newSampleProbability = Math.min(1.0, sampleProbability * SAMPLE_ROW_COUNT / Math.max(1, rowCount));
533+
if (newSampleProbability > SAMPLE_PROBABILITY_THRESHOLD) {
534+
// If the new sample probability is large, run the original query.
535+
runner.run(toPhysicalPlan.apply(logicalPlan), configuration, foldContext, listener);
536+
} else if (rowCount <= SAMPLE_ROW_COUNT_FOR_COUNT_ESTIMATION / 2) {
533537
// Not enough rows are sampled yet; increase the sample probability and try again.
534-
double newSampleProbability = Math.min(
535-
1.0,
536-
sampleProbability * SAMPLE_ROW_COUNT_FOR_COUNT_ESTIMATION / Math.max(1, rowCount)
537-
);
538+
newSampleProbability = Math.min(1.0, sampleProbability * SAMPLE_ROW_COUNT_FOR_COUNT_ESTIMATION / Math.max(1, rowCount));
538539
runner.run(
539540
toPhysicalPlan.apply(countPlan(newSampleProbability)),
540541
configuration,
541542
foldContext,
542543
countListener(newSampleProbability, listener)
543544
);
544545
} else {
545-
double newSampleProbability = Math.min(1.0, sampleProbability * SAMPLE_ROW_COUNT / rowCount);
546546
runner.run(toPhysicalPlan.apply(approximatePlan(newSampleProbability)), configuration, foldContext, listener);
547547
}
548548
});

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/approximate/ApproximateTests.java

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -294,15 +294,14 @@ public void testCountPlan_smallDataAfterFiltering() throws Exception {
294294
// - one pass to get the total number of rows
295295
// - three passes to get the number of filtered rows (which is small)
296296
// - one pass to execute the original query
297-
assertThat(runner.invocations, hasSize(8));
297+
assertThat(runner.invocations, hasSize(7));
298298
assertThat(runner.invocations.get(0), allOf(hasFilter("emp_no"), not(hasSample()), hasSum()));
299299
assertThat(runner.invocations.get(1), allOf(not(hasFilter("emp_no")), not(hasSample()), not(hasSum())));
300300
assertThat(runner.invocations.get(2), allOf(hasFilter("emp_no"), hasSample(1e-14), not(hasSum())));
301301
assertThat(runner.invocations.get(3), allOf(hasFilter("emp_no"), hasSample(1e-10), not(hasSum())));
302302
assertThat(runner.invocations.get(4), allOf(hasFilter("emp_no"), hasSample(1e-6), not(hasSum())));
303303
assertThat(runner.invocations.get(5), allOf(hasFilter("emp_no"), hasSample(1e-2), not(hasSum())));
304-
assertThat(runner.invocations.get(6), allOf(hasFilter("emp_no"), not(hasSample()), not(hasSum())));
305-
assertThat(runner.invocations.get(7), allOf(hasFilter("emp_no"), not(hasSample()), hasSum()));
304+
assertThat(runner.invocations.get(6), allOf(hasFilter("emp_no"), not(hasSample()), hasSum()));
306305
}
307306

308307
public void testCountPlan_smallDataBeforeFiltering() throws Exception {
@@ -390,13 +389,12 @@ public void testCountPlan_sampleProbabilityThreshold_withFilter() throws Excepti
390389
// - one pass to get the total number of rows
391390
// - two passes to get the number of filtered rows (which determines the sample probability)
392391
// - one pass to execute the original query (because the sample probability is 50%)
393-
assertThat(runner.invocations, hasSize(6));
392+
assertThat(runner.invocations, hasSize(5));
394393
assertThat(runner.invocations.get(0), allOf(not(hasSample()), hasFilter("emp_no"), hasSum()));
395394
assertThat(runner.invocations.get(1), allOf(not(hasSample()), not(hasFilter("emp_no")), not(hasSum())));
396395
assertThat(runner.invocations.get(2), allOf(hasSample(1e-8), hasFilter("emp_no"), not(hasSum())));
397396
assertThat(runner.invocations.get(3), allOf(hasSample(1e-4), hasFilter("emp_no"), not(hasSum())));
398-
assertThat(runner.invocations.get(4), allOf(hasSample(0.05), hasFilter("emp_no"), not(hasSum())));
399-
assertThat(runner.invocations.get(5), allOf(not(hasSample()), hasFilter("emp_no"), hasSum()));
397+
assertThat(runner.invocations.get(4), allOf(not(hasSample()), hasFilter("emp_no"), hasSum()));
400398
}
401399

402400
public void testApproximatePlan_createsConfidenceInterval_withoutGrouping() throws Exception {

0 commit comments

Comments
 (0)