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.

Sign up for GitHub

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

Jump to bottom

Allow partial results by default in ES|QL #125060

Merged
merged 11 commits into from
Apr 3, 2025
Merged

Allow partial results by default in ES|QL #125060

merged 11 commits into from
Apr 3, 2025

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Mar 17, 2025 •
edited
Loading

With this change, ES|QL will return partial results instead of failing the entire query when encountering errors. Callers should check the partial_results flag in the response to determine if the result is partial or complete. If returning partial results is not desired, this option can be overridden per request via the allow_partial_results parameter in the query URL or globally via the cluster setting esql.allow_partial_results.

Relates #122802

@dnhatn dnhatn force-pushed the enable-partial-results branch 4 times, most recently from cfd1592 to d42e976 Compare March 19, 2025 06:06
@dnhatn dnhatn force-pushed the enable-partial-results branch 4 times, most recently from f21845b to c3c8e3e Compare April 2, 2025 19:19
@elasticsearchmachine elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Apr 2, 2025
@dnhatn dnhatn force-pushed the enable-partial-results branch from c3c8e3e to 6fe39e8 Compare April 2, 2025 23:19
@elasticsearchmachine
Copy link
Collaborator

Hi @dnhatn, I've created a changelog YAML for you. Note that since this PR is labelled >breaking, you need to update the changelog YAML to fill out the extended information sections.

@dnhatn dnhatn added v8.19.0 auto-backport Automatically create backport pull requests when merged labels Apr 3, 2025
@dnhatn dnhatn requested review from smalyshev and quux00 April 3, 2025 06:38
@dnhatn dnhatn marked this pull request as ready for review April 3, 2025 06:38
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 3, 2025
@dnhatn dnhatn mentioned this pull request Apr 3, 2025
Open
9 tasks
@smalyshev
Copy link
Contributor

smalyshev commented Apr 3, 2025 •
edited
Loading

I saw in #122802 there is one thing before enabling the default:

  • Return fatal/non-fatal errors in data node responses to enable retries (or one shard at the time?)

Is this done or should it be done after this?

smalyshev
smalyshev reviewed Apr 3, 2025
View reviewed changes
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/63_enrich_int_range.yml Outdated
- method: POST
path: /_query
parameters: [ ]
capabilities: [ esql_allow_partial_results ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand - if we have disabled compat test with this in gradle, why do we need this check?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are two different things. The rest-compat tests are from the source code of a previous branch. We need to keep this skip to avoid running this test from the current branch with clusters that don't support partial results.

@dnhatn
Copy link
Member Author

dnhatn commented Apr 3, 2025

@smalyshev All good points. I think I have addressed them. Can you please take another look? Thanks!

@dnhatn dnhatn requested a review from smalyshev April 3, 2025 16:40
@dnhatn
Copy link
Member Author

dnhatn commented Apr 3, 2025

Return fatal/non-fatal errors in data node responses to enable retries (or one shard at the time?)

We can do this later. I prefer to have this in first so that the other teams can start working on the integration.

smalyshev
smalyshev reviewed Apr 3, 2025
View reviewed changes
docs/changelog/125060.yaml Outdated
summary: Allow partial results by default in ES|QL
area: ES|QL
type: breaking
issues: []
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want maybe put #122802 here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, 5ffc0d7

@dnhatn
Copy link
Member Author

dnhatn commented Apr 3, 2025

@smalyshev Thanks for the review.

@dnhatn dnhatn merged commit 81555cc into elastic:main Apr 3, 2025
17 checks passed
@dnhatn dnhatn deleted the enable-partial-results branch April 3, 2025 19:30
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
❌ 8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 125060

dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Apr 4, 2025
@dnhatn
Allow partial results by default in ES|QL (elastic#125060)
04b3b12 
With this change, ES|QL will return partial results instead of failing
the entire query when encountering errors. Callers should check the
partial_results flag in the response to determine if the result is
partial or complete. If returning partial results is not desired, this
option can be overridden per request via the allow_partial_results
parameter in the query URL or globally via the cluster setting
esql.allow_partial_results.

Relates elastic#122802
@dnhatn dnhatn mentioned this pull request Apr 4, 2025
Draft
@alex-spies alex-spies mentioned this pull request Apr 4, 2025
Closed
alex-spies added a commit to alex-spies/elasticsearch that referenced this pull request Apr 4, 2025
@alex-spies
Revert "Allow partial results by default in ES|QL (elastic#125060)"
ec4b8d8 
This reverts commit 81555cc.
@alex-spies alex-spies mentioned this pull request Apr 4, 2025
Merged
elasticsearchmachine pushed a commit that referenced this pull request Apr 4, 2025
@alex-spies
ESQL: Revert "Allow partial results by default in ES|QL (#125060)" (#…
8f38b13 
…126286)

This reverts commit 81555cc from
#125060.

Fix #126275

@idegtiarenko and I investigated and believe this needs reverting:
silently dropping results from the query response in case any index is
missing can lead to real problems if users don't spot their mistake. I'm
also not sure if all the results will get dropped, or only from some
nodes/shards/clusters, meaning that this might be hard to spot by users
if only some results get dropped.

The main PR has no transport version bump, no new ESQL capability, and
was merged 15h ago - so it should be safe to just revert it. I noticed
there was a linked Serverless PR on the original PR, but it merely
disabled some obsolete tests on Serverless and doesn't require reverting
itself.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smalyshev smalyshev smalyshev approved these changes

@quux00 quux00 Awaiting requested review from quux00

Assignees
No one assigned
Labels
:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged backport pending >breaking serverless-linked Added by automation, don't add manually Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.19.0 v9.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dnhatn @elasticsearchmachine @smalyshev