Skip to content

Conversation

ivancea
Copy link
Contributor

@ivancea ivancea commented Jul 17, 2025

Log the ESQL request profile when a test fails.

@ivancea ivancea requested a review from nik9000 July 17, 2025 17:43
@ivancea ivancea added >test Issues or PRs that are addressing/adding tests Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL v9.2.0 labels Jul 17, 2025
@ivancea ivancea marked this pull request as ready for review July 18, 2025 11:13
@ivancea ivancea requested a review from nik9000 July 18, 2025 11:13
@elasticsearchmachine
Copy link
Collaborator

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

@@ -302,14 +304,6 @@ protected final void doTest(String query) throws Throwable {
}
}

static Map<String, Object> assertNotPartial(Map<String, Object> answer) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this to the other class, with runEsql and assertWarnings

}

public static Map<String, Object> runEsql(RequestObjectBuilder requestObject, AssertWarnings assertWarnings, Mode mode)
throws IOException {
return runEsql(requestObject, assertWarnings, mode, true);
}

public static Map<String, Object> runEsql(RequestObjectBuilder requestObject, AssertWarnings assertWarnings,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept here the overloads without the ProfileLogger param, as they will be easier to track, instead of looking for a null as a param

@ivancea ivancea force-pushed the show-profile-on-test-error branch from 548ad86 to b284c41 Compare July 18, 2025 11:54
ivancea added 4 commits July 18, 2025 14:06
# Conflicts:
#	x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/EsqlSpecIT.java
ivancea added a commit that referenced this pull request Jul 21, 2025
`SampleOperator.Status` wasn't declared as a NamedWritable by the plugin, leading to serialization errors when `SAMPLE` is used with `profile: true`.

It leads to an `IllegalArgumentException: Unknown NamedWriteable [org.elasticsearch.compute.operator.Operator$Status][sample]`

Profiles will be tested in this PR: #131474, that's currently failing because of this bug
ivancea added a commit to ivancea/elasticsearch that referenced this pull request Jul 21, 2025
`SampleOperator.Status` wasn't declared as a NamedWritable by the plugin, leading to serialization errors when `SAMPLE` is used with `profile: true`.

It leads to an `IllegalArgumentException: Unknown NamedWriteable [org.elasticsearch.compute.operator.Operator$Status][sample]`

Profiles will be tested in this PR: elastic#131474, that's currently failing because of this bug
Copy link
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.

Looks good to me.

// new RestEsqlTestCase.RequestObjectBuilder().query("FROM test* | LIMIT 2"),
// new AssertWarnings.NoWarnings(),
// profileLogger
// );
Copy link
Member

Choose a reason for hiding this comment

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

Oh boy. I think maybe I should look at this one. I wrote this like two years ago.

Copy link

cla-checker-service bot commented Jul 21, 2025

💚 CLA has been signed

@@ -202,7 +202,7 @@ public static class Status implements Operator.Status {
public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(
Operator.Status.class,
"page_mapping_to_iterator",
AbstractPageMappingOperator.Status::new
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was another serialization error

@ivancea ivancea added the test-release Trigger CI checks against release build label Jul 22, 2025
@ivancea ivancea enabled auto-merge (squash) July 22, 2025 12:13
# Conflicts:
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
@ivancea ivancea merged commit 093d23b into elastic:main Jul 22, 2025
35 checks passed
@ivancea ivancea deleted the show-profile-on-test-error branch July 22, 2025 15:27
ivancea added a commit that referenced this pull request Jul 28, 2025
ivancea added a commit to ivancea/elasticsearch that referenced this pull request Jul 28, 2025
ivancea added a commit to ivancea/elasticsearch that referenced this pull request Jul 28, 2025
ivancea added a commit to ivancea/elasticsearch that referenced this pull request Jul 28, 2025
elasticsearchmachine pushed a commit that referenced this pull request Jul 28, 2025
elasticsearchmachine pushed a commit that referenced this pull request Jul 28, 2025
elasticsearchmachine pushed a commit that referenced this pull request Jul 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests test-release Trigger CI checks against release build v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants