Skip to content

Commit

Permalink
ESQL: Lower the implicit limit, if none is user-provided (elastic#99816)
Browse files Browse the repository at this point in the history
This lowers the implicit (max) 10K limit to 500 (default). The new
default limit is only added if no other limit is detected after the last
pipeline breaker. Otherwise the capping max limit is applied.
  • Loading branch information
bpintea authored Sep 25, 2023
1 parent a1caba1 commit dfec836
Show file tree
Hide file tree
Showing 13 changed files with 122 additions and 57 deletions.
6 changes: 6 additions & 0 deletions docs/changelog/99816.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 99816
summary: "ESQL: Lower the implicit limit, if none is user-provided"
area: ES|QL
type: enhancement
issues:
- 99458
Original file line number Diff line number Diff line change
Expand Up @@ -81,17 +81,22 @@ public byte[] max(String field, DataType dataType) {

public static final TestSearchStats TEST_SEARCH_STATS = new TestSearchStats();

public static final EsqlConfiguration TEST_CFG = new EsqlConfiguration(
DateUtils.UTC,
Locale.US,
null,
null,
new QueryPragmas(Settings.EMPTY),
EsqlPlugin.QUERY_RESULT_TRUNCATION_MAX_SIZE.getDefault(Settings.EMPTY)
);
public static final EsqlConfiguration TEST_CFG = configuration(new QueryPragmas(Settings.EMPTY));

private EsqlTestUtils() {}

public static EsqlConfiguration configuration(QueryPragmas pragmas) {
return new EsqlConfiguration(
DateUtils.UTC,
Locale.US,
null,
null,
pragmas,
EsqlPlugin.QUERY_RESULT_TRUNCATION_MAX_SIZE.getDefault(Settings.EMPTY),
EsqlPlugin.QUERY_RESULT_TRUNCATION_DEFAULT_SIZE.getDefault(Settings.EMPTY)
);
}

public static Literal L(Object value) {
return of(value);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -613,11 +613,11 @@ private static LogicalPlan removeAggDuplicates(Aggregate agg) {
private static class AddImplicitLimit extends ParameterizedRule<LogicalPlan, LogicalPlan, AnalyzerContext> {
@Override
public LogicalPlan apply(LogicalPlan logicalPlan, AnalyzerContext context) {
return new Limit(
Source.EMPTY,
new Literal(Source.EMPTY, context.configuration().resultTruncationMaxSize(), DataTypes.INTEGER),
logicalPlan
);
List<LogicalPlan> limits = logicalPlan.collectFirstChildren(Limit.class::isInstance);
var limit = limits.isEmpty() == false
? context.configuration().resultTruncationMaxSize() // user provided a limit: cap result entries to the max
: context.configuration().resultTruncationDefaultSize(); // user provided no limit: cap to a default
return new Limit(Source.EMPTY, new Literal(Source.EMPTY, limit, DataTypes.INTEGER), logicalPlan);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,14 @@ public class EsqlPlugin extends Plugin implements ActionPlugin {
Setting.Property.NodeScope
);

public static final Setting<Integer> QUERY_RESULT_TRUNCATION_DEFAULT_SIZE = Setting.intSetting(
"esql.query.result_truncation_max_size",
500,
1,
10000,
Setting.Property.NodeScope
);

@Override
public Collection<Object> createComponents(
Client client,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ private void doExecuteForked(Task task, EsqlQueryRequest request, ActionListener
null,
clusterService.getClusterName().value(),
request.pragmas(),
EsqlPlugin.QUERY_RESULT_TRUNCATION_MAX_SIZE.get(settings)
EsqlPlugin.QUERY_RESULT_TRUNCATION_MAX_SIZE.get(settings),
EsqlPlugin.QUERY_RESULT_TRUNCATION_DEFAULT_SIZE.get(settings)
);
String sessionId = sessionID(task);
planExecutor.esql(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ public class EsqlConfiguration extends Configuration implements Writeable {
private final QueryPragmas pragmas;

private final int resultTruncationMaxSize;
private final int resultTruncationDefaultSize;

private final Locale locale;

Expand All @@ -32,19 +33,22 @@ public EsqlConfiguration(
String username,
String clusterName,
QueryPragmas pragmas,
int resultTruncationMaxSize
int resultTruncationMaxSize,
int resultTruncationDefaultSize
) {
super(zi, username, clusterName);
this.locale = locale;
this.pragmas = pragmas;
this.resultTruncationMaxSize = resultTruncationMaxSize;
this.resultTruncationDefaultSize = resultTruncationDefaultSize;
}

public EsqlConfiguration(StreamInput in) throws IOException {
super(in.readZoneId(), Instant.ofEpochSecond(in.readVLong(), in.readVInt()), in.readOptionalString(), in.readOptionalString());
locale = Locale.forLanguageTag(in.readString());
this.pragmas = new QueryPragmas(in);
this.resultTruncationMaxSize = in.readVInt();
this.resultTruncationDefaultSize = in.readVInt();
}

@Override
Expand All @@ -58,6 +62,7 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeString(locale.toLanguageTag());
pragmas.writeTo(out);
out.writeVInt(resultTruncationMaxSize);
out.writeVInt(resultTruncationDefaultSize);
}

public QueryPragmas pragmas() {
Expand All @@ -68,6 +73,10 @@ public int resultTruncationMaxSize() {
return resultTruncationMaxSize;
}

public int resultTruncationDefaultSize() {
return resultTruncationDefaultSize;
}

public Locale locale() {
return locale;
}
Expand All @@ -77,6 +86,7 @@ public boolean equals(Object o) {
if (super.equals(o)) {
EsqlConfiguration that = (EsqlConfiguration) o;
return resultTruncationMaxSize == that.resultTruncationMaxSize
&& resultTruncationDefaultSize == that.resultTruncationDefaultSize
&& Objects.equals(pragmas, that.pragmas)
&& Objects.equals(locale, that.locale);
}
Expand All @@ -85,6 +95,6 @@ public boolean equals(Object o) {

@Override
public int hashCode() {
return Objects.hash(super.hashCode(), pragmas, resultTruncationMaxSize, locale);
return Objects.hash(super.hashCode(), pragmas, resultTruncationMaxSize, resultTruncationDefaultSize, locale);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@
import org.elasticsearch.xpack.esql.planner.Mapper;
import org.elasticsearch.xpack.esql.planner.PlannerUtils;
import org.elasticsearch.xpack.esql.planner.TestPhysicalOperationProviders;
import org.elasticsearch.xpack.esql.plugin.EsqlPlugin;
import org.elasticsearch.xpack.esql.plugin.QueryPragmas;
import org.elasticsearch.xpack.esql.session.EsqlConfiguration;
import org.elasticsearch.xpack.esql.stats.DisabledSearchStats;
Expand All @@ -87,12 +86,10 @@

import java.io.IOException;
import java.net.URL;
import java.time.ZoneOffset;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
Expand Down Expand Up @@ -151,13 +148,8 @@ public class CsvTests extends ESTestCase {
private final Integer lineNumber;
private final CsvSpecReader.CsvTestCase testCase;

private final EsqlConfiguration configuration = new EsqlConfiguration(
ZoneOffset.UTC,
Locale.US,
null,
null,
new QueryPragmas(Settings.builder().put("page_size", randomPageSize()).build()),
EsqlPlugin.QUERY_RESULT_TRUNCATION_MAX_SIZE.getDefault(Settings.EMPTY)
private final EsqlConfiguration configuration = EsqlTestUtils.configuration(
new QueryPragmas(Settings.builder().put("page_size", randomPageSize()).build())
);
private final FunctionRegistry functionRegistry = new EsqlFunctionRegistry();
private final EsqlParser parser = new EsqlParser();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@

package org.elasticsearch.xpack.esql.analysis;

import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.esql.expression.function.aggregate.Max;
import org.elasticsearch.xpack.esql.plan.logical.EsqlUnresolvedRelation;
import org.elasticsearch.xpack.esql.plan.logical.Eval;
import org.elasticsearch.xpack.esql.plan.logical.Row;
import org.elasticsearch.xpack.esql.plugin.EsqlPlugin;
import org.elasticsearch.xpack.ql.expression.Alias;
import org.elasticsearch.xpack.ql.expression.Attribute;
import org.elasticsearch.xpack.ql.expression.Expressions;
Expand All @@ -25,6 +27,7 @@
import org.elasticsearch.xpack.ql.plan.TableIdentifier;
import org.elasticsearch.xpack.ql.plan.logical.Aggregate;
import org.elasticsearch.xpack.ql.plan.logical.EsRelation;
import org.elasticsearch.xpack.ql.plan.logical.Filter;
import org.elasticsearch.xpack.ql.plan.logical.Limit;
import org.elasticsearch.xpack.ql.plan.logical.OrderBy;
import org.elasticsearch.xpack.ql.type.DataType;
Expand All @@ -42,6 +45,7 @@
import static org.elasticsearch.xpack.ql.tree.Source.EMPTY;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.instanceOf;

Expand All @@ -54,6 +58,9 @@ public class AnalyzerTests extends ESTestCase {
List.of()
);

private static final int MAX_LIMIT = EsqlPlugin.QUERY_RESULT_TRUNCATION_MAX_SIZE.getDefault(Settings.EMPTY);
private static final int DEFAULT_LIMIT = EsqlPlugin.QUERY_RESULT_TRUNCATION_DEFAULT_SIZE.getDefault(Settings.EMPTY);

public void testIndexResolution() {
EsIndex idx = new EsIndex("idx", Map.of());
Analyzer analyzer = analyzer(IndexResolution.valid(idx));
Expand Down Expand Up @@ -821,14 +828,60 @@ public void testProjectAggGroupsRefs() {
""", "d", "last_name");
}

public void testExplicitProjectAndLimit() {
public void testImplicitLimit() {
var plan = analyze("""
from test
""");
var limit = as(plan, Limit.class);
assertThat(limit.limit().fold(), equalTo(DEFAULT_LIMIT));
as(limit.child(), EsRelation.class);
}

public void testImplicitMaxLimitAfterLimit() {
for (int i = -1; i <= 1; i++) {
var plan = analyze("from test | limit " + (MAX_LIMIT + i));
var limit = as(plan, Limit.class);
assertThat(limit.limit().fold(), equalTo(MAX_LIMIT));
limit = as(limit.child(), Limit.class);
as(limit.child(), EsRelation.class);
}
}

/*
Limit[10000[INTEGER]]
\_Filter[s{r}#3 > 0[INTEGER]]
\_Eval[[salary{f}#10 * 10[INTEGER] AS s]]
\_Limit[10000[INTEGER]]
\_EsRelation[test][_meta_field{f}#11, emp_no{f}#5, first_name{f}#6, ge..]
*/
public void testImplicitMaxLimitAfterLimitAndNonLimit() {
for (int i = -1; i <= 1; i++) {
var plan = analyze("from test | limit " + (MAX_LIMIT + i) + " | eval s = salary * 10 | where s > 0");
var limit = as(plan, Limit.class);
assertThat(limit.limit().fold(), equalTo(MAX_LIMIT));
var filter = as(limit.child(), Filter.class);
var eval = as(filter.child(), Eval.class);
limit = as(eval.child(), Limit.class);
as(limit.child(), EsRelation.class);
}
}

public void testImplicitDefaultLimitAfterLimitAndBreaker() {
for (var breaker : List.of("stats c = count(salary) by last_name", "sort salary")) {
var plan = analyze("from test | limit 100000 | " + breaker);
var limit = as(plan, Limit.class);
assertThat(limit.limit().fold(), equalTo(MAX_LIMIT));
}
}

public void testImplicitDefaultLimitAfterBreakerAndNonBreakers() {
for (var breaker : List.of("stats c = count(salary) by last_name", "eval c = salary | sort c")) {
var plan = analyze("from test | " + breaker + " | eval cc = c * 10 | where cc > 0");
var limit = as(plan, Limit.class);
assertThat(limit.limit().fold(), equalTo(DEFAULT_LIMIT));
}
}

private static final String[] COMPARISONS = new String[] { "==", "!=", "<", "<=", ">", ">=" };

public void testCompareIntToString() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
import org.elasticsearch.xpack.esql.planner.Mapper;
import org.elasticsearch.xpack.esql.planner.PhysicalVerificationException;
import org.elasticsearch.xpack.esql.planner.PlannerUtils;
import org.elasticsearch.xpack.esql.plugin.EsqlPlugin;
import org.elasticsearch.xpack.esql.plugin.QueryPragmas;
import org.elasticsearch.xpack.esql.querydsl.query.SingleValueQuery;
import org.elasticsearch.xpack.esql.session.EsqlConfiguration;
Expand All @@ -76,20 +75,19 @@
import org.elasticsearch.xpack.ql.index.EsIndex;
import org.elasticsearch.xpack.ql.index.IndexResolution;
import org.elasticsearch.xpack.ql.type.DataTypes;
import org.elasticsearch.xpack.ql.type.DateUtils;
import org.elasticsearch.xpack.ql.type.EsField;
import org.junit.Before;

import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

import static java.util.Arrays.asList;
import static org.elasticsearch.core.Tuple.tuple;
import static org.elasticsearch.xpack.esql.EsqlTestUtils.as;
import static org.elasticsearch.xpack.esql.EsqlTestUtils.configuration;
import static org.elasticsearch.xpack.esql.EsqlTestUtils.loadMapping;
import static org.elasticsearch.xpack.esql.EsqlTestUtils.statsForMissingField;
import static org.elasticsearch.xpack.esql.SerializationTestUtils.assertSerialization;
Expand Down Expand Up @@ -129,16 +127,7 @@ public class PhysicalPlanOptimizerTests extends ESTestCase {
public static List<Object[]> readScriptSpec() {
return settings().stream().map(t -> {
var settings = Settings.builder().loadFromMap(t.v2()).build();
return new Object[] {
t.v1(),
new EsqlConfiguration(
DateUtils.UTC,
Locale.US,
null,
null,
new QueryPragmas(settings),
EsqlPlugin.QUERY_RESULT_TRUNCATION_MAX_SIZE.getDefault(settings)
) };
return new Object[] { t.v1(), configuration(new QueryPragmas(settings)) };
}).toList();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,15 @@ public class EvalMapperTests extends ESTestCase {
private static final FieldAttribute LONG = field("long", DataTypes.LONG);
private static final FieldAttribute DATE = field("date", DataTypes.DATETIME);

private static final EsqlConfiguration TEST_CONFIG = new EsqlConfiguration(ZoneOffset.UTC, Locale.US, "test", null, null, 10000000);
private static final EsqlConfiguration TEST_CONFIG = new EsqlConfiguration(
ZoneOffset.UTC,
Locale.US,
"test",
null,
null,
10000000,
10000
);

@ParametersFactory(argumentFormatting = "%1$s")
public static List<Object[]> params() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@ private EsqlConfiguration config() {
"test_user",
"test_cluser",
pragmas,
EsqlPlugin.QUERY_RESULT_TRUNCATION_MAX_SIZE.getDefault(null)
EsqlPlugin.QUERY_RESULT_TRUNCATION_MAX_SIZE.getDefault(null),
EsqlPlugin.QUERY_RESULT_TRUNCATION_DEFAULT_SIZE.getDefault(null)
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import org.elasticsearch.xpack.esql.parser.EsqlParser;
import org.elasticsearch.xpack.esql.plan.physical.PhysicalPlan;
import org.elasticsearch.xpack.esql.planner.Mapper;
import org.elasticsearch.xpack.esql.session.EsqlConfiguration;
import org.elasticsearch.xpack.esql.session.EsqlConfigurationSerializationTests;
import org.elasticsearch.xpack.esql.stats.Metrics;
import org.elasticsearch.xpack.ql.expression.function.FunctionRegistry;
Expand All @@ -37,11 +36,10 @@
import org.elasticsearch.xpack.ql.type.EsField;

import java.io.IOException;
import java.time.ZoneOffset;
import java.util.List;
import java.util.Locale;
import java.util.Map;

import static org.elasticsearch.xpack.esql.EsqlTestUtils.TEST_CFG;
import static org.elasticsearch.xpack.esql.EsqlTestUtils.emptyPolicyResolution;
import static org.elasticsearch.xpack.esql.EsqlTestUtils.loadMapping;

Expand Down Expand Up @@ -176,15 +174,7 @@ static LogicalPlan parse(String query) {
}

static PhysicalPlan mapAndMaybeOptimize(LogicalPlan logicalPlan) {
var configuration = new EsqlConfiguration(
ZoneOffset.UTC,
Locale.US,
null,
null,
new QueryPragmas(Settings.EMPTY),
EsqlPlugin.QUERY_RESULT_TRUNCATION_MAX_SIZE.getDefault(Settings.EMPTY)
);
var physicalPlanOptimizer = new PhysicalPlanOptimizer(new PhysicalOptimizerContext(configuration));
var physicalPlanOptimizer = new PhysicalPlanOptimizer(new PhysicalOptimizerContext(TEST_CFG));
FunctionRegistry functionRegistry = new EsqlFunctionRegistry();
var mapper = new Mapper(functionRegistry);
var physical = mapper.map(logicalPlan);
Expand Down
Loading

0 comments on commit dfec836

Please sign in to comment.