ESQL: Limit how many bytes concat() can process
#100360
Conversation
This adds a limit on the bytes length that a concatenated string can get to. The limit is set to 10MB (per entry). When the limit is hit, an exception is returned to the caller (similar to an accounting circuit breaking).
elasticsearchmachine
added
the
Team:QL (Deprecated)
|
Pinging @elastic/es-ql (Team:QL) |
|
Hi @bpintea, I've created a changelog YAML for you. |
|
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Concat.java
Show resolved
Hide resolved
| @@ -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)); | |||
| } | |||
There was a problem hiding this comment.
Can we just replace the body of this with testHugeConcatRejected.
...sql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/string/Concat.java
Show resolved
Hide resolved
- grow cratch builder with the checked length of input arguments - replace disabled test
Lower max concat len to 1MB
Lower max concat len to 1MB in test.
Integrating this exception into the unit tests is doable, but not cleanly: either having a dedicated test ignoring the test providers (but being run against all of them), or contriving a test with warnings and type errors to avoid running most of the inherited tests. I've left it out for now, but I can add it in a follow up if needed. I've also lowered the limit to 1MB, since 10MB still generates OOMs in integration tests. Noteworthy, also integration tests VM OOMs with 10MB of test data. |
I'd bang up a dedicated unit test for it. I think it's probably worth it, but I get that it's annoying with all the providers in the way. |
Add unit testing for max len concat'ing
bpintea
added
auto-merge-without-approval
Switch to lower frequency of oversized handling.
Stop checking after oversize test
|
@elasticsearchmachine run elasticsearch-ci/part-2 |
|
@elasticsearchmachine run elasticsearch-ci/part-2 |
1 similar comment
|
@elasticsearchmachine run elasticsearch-ci/part-2 |
bpintea
removed
the
buildkite-opt-in
|
@elasticsearchmachine run elasticsearch-ci/part-2 |
|
@elasticsearchmachine run elasticsearch-ci/part-3 |
💚 Backport successful
|
This adds a limit on the bytes length that a concatenated string can get to. The limit is set to 1MB (per entry). When the limit is hit, an exception is returned to the caller (similar to an accounting circuit breaking) and execution halted. Related: elastic#100288.
This adds a limit on the bytes length that a concatenated string can get to. The limit is set to 1MB (per entry). When the limit is hit, an exception is returned to the caller (similar to an accounting circuit breaking) and execution halted. Related: #100288.
This adds a limit on the bytes length that a concatenated string can get to.
The limit is set to 1MB (per entry).
When the limit is hit, an exception is returned to the caller (similar to an accounting circuit breaking) and execution halted.
Related: #100288.