Skip to content

ESQL: Added exception catching to aggregators#111829

Merged
ivancea merged 3 commits intoelastic:mainfrom
ivancea:aggregators-warn-exceptions
Aug 21, 2024
Merged

ESQL: Added exception catching to aggregators#111829
ivancea merged 3 commits intoelastic:mainfrom
ivancea:aggregators-warn-exceptions

Conversation

@ivancea
Copy link
Copy Markdown
Contributor

@ivancea ivancea commented Aug 13, 2024

Add the warnExceptions capability to aggregators. Same as for evaluators. This requires:

  • Having a "Warnings-like" object in the compute module. I created a new class for that, temporarily
  • Added new "FaillibleState" objects, that hold a "failed" boolean/bitset to mark failed groups
  • Catching exceptions in all combine methods receiving a single group (Not catching in evaluateFinal/Intermediate, nor in combines that receive the full state)
  • Shortcircuiting operations if the group is in "failed" state
  • Having a third intermediate state: values, seen, failed

Extracted from #111639. Check it to see a use case

@ivancea ivancea added >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL labels Aug 13, 2024
@ivancea ivancea requested a review from nik9000 August 13, 2024 09:59
@ivancea ivancea requested a review from a team as a code owner August 13, 2024 09:59
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@nik9000 nik9000 self-assigned this Aug 19, 2024
@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Aug 19, 2024

I'll have a look at this one tomorrow. It's going to take more brainpower than I have this afternoon.

Copy link
Copy Markdown
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

LGTM.

Important bit for those following along: this isn't plugged in to any agg yet. We will fix sum to be fallible so overflows become null with a warning. But not yet. It's easy to fix the sum to do that now that we have this PR. BUT doing it in the backward compatible way is not simple. Thta's the magic.

import org.elasticsearch.common.util.BitArray;
import org.elasticsearch.core.Releasables;

public class AbstractFallibleArrayState extends AbstractArrayState {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wants javadoc I think.

@ivancea ivancea merged commit 2681bb8 into elastic:main Aug 21, 2024
@ivancea ivancea deleted the aggregators-warn-exceptions branch August 21, 2024 08:59
cbuescher pushed a commit to cbuescher/elasticsearch that referenced this pull request Sep 4, 2024
Add the `warnExceptions` capability to aggregators. Same as for evaluators. This requires:
- Having a "Warnings-like" object in the compute module. I created a new class for that, temporarily
- Added new "FaillibleState" objects, that hold a "failed" boolean/bitset to mark failed groups
- Catching exceptions in all combine methods receiving a single group (Not catching in evaluateFinal/Intermediate, nor in combines that receive the full state)
- Shortcircuiting operations if the group is in "failed" state
- Having a third intermediate state: `values, seen, failed`

Extracted from elastic#111639. Check it to see a use case
davidkyle pushed a commit to davidkyle/elasticsearch that referenced this pull request Sep 5, 2024
Add the `warnExceptions` capability to aggregators. Same as for evaluators. This requires:
- Having a "Warnings-like" object in the compute module. I created a new class for that, temporarily
- Added new "FaillibleState" objects, that hold a "failed" boolean/bitset to mark failed groups
- Catching exceptions in all combine methods receiving a single group (Not catching in evaluateFinal/Intermediate, nor in combines that receive the full state)
- Shortcircuiting operations if the group is in "failed" state
- Having a third intermediate state: `values, seen, failed`

Extracted from elastic#111639. Check it to see a use case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.16.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants