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

Fix categorize csv test #113089

Merged
merged 5 commits into from
Sep 19, 2024
Merged

Fix categorize csv test #113089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
64454dd
Move CATEGORIZE from EsqlFeatures to EsqlCapabilities
jan-elastic Sep 18, 2024
c089300
Make CATEGORIZE csv test deterministic
jan-elastic Sep 18, 2024
7f515dd
Unmute categorize test
jan-elastic Sep 18, 2024
bf22956
spotless
jan-elastic Sep 18, 2024
cb64f98
Deterministic categorize csv test with category IDs
jan-elastic Sep 18, 2024
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
6 changes: 0 additions & 6 deletions muted-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,12 +221,6 @@ tests:
- class: org.elasticsearch.xpack.ml.integration.MlJobIT
method: testCreateJob_WithClashingFieldMappingsFails
issue: https://github.com/elastic/elasticsearch/issues/113046
- class: org.elasticsearch.xpack.esql.qa.multi_node.EsqlSpecIT
method: test {categorize.Categorize SYNC}
issue: https://github.com/elastic/elasticsearch/issues/113054
- class: org.elasticsearch.xpack.esql.qa.multi_node.EsqlSpecIT
method: test {categorize.Categorize ASYNC}
issue: https://github.com/elastic/elasticsearch/issues/113055
- class: org.elasticsearch.xpack.sql.qa.security.JdbcSqlSpecIT
method: test {case-functions.testUcaseInline1}
issue: https://github.com/elastic/elasticsearch/issues/112641
Expand Down
5 changes: 3 additions & 2 deletions x-pack/plugin/esql/qa/testFixtures/src/main/resources/categorize.csv-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@ categorize
required_capability: categorize

FROM sample_data
| STATS count=COUNT(), values=VALUES(message) BY category=CATEGORIZE(message)
| SORT count DESC, category ASC
| SORT message ASC
| STATS count=COUNT(), values=MV_SORT(VALUES(message)) BY category=CATEGORIZE(message)
| SORT category
;

count:long | values:keyword | category:integer
Expand Down
7 changes: 6 additions & 1 deletion x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,12 @@ public enum Cap {
/**
* Support explicit casting from string literal to DATE_PERIOD or TIME_DURATION.
*/
CAST_STRING_LITERAL_TO_TEMPORAL_AMOUNT;
CAST_STRING_LITERAL_TO_TEMPORAL_AMOUNT,

/**
* Supported the text categorization function "CATEGORIZE".
*/
CATEGORIZE(true);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question: does this piece of code work as intended?

If you add a non-existing required capability to a CSV test, it always passes.

Copy link
Contributor

@ivancea ivancea Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There's a capability presence check here:

elasticsearch/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/CsvTests.java

Lines 255 to 261 in bb872e6

if (Build.current().isSnapshot()) {
assertThat(
"Capability is not included in the enabled list capabilities on a snapshot build. Spelling mistake?",
testCase.requiredCapabilities,
everyItem(in(EsqlCapabilities.CAPABILITIES))
);
} else {

It was moved to snapshot-only for reasons I don't remember. It probably shouldn't.
I'm not sure if the CI runs a snapshot or not actually (I guess not); it feels weird that it didn't fail in any part of the process

Edit: Thinking about this again, the features are also automatically included as capabilities, so in snapshot mode, it maybe worked?

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 think the CI runs snapshot, and therefore it succeeded

Copy link
Member

Choose a reason for hiding this comment

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

There's a tag you can add to make the tests run in release mode if you want.

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've run the test locally with and without snapshot and both work, so I think we're good.

private final boolean snapshotOnly;
private final FeatureFlag featureFlag;
Expand Down
7 changes: 1 addition & 6 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/EsqlFeatures.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,14 +179,9 @@ public class EsqlFeatures implements FeatureSpecification {
*/
public static final NodeFeature RESOLVE_FIELDS_API = new NodeFeature("esql.resolve_fields_api");

/**
* Support categorize
*/
public static final NodeFeature CATEGORIZE = new NodeFeature("esql.categorize");
Copy link
Member

Choose a reason for hiding this comment

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

Oh yikes. I hadn't noticed the feature vs capability. You can't really remove these once they've left snapshot-land. But this one stayed in snapshot-land forever so I think it's safe to remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I missed the pretty explicit:

Don't make more of those - add them to {@link EsqlCapabilities} instead.


private Set<NodeFeature> snapshotBuildFeatures() {
assert Build.current().isSnapshot() : Build.current();
return Set.of(METRICS_SYNTAX, CATEGORIZE);
return Set.of(METRICS_SYNTAX);
}

@Override
Expand Down
Loading