Skip to content

ESQL: Run tests with allow_partial_results set to false #129271

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

Closed
wants to merge 2 commits into from
Closed

ESQL: Run tests with allow_partial_results set to false #129271

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3835988
Run tests with `allow_partial_results` set to `false`
bpintea Jun 11, 2025
659b1da
Review feedback
bpintea Jun 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion ...e/src/yamlRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/EsqlClientYamlIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;

import org.elasticsearch.test.rest.yaml.ClientYamlTestCandidate;
import org.elasticsearch.test.rest.yaml.section.DoSection;
import org.elasticsearch.test.rest.yaml.section.ExecutableSection;

/**
* Run the ESQL yaml tests against the synchronous API.
Expand All @@ -22,6 +24,17 @@ public EsqlClientYamlIT(final ClientYamlTestCandidate testCandidate) {

@ParametersFactory
public static Iterable<Object[]> parameters() throws Exception {
return createParameters();
return updateEsqlQueryDoSections(createParameters(), EsqlClientYamlIT::modifyEsqlQueryExecutableSection);
}

private static ExecutableSection modifyEsqlQueryExecutableSection(DoSection doSection) {
var apiCallSection = doSection.getApiCallSection();
if (apiCallSection.getApi().equals("esql.query")) {
if (apiCallSection.getParams().containsKey("allow_partial_results") == false) {
apiCallSection.addParam("allow_partial_results", "false"); // we want any error to fail the test
Copy link
Member

@nik9000 nik9000 Jun 11, 2025

Choose a reason for hiding this comment

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

Should we do this in the other esql yaml subclasses?

I'm a bit worried that this makes our tests unlike production. If we're hacking the yaml files, maybe we should add an assertion that there aren't any partial failures instead of swapping this key.

@dnhatn, you wanted some time for this setting to test, what do you think?

Copy link
Contributor Author

@bpintea bpintea Jun 12, 2025

Choose a reason for hiding this comment

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

Should we do this in the other esql yaml subclasses?

I've extended the setting to the async tests too, as they're not really fetching chunks of partial results. Thanks.

I'm a bit worried that this makes our tests unlike production. If we're hacking the yaml files, maybe we should add an assertion that there aren't any partial failures instead of swapping this key.

I've opened #129293 as an alternative to this approach. Some of the KNN tests seem to fail with both approaches, will follow up.

Copy link
Member

@dnhatn dnhatn Jun 12, 2025

Choose a reason for hiding this comment

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

Thanks for the ping. I will take a look at the new PR.

}
}
return doSection;
}

}
6 changes: 4 additions & 2 deletions ...n/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,12 +266,14 @@ protected void doTest() throws Throwable {
protected final void doTest(String query) throws Throwable {
RequestObjectBuilder builder = new RequestObjectBuilder(randomFrom(XContentType.values()));

builder.query(query);
builder.allowPartialResults(false); // we want any error to fail the test
if (query.toUpperCase(Locale.ROOT).contains("LOOKUP_\uD83D\uDC14")) {
builder.tables(tables());
}

Map<?, ?> prevTooks = supportsTook() ? tooks() : null;
Map<String, Object> answer = runEsql(builder.query(query), testCase.assertWarnings(deduplicateExactWarnings()));
Map<String, Object> answer = runEsql(builder, testCase.assertWarnings(deduplicateExactWarnings()));

var expectedColumnsWithValues = loadCsvSpecValues(testCase.expectedResults);

Expand All @@ -296,7 +298,7 @@ protected final void doTest(String query) throws Throwable {
}
}

private Map<?, ?> tooks() throws IOException {
private static Map<?, ?> tooks() throws IOException {
Request request = new Request("GET", "/_xpack/usage");
HttpEntity entity = client().performRequest(request).getEntity();
Map<?, ?> usage = XContentHelper.convertToMap(XContentType.JSON.xContent(), entity.getContent(), false);
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/63_enrich_int_range.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,5 @@ teardown:
- do:
catch: /ENRICH range and input types are incompatible. range\[INTEGER\], input\[DOUBLE\]/
esql.query:
allow_partial_results: false
body:
query: 'FROM employees | ENRICH ages-policy ON salary | STATS count=COUNT(*) BY description | SORT count DESC, description ASC'