Skip to content

Commit 7842154

Browse files
carlosdelestcarlosdelest
authored
Merge branch '8.x' into backport/8.x/pr-125282
2 parents 248165e + 7444595 commit 7842154

File tree

6 files changed

+91
-33
lines changed

6 files changed

+91
-33
lines changed

‎docs/changelog/124446.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 124446
2+
summary: "ESQL: Fail in `AggregateFunction` when `LogicPlan` is not an `Aggregate`"
3+
area: ES|QL
4+
type: bug
5+
issues:
6+
- 124311

‎modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3ClientSettings.java‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -122,28 +122,28 @@ final class S3ClientSettings {
122122
static final Setting.AffixSetting<TimeValue> READ_TIMEOUT_SETTING = Setting.affixKeySetting(
123123
PREFIX,
124124
"read_timeout",
125-
key -> Setting.timeSetting(key, TimeValue.timeValueMillis(ClientConfiguration.DEFAULT_SOCKET_TIMEOUT), Property.NodeScope)
125+
key -> Setting.timeSetting(key, Defaults.READ_TIMEOUT, Property.NodeScope)
126126
);
127127

128128
/** The maximum number of concurrent connections to use. */
129129
static final Setting.AffixSetting<Integer> MAX_CONNECTIONS_SETTING = Setting.affixKeySetting(
130130
PREFIX,
131131
"max_connections",
132-
key -> Setting.intSetting(key, ClientConfiguration.DEFAULT_MAX_CONNECTIONS, 1, Property.NodeScope)
132+
key -> Setting.intSetting(key, Defaults.MAX_CONNECTIONS, 1, Property.NodeScope)
133133
);
134134

135135
/** The number of retries to use when an s3 request fails. */
136136
static final Setting.AffixSetting<Integer> MAX_RETRIES_SETTING = Setting.affixKeySetting(
137137
PREFIX,
138138
"max_retries",
139-
key -> Setting.intSetting(key, ClientConfiguration.DEFAULT_RETRY_POLICY.getMaxErrorRetry(), 0, Property.NodeScope)
139+
key -> Setting.intSetting(key, Defaults.RETRY_COUNT, 0, Property.NodeScope)
140140
);
141141

142142
/** Whether retries should be throttled (ie use backoff). */
143143
static final Setting.AffixSetting<Boolean> USE_THROTTLE_RETRIES_SETTING = Setting.affixKeySetting(
144144
PREFIX,
145145
"use_throttle_retries",
146-
key -> Setting.boolSetting(key, ClientConfiguration.DEFAULT_THROTTLE_RETRIES, Property.NodeScope)
146+
key -> Setting.boolSetting(key, Defaults.THROTTLE_RETRIES, Property.NodeScope)
147147
);
148148

149149
/** Whether the s3 client should use path style access. */
@@ -336,7 +336,7 @@ S3ClientSettings refine(Settings repositorySettings) {
336336

337337
/**
338338
* Load all client settings from the given settings.
339-
*
339+
* <p>
340340
* Note this will always at least return a client named "default".
341341
*/
342342
static Map<String, S3ClientSettings> load(Settings settings) {
@@ -502,4 +502,11 @@ private static <T> T getRepoSettingOrDefault(Setting.AffixSetting<T> setting, Se
502502
}
503503
return defaultValue;
504504
}
505+
506+
static final class Defaults {
507+
static final TimeValue READ_TIMEOUT = TimeValue.timeValueMillis(ClientConfiguration.DEFAULT_SOCKET_TIMEOUT);
508+
static final int MAX_CONNECTIONS = ClientConfiguration.DEFAULT_MAX_CONNECTIONS;
509+
static final int RETRY_COUNT = ClientConfiguration.DEFAULT_RETRY_POLICY.getMaxErrorRetry();
510+
static final boolean THROTTLE_RETRIES = ClientConfiguration.DEFAULT_THROTTLE_RETRIES;
511+
}
505512
}

‎modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,8 +111,8 @@ class S3Service implements Closeable {
111111

112112
/**
113113
* Refreshes the settings for the AmazonS3 clients and clears the cache of
114-
* existing clients. New clients will be build using these new settings. Old
115-
* clients are usable until released. On release they will be destroyed instead
114+
* existing clients. New clients will be built using these new settings. Old
115+
* clients are usable until released. On release, they will be destroyed instead
116116
* of being returned to the cache.
117117
*/
118118
public synchronized void refreshAndClearCache(Map<String, S3ClientSettings> clientsSettings) {
@@ -122,7 +122,7 @@ public synchronized void refreshAndClearCache(Map<String, S3ClientSettings> clie
122122
this.staticClientSettings = Maps.ofEntries(clientsSettings.entrySet());
123123
derivedClientSettings = emptyMap();
124124
assert this.staticClientSettings.containsKey("default") : "always at least have 'default'";
125-
// clients are built lazily by {@link client}
125+
/* clients are built lazily by {@link #client} */
126126
}
127127

128128
/**
@@ -330,7 +330,8 @@ public void refresh() {
330330
* <ul>
331331
* <li>Reads the the location of the web identity token not from AWS_WEB_IDENTITY_TOKEN_FILE, but from a symlink
332332
* in the plugin directory, so we don't need to create a hardcoded read file permission for the plugin.</li>
333-
* <li>Supports customization of the STS endpoint via a system property, so we can test it against a test fixture.</li>
333+
* <li>Supports customization of the STS (Security Token Service) endpoint via a system property, so we can
334+
* test it against a test fixture.</li>
334335
* <li>Supports gracefully shutting down the provider and the STS client.</li>
335336
* </ul>
336337
*/
@@ -373,7 +374,7 @@ static class CustomWebIdentityTokenCredentialsProvider implements AWSCredentials
373374
if (roleArn == null) {
374375
LOGGER.warn(
375376
"Unable to use a web identity token for authentication. The AWS_WEB_IDENTITY_TOKEN_FILE environment "
376-
+ "variable is set, but either AWS_ROLE_ARN is missing"
377+
+ "variable is set, but AWS_ROLE_ARN is missing"
377378
);
378379
return;
379380
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/AggregateFunction.java‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@
1919
import org.elasticsearch.xpack.esql.core.tree.Source;
2020
import org.elasticsearch.xpack.esql.core.util.CollectionUtils;
2121
import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput;
22+
import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
2223
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
23-
import org.elasticsearch.xpack.esql.plan.logical.OrderBy;
2424

2525
import java.io.IOException;
2626
import java.util.List;
@@ -139,14 +139,14 @@ public boolean equals(Object obj) {
139139
@Override
140140
public BiConsumer<LogicalPlan, Failures> postAnalysisPlanVerification() {
141141
return (p, failures) -> {
142-
if (p instanceof OrderBy order) {
143-
order.order().forEach(o -> {
144-
o.forEachDown(Function.class, f -> {
145-
if (f instanceof AggregateFunction) {
146-
failures.add(fail(f, "Aggregate functions are not allowed in SORT [{}]", f.functionName()));
147-
}
148-
});
149-
});
142+
if ((p instanceof Aggregate) == false) {
143+
p.expressions().forEach(x -> x.forEachDown(AggregateFunction.class, af -> {
144+
if (af instanceof Rate) {
145+
failures.add(fail(af, "aggregate function [{}] not allowed outside METRICS command", af.sourceText()));
146+
} else {
147+
failures.add(fail(af, "aggregate function [{}] not allowed outside STATS command", af.sourceText()));
148+
}
149+
}));
150150
}
151151
};
152152
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Eval.java‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,6 @@
2424
import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
2525
import org.elasticsearch.xpack.esql.core.tree.Source;
2626
import org.elasticsearch.xpack.esql.core.type.DataType;
27-
import org.elasticsearch.xpack.esql.expression.function.aggregate.AggregateFunction;
28-
import org.elasticsearch.xpack.esql.expression.function.aggregate.Rate;
2927
import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput;
3028
import org.elasticsearch.xpack.esql.plan.GeneratingPlan;
3129

@@ -179,14 +177,6 @@ public void postAnalysisVerification(Failures failures) {
179177
)
180178
);
181179
}
182-
// check no aggregate functions are used
183-
field.forEachDown(AggregateFunction.class, af -> {
184-
if (af instanceof Rate) {
185-
failures.add(fail(af, "aggregate function [{}] not allowed outside METRICS command", af.sourceText()));
186-
} else {
187-
failures.add(fail(af, "aggregate function [{}] not allowed outside STATS command", af.sourceText()));
188-
}
189-
});
190180
});
191181
}
192182
}

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java‎

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -978,6 +978,17 @@ public void testNotFoundFieldInNestedFunction() {
978978
line 1:23: Unknown column [avg]""", error("from test | stats c = avg by missing + 1, not_found"));
979979
}
980980

981+
public void testMultipleAggsOutsideStats() {
982+
assertEquals(
983+
"""
984+
1:71: aggregate function [avg(salary)] not allowed outside STATS command
985+
line 1:96: aggregate function [median(emp_no)] not allowed outside STATS command
986+
line 1:22: aggregate function [sum(salary)] not allowed outside STATS command
987+
line 1:39: aggregate function [avg(languages)] not allowed outside STATS command""",
988+
error("from test | eval s = sum(salary), l = avg(languages) | where salary > avg(salary) and emp_no > median(emp_no)")
989+
);
990+
}
991+
981992
public void testSpatialSort() {
982993
String prefix = "ROW wkt = [\"POINT(42.9711 -14.7553)\", \"POINT(75.8093 22.7277)\"] | MV_EXPAND wkt ";
983994
assertEquals("1:130: cannot sort on geo_point", error(prefix + "| EVAL shape = TO_GEOPOINT(wkt) | limit 5 | sort shape"));
@@ -2024,10 +2035,53 @@ public void testCategorizeWithFilteredAggregations() {
20242035
}
20252036

20262037
public void testSortByAggregate() {
2027-
assertEquals("1:18: Aggregate functions are not allowed in SORT [COUNT]", error("ROW a = 1 | SORT count(*)"));
2028-
assertEquals("1:28: Aggregate functions are not allowed in SORT [COUNT]", error("ROW a = 1 | SORT to_string(count(*))"));
2029-
assertEquals("1:22: Aggregate functions are not allowed in SORT [MAX]", error("ROW a = 1 | SORT 1 + max(a)"));
2030-
assertEquals("1:18: Aggregate functions are not allowed in SORT [COUNT]", error("FROM test | SORT count(*)"));
2038+
assertEquals("1:18: aggregate function [count(*)] not allowed outside STATS command", error("ROW a = 1 | SORT count(*)"));
2039+
assertEquals(
2040+
"1:28: aggregate function [count(*)] not allowed outside STATS command",
2041+
error("ROW a = 1 | SORT to_string(count(*))")
2042+
);
2043+
assertEquals("1:22: aggregate function [max(a)] not allowed outside STATS command", error("ROW a = 1 | SORT 1 + max(a)"));
2044+
assertEquals("1:18: aggregate function [count(*)] not allowed outside STATS command", error("FROM test | SORT count(*)"));
2045+
}
2046+
2047+
public void testFilterByAggregate() {
2048+
assertEquals("1:19: aggregate function [count(*)] not allowed outside STATS command", error("ROW a = 1 | WHERE count(*) > 0"));
2049+
assertEquals(
2050+
"1:29: aggregate function [count(*)] not allowed outside STATS command",
2051+
error("ROW a = 1 | WHERE to_string(count(*)) IS NOT NULL")
2052+
);
2053+
assertEquals("1:23: aggregate function [max(a)] not allowed outside STATS command", error("ROW a = 1 | WHERE 1 + max(a) > 0"));
2054+
assertEquals(
2055+
"1:24: aggregate function [min(languages)] not allowed outside STATS command",
2056+
error("FROM employees | WHERE min(languages) > 2")
2057+
);
2058+
}
2059+
2060+
public void testDissectByAggregate() {
2061+
assertEquals(
2062+
"1:21: aggregate function [min(first_name)] not allowed outside STATS command",
2063+
error("from test | dissect min(first_name) \"%{foo}\"")
2064+
);
2065+
assertEquals(
2066+
"1:21: aggregate function [avg(salary)] not allowed outside STATS command",
2067+
error("from test | dissect avg(salary) \"%{foo}\"")
2068+
);
2069+
}
2070+
2071+
public void testGrokByAggregate() {
2072+
assertEquals(
2073+
"1:18: aggregate function [max(last_name)] not allowed outside STATS command",
2074+
error("from test | grok max(last_name) \"%{WORD:foo}\"")
2075+
);
2076+
assertEquals(
2077+
"1:18: aggregate function [sum(salary)] not allowed outside STATS command",
2078+
error("from test | grok sum(salary) \"%{WORD:foo}\"")
2079+
);
2080+
}
2081+
2082+
public void testAggregateInRow() {
2083+
assertEquals("1:13: aggregate function [count(*)] not allowed outside STATS command", error("ROW a = 1 + count(*)"));
2084+
assertEquals("1:9: aggregate function [avg(2)] not allowed outside STATS command", error("ROW a = avg(2)"));
20312085
}
20322086

20332087
public void testLookupJoinDataTypeMismatch() {

0 commit comments

Comments
 (0)