Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESQL: Limit how many bytes concat() can process #100360

Merged
merged 12 commits into from
Oct 9, 2023
5 changes: 5 additions & 0 deletions docs/changelog/100360.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 100360
summary: "ESQL: Limit how many bytes `concat()` can process"
area: ES|QL
type: bug
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -168,11 +168,24 @@ public void testSmallConcat() throws IOException {
}

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/99826")
// TODO: drop testHugeConcatRejected() once this is fixed
public void testHugeConcat() throws IOException {
initSingleDocIndex();
assertCircuitBreaks(() -> concat(10));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just replace the body of this with testHugeConcatRejected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced.


public void testHugeConcatRejected() throws IOException {
initSingleDocIndex();
ResponseException e = expectThrows(ResponseException.class, () -> concat(10));
Map<?, ?> map = XContentHelper.convertToMap(JsonXContent.jsonXContent, EntityUtils.toString(e.getResponse().getEntity()), false);
logger.info("expected request rejected {}", map);
assertMap(
map,
matchesMap().entry("status", 400)
.entry("error", matchesMap().extraOk().entry("reason", "concatenating more than [10485760] bytes is not supported"))
);
}

private Response concat(int evals) throws IOException {
StringBuilder query = new StringBuilder();
query.append("{\"query\":\"FROM single | EVAL str = TO_STRING(a)");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import org.elasticsearch.compute.operator.BreakingBytesRefBuilder;
import org.elasticsearch.compute.operator.EvalOperator;
import org.elasticsearch.compute.operator.EvalOperator.ExpressionEvaluator;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.xpack.esql.EsqlClientException;
import org.elasticsearch.xpack.esql.evaluator.mapper.EvaluatorMapper;
import org.elasticsearch.xpack.ql.expression.Expression;
import org.elasticsearch.xpack.ql.expression.Expressions;
Expand All @@ -27,13 +29,17 @@
import java.util.function.Function;
import java.util.stream.Stream;

import static org.elasticsearch.common.unit.ByteSizeUnit.MB;
import static org.elasticsearch.xpack.ql.expression.TypeResolutions.ParamOrdinal.DEFAULT;
import static org.elasticsearch.xpack.ql.expression.TypeResolutions.isString;

/**
* Join strings.
*/
public class Concat extends ScalarFunction implements EvaluatorMapper {

static final long MAX_CONCAT_LENGTH = MB.toBytes(10);

public Concat(Source source, Expression first, List<? extends Expression> rest) {
super(source, Stream.concat(Stream.of(first), rest.stream()).toList());
}
Expand Down Expand Up @@ -83,13 +89,29 @@ public ExpressionEvaluator.Factory toEvaluator(Function<Expression, ExpressionEv

@Evaluator
static BytesRef process(@Fixed(includeInToString = false) BreakingBytesRefBuilder scratch, BytesRef[] values) {
ensureIsBelowMaxLength(values);
scratch.clear();
bpintea marked this conversation as resolved.
Show resolved Hide resolved
for (int i = 0; i < values.length; i++) {
scratch.append(values[i]);
}
return scratch.bytesRefView();
}

private static void ensureIsBelowMaxLength(BytesRef[] values) {
long length = 0;
for (var v : values) {
length += v.length;
}
if (length > MAX_CONCAT_LENGTH) {
throw new EsqlClientException("concatenating more than [" + MAX_CONCAT_LENGTH + "] bytes is not supported") {
bpintea marked this conversation as resolved.
Show resolved Hide resolved
@Override
public RestStatus status() {
return RestStatus.BAD_REQUEST; // return a 400 response
}
};
}
}

@Override
public Expression replaceChildren(List<Expression> newChildren) {
return new Concat(source(), newChildren.get(0), newChildren.subList(1, newChildren.size()));
Expand Down