[ESQL] Catch-and-rethrow TooComplexToDeterminizeException #137024
Conversation
elasticsearchmachine
added
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @MattAlp, I've created a changelog YAML for you. |
| @@ -1354,8 +1333,15 @@ public void testLikeRLike() { | |||
| ); | |||
| } | |||
|
|
|||
| public void testIdentifierPatternTooComplex() { | |||
| // It is incredibly unlikely that we will see this limit hit in practice | |||
There was a problem hiding this comment.
I question if we really want to test for this, but it doesn't hurt to leave it in.
MattAlp
added
the
auto-backport
not-napoleon
left a comment
not-napoleon
left a comment
There was a problem hiding this comment.
This looks good to me. As discussed on slack, we should backport this as it is a bug fix. Thanks for fielding it!
|
If anyone has thoughts on the error string passed back to the user, please weigh in. The current one is good enough, but not great. |
nik9000
left a comment
nik9000
left a comment
There was a problem hiding this comment.
My general view using exceptions for error reporting is "they are convenient but the spooky action at a distance nature makes me want integration tests."
add a single yaml test that asserts that the error is 400 and has a decent message?
bpintea
left a comment
bpintea
left a comment
There was a problem hiding this comment.
LGTM.
Might be worth adding a method to do this exception conversion (that maybe just gets a Runnable?), so that we have it "centralised", though not sure if it'll make things clearer.
|
@bpintea I had considered a functional interface / runnable handler for this, but after writing one I felt that this would make more sense until we have a reason to have centralized logic. |
💔 Backport failedThe backport operation could not be completed due to the following error: You can use sqren/backport to manually backport by running |
…ic#137024) --------- Co-authored-by: elasticsearchmachine <[email protected]>
MattAlp
changed the title
…ic#137024) --------- Co-authored-by: elasticsearchmachine <[email protected]>
5 participants
Addresses #133676 in ESQL-specific locations; patches wildcard parsing of field names and regex predicates (
AbstractStringPatternimplementations). This is more ESQL-specific than wrapping all 30-something instances ofOperations.determinize()across this repo intry/catchblocks.It's also worth noting that the classes affected by 938d429 were originally pulled in from
org.elasticsearch.xpack.ql.expression.predicate.regexand we may want to apply similar changes there as part of this PR.