Fix categorize csv test #113089

jan-elastic · 2024-09-18T08:19:25Z

No description provided.

elasticsearchmachine · 2024-09-18T08:19:49Z

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

jan-elastic · 2024-09-18T08:50:45Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

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



Question: does this piece of code work as intended?

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

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?

I think the CI runs snapshot, and therefore it succeeded

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

I've run the test locally with and without snapshot and both work, so I think we're good.

ivancea · 2024-09-18T12:59:14Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/categorize.csv-spec

@@ -1,13 +1,15 @@
 categorize
 required_capability: categorize

+# Drop the category ID, because it's non-deterministic


Is it not deterministic because of the order of the rows? What about sorting the data before it enters the STATS?
Dumping the tested function output is weird 👀

I've re-added the category column and a SORT message inbetween. Thanks for the suggestion.

For my understanding: does the additional SORT step mean that every CATEGORIZE is happening on the coordinating node? I think it must be, otherwise this isn't working correctly, right?

For my understanding: does the additional SORT step mean that every CATEGORIZE is happening on the coordinating node? I think it must be, otherwise this isn't working correctly, right?

Yes. Your instincts for where we split are almost certainly right. We'll just run the agg after the sort. It's not ideal testing, but it's fine for now.

Uhm, the CSV tests are executed in different ways. The CsvTests class, I think it executes like an unit test, like a single node. There are also tests like https://github.com/elastic/elasticsearch/blob/c1daf18bf5f5178d43dc17d3a3d1f5db9773098e/x-pack/plugin/esql/qa/server/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java, which use a full cluster.
But now thinking about the categorize function and how it works, I don't really know how is the sort executed. And specially as this is the first grouping function depending on order, maybe it makes sense in fact to not check the category column...

Sorry, I think the original solution was correct 🙏
Let's see if @nik9000 can confirm this

This failing test suggests that data does not arrive at the categorizer in sorted order.

@nik9000 do you understand why?

I'll leave the test muted for now. Once we replace the (arbitrary) IDs by regexes, the output will be deterministic.

ivancea · 2024-09-18T13:02:22Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

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



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?

nik9000 · 2024-09-18T14:26:38Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

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



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

nik9000 · 2024-09-18T14:27:57Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/EsqlFeatures.java

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


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.

And I missed the pretty explicit:

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

* Move CATEGORIZE from EsqlFeatures to EsqlCapabilities * Make CATEGORIZE csv test deterministic * Unmute categorize test * spotless * Deterministic categorize csv test with category IDs

* ES|QL categorize v1 (#112860) * Prepare TokenListCategorizer for usage in ES|QL * Expose text categorization from ML module * Let esql plugin depend on ml plugin * Fix/suppress this-escape * Incomplete v1 of ES|QL Categorize * Refactor / remove CategorizeInternal * Fix categorize csv test (#113089) * Move CATEGORIZE from EsqlFeatures to EsqlCapabilities * Make CATEGORIZE csv test deterministic * Unmute categorize test * spotless * Deterministic categorize csv test with category IDs --------- Co-authored-by: Jan Kuipers <148754765+jan-elastic@users.noreply.github.com>

jan-elastic 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 labels Sep 18, 2024

jan-elastic requested a review from a team as a code owner September 18, 2024 08:19

elasticsearchmachine added the v9.0.0 label Sep 18, 2024

This was referenced Sep 18, 2024

[CI] EsqlSpecIT test {categorize.Categorize SYNC} failing #113054

Open

[CI] EsqlSpecIT test {categorize.Categorize ASYNC} failing #113055

Open

[CI] CsvTests test {categorize.Categorize} failing #113064

Open

jan-elastic force-pushed the fix-categorize-csv-test branch from 0c93423 to bf730ee Compare September 18, 2024 08:27

jan-elastic added 3 commits September 18, 2024 10:38

Move CATEGORIZE from EsqlFeatures to EsqlCapabilities

64454dd

Make CATEGORIZE csv test deterministic

c089300

Unmute categorize test

7f515dd

jan-elastic force-pushed the fix-categorize-csv-test branch from 90e855c to 7f515dd Compare September 18, 2024 08:40

jan-elastic assigned nik9000 Sep 18, 2024

astefan added the test-release Trigger CI checks against release build label Sep 18, 2024

jan-elastic commented Sep 18, 2024

View reviewed changes

spotless

bf22956

astefan requested a review from not-napoleon September 18, 2024 11:54

astefan unassigned nik9000 Sep 18, 2024

ivancea reviewed Sep 18, 2024

View reviewed changes

Deterministic categorize csv test with category IDs

cb64f98

jan-elastic requested a review from nik9000 September 18, 2024 14:01

nik9000 approved these changes Sep 18, 2024

View reviewed changes

jan-elastic merged commit 47f62f5 into main Sep 19, 2024
17 of 18 checks passed

jan-elastic deleted the fix-categorize-csv-test branch September 19, 2024 06:52

nik9000 added the v8.16.0 label Sep 19, 2024

nik9000 mentioned this pull request Sep 19, 2024

Incomplete version of CATEGORIZE for ESQL #113207

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix categorize csv test #113089

Fix categorize csv test #113089

jan-elastic commented Sep 18, 2024

elasticsearchmachine commented Sep 18, 2024

jan-elastic Sep 18, 2024

ivancea Sep 18, 2024 •

edited

Loading

jan-elastic Sep 18, 2024

nik9000 Sep 18, 2024

jan-elastic Sep 18, 2024

ivancea Sep 18, 2024

jan-elastic Sep 18, 2024

nik9000 Sep 18, 2024

ivancea Sep 18, 2024

jan-elastic Sep 20, 2024

ivancea Sep 18, 2024 •

edited

Loading

nik9000 Sep 18, 2024

nik9000 Sep 18, 2024

jan-elastic Sep 18, 2024

	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 {

Fix categorize csv test #113089

Fix categorize csv test #113089

Conversation

jan-elastic commented Sep 18, 2024

elasticsearchmachine commented Sep 18, 2024

Choose a reason for hiding this comment

ivancea Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivancea Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivancea Sep 18, 2024 •

edited

Loading

ivancea Sep 18, 2024 •

edited

Loading