ES|QL categorize with multiple groupings #118173

jan-elastic · 2024-12-06T16:01:54Z

No description provided.

elasticsearchmachine · 2024-12-06T16:02:52Z

Pinging @elastic/ml-core (Team:ML)

elasticsearchmachine · 2024-12-06T16:02:54Z

Hi @jan-elastic, I've created a changelog YAML for you.

...in/java/org/elasticsearch/compute/aggregation/blockhash/CategorizePackedValuesBlockHash.java

elasticsearchmachine · 2024-12-09T14:33:11Z

Hi @jan-elastic, I've created a changelog YAML for you.

nik9000

Very nice!

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/Page.java

...in/java/org/elasticsearch/compute/aggregation/blockhash/CategorizePackedValuesBlockHash.java

ivancea

Nice work 👀
Some nits and new tests in comments; the impl looks quite solid to me

...in/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/blockhash/BlockHash.java

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

...in/java/org/elasticsearch/compute/aggregation/blockhash/CategorizePackedValuesBlockHash.java

ivancea · 2024-12-10T13:37:12Z

...in/java/org/elasticsearch/compute/aggregation/blockhash/CategorizePackedValuesBlockHash.java

+                    if (id == 0) {
+                        builder.appendNull();
+                    } else {
+                        builder.appendBytesRef(regexes.getBytesRef(id + idsOffset, scratch));


Not for now: We're repeating, potentially, a lot of bytesref values here. I wonder if there is or it would make sense to have a BytesRefBlock that instead of all the bytesrefs, stores every value just once, and then a reference per position:

AAAAAA BBBBBBB AAAAAA AAAAAA

->

// 1: AAAAAA // 2: BBBBBBB 1 2 1 1

@nik9000 Something to consider for later? Maybe it's too specific for this. And anyway, the next EVAL or whatever will duplicate the value again.

That sounds like a nice thing to have, but definitely out of scope for this PR.

However, the next EVAL should not duplicate the value again.

If you have:

// 1: AAAAAA // 2: BBBBBBB 1 2 1 1

then an efficient EVAL x=SUBSTRING(x, 1, 2) should give

// 1: AA // 2: BB 1 2 1 1

without ever duplicating.

For that SUBSTRING to not duplicate, we would need to add that "hashtable" strategy in the BytesRefBlockBuilder. It looks goo (?), but I wonder if using that by default could perform negatively in some scenarios. Something to try eventually probably

Sounds like worth trying in the future. Are you making a note (issue) of this, so that the idea doesn't get lost?

Sure! I'll comment it with Nik, just in case it was considered and discarded already, and then I'll document it in an issue somewhere

jan-elastic · 2024-12-11T10:34:12Z

Thanks for the feedback @ivancea . I've processed all your comments. PTAL

…egorize-multiple-groupings

ivancea

Thanks!

alex-spies

Heya, this generally looks very good, thank you @jan-elastic !

I only have one observation about potential untracked memory that I think we should ponder at least a bit to ensure we're safe; see below.

alex-spies · 2024-12-11T11:26:05Z

...in/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/blockhash/BlockHash.java

+            assert groups.get(0).isCategorize();
+            assert groups.subList(1, groups.size()).stream().noneMatch(GroupSpec::isCategorize);


nit/bikeshed: throwing IllegalArgumentException would be more friendly towards test; when assertions trigger, they bring down a whole node because that's an error, not exception. It's probably fine, though.

This shouldn't happen, right? If this assertion fails, other code is broken (the verifier). I'll leave it as is unless you object. BTW, do you know if we run assertions in production?

Assertions are disabled in Prod, and this indeed shouldn't happen. Occasionally, a bug slips through, though, and when it triggers an assertion, it kill the whole IT suite run because it kills a node. It's fine to leave as-is, though!

alex-spies · 2024-12-11T12:09:49Z

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

@@ -1897,28 +1897,21 @@ public void testIntervalAsString() {
    public void testCategorizeSingleGrouping() {
        assumeTrue("requires Categorize capability", EsqlCapabilities.Cap.CATEGORIZE_V5.isEnabled());


out of scope, but this assumeTrue could be removed now that CATEGORIZE is in tech preview.

Removed them all from the unit tests. I still need them in the csv tests, right (for bwc tests)?

The capability checks remain required, otherwise we'll run new tests against old nodes, indeed!

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

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

...in/java/org/elasticsearch/compute/aggregation/blockhash/CategorizePackedValuesBlockHash.java

alex-spies · 2024-12-11T13:37:22Z

...in/java/org/elasticsearch/compute/aggregation/blockhash/CategorizePackedValuesBlockHash.java

+            try (BytesStreamOutput out = new BytesStreamOutput()) {
+                out.writeBytesRef(categorizeBlockHash.serializeCategorizer());
+                IntVector idsVector = (IntVector) keys[0].asVector();
+                int[] idsArray = new int[idsVector.getPositionCount()];


I'm a little afraid that we allocate potentially quite a bit of memory without asking the breaker first. I believe this will lead to tricky-to-debug situations when the memory pressure is already high and this leads to an OOM. Not sure how likely, but still.

The blockFactory has convenience methods preAdjustBreakerForInt and adjustBreaker that we better use here. That needs to be done carefully re. try/catching as not to have a circuit breaker leak.

@nik9000 wdyt? Should we play it safe here?

Instead of manually handling the memory here, maybe we should just do a idsVector.writeTo(...), so we remove a chunk of code from here, and avoid allocating anything else?

It would be nice to track these. I'm not sure it has to be a blocker though. Until a few months ago aggs didn't track the a few similar things to this. OTOH, it could cause problems.....

Ah, it looks like we just writeIntArray with this. In that case, yeah, I'd write the ids manually.

Ok, I checked and we also don't really track memory in CategorizeBlockHash.getKeys; ~~neither do we track the memory for the categorizer itself.~~ Update: actually, we probably do - so that should be covered.

The problem here is that due to combinatorial explosion, the untracked memory when writing the idsVector can be a lot larger than the actual categorizer state.

E.g. STATS ... BY CATEGORIZE(message), field1, field2.

If there are n categories of messages, m distinct field1 values and o distinct field2 values, then the number of rows - and thus ids - will be n*m*o. And we're copying this twice: once into an int[] and another time when writing into out.

I'll leave this up to you all to decide...

...in/java/org/elasticsearch/compute/aggregation/blockhash/CategorizePackedValuesBlockHash.java

alex-spies

Let's wait with merging until we had a chance to look at this subtle point.

alex-spies · 2024-12-11T14:39:37Z

...va/org/elasticsearch/compute/aggregation/blockhash/CategorizePackedValuesBlockHashTests.java

Heya, the test here is a good start - but we could maybe also add some randomization, nulls and multivalues to gain more confidence.

jan-elastic · 2024-12-11T16:15:14Z

@alex-spies Thanks for the review! Processed all your comments, except the OOM one, which needs more discussion. PTAL

alex-spies

Thanks @jan-elastic !

Let's take care of the memory accounting in a follow-up.

Maybe we want to add a couple more block hash test cases, but otherwise this LGTM and should be unblocked :)

alex-spies · 2024-12-11T17:05:03Z

...in/java/org/elasticsearch/compute/aggregation/blockhash/CategorizePackedValuesBlockHash.java

-                    return newIds.build();
-                }
+                int[] ids = in.readIntArray();
+                ids = categorizeBlockHash.recategorize(categorizerState, ids);


++ definitely easier to grasp, thanks!

alex-spies · 2024-12-11T17:07:25Z

...va/org/elasticsearch/compute/aggregation/blockhash/CategorizePackedValuesBlockHashTests.java

This could still use randomization, nulls and multi-values.

jan-elastic added >non-issue :ml Machine learning v9.0.0 v8.18.0 Team:ML Meta label for the ML team >feature and removed >non-issue labels Dec 6, 2024

jan-elastic marked this pull request as draft December 6, 2024 16:02

jan-elastic requested a review from nik9000 December 6, 2024 16:03

alex-spies self-requested a review December 6, 2024 16:04

jan-elastic force-pushed the esql-categorize-multiple-groupings branch 2 times, most recently from 099e143 to 7409695 Compare December 9, 2024 09:05

jan-elastic added 5 commits December 9, 2024 15:19

ES|QL categorize with multiple groupings.

41279c7

Fix VerifierTests

9f770d5

Close stuff when constructing CategorizePackedValuesBlockHash fails

d7af8bf

CategorizePackedValuesBlockHashTests

e6ac068

Improve categorize javadocs

35e9811

jan-elastic force-pushed the esql-categorize-multiple-groupings branch from 7409695 to 35e9811 Compare December 9, 2024 14:22

jan-elastic commented Dec 9, 2024

View reviewed changes

...in/java/org/elasticsearch/compute/aggregation/blockhash/CategorizePackedValuesBlockHash.java Outdated Show resolved Hide resolved

jan-elastic marked this pull request as ready for review December 9, 2024 14:32

Update docs/changelog/118173.yaml

b44663a

nik9000 reviewed Dec 9, 2024

View reviewed changes

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/Page.java Outdated Show resolved Hide resolved

...in/java/org/elasticsearch/compute/aggregation/blockhash/CategorizePackedValuesBlockHash.java Outdated Show resolved Hide resolved

Create CategorizePackedValuesBlockHash's deletegate page differently

82a38f9

jan-elastic requested a review from nik9000 December 9, 2024 16:17

ivancea self-requested a review December 10, 2024 11:52

jan-elastic added the auto-backport Automatically create backport pull requests when merged label Dec 10, 2024

ivancea reviewed Dec 10, 2024

View reviewed changes

jan-elastic added 3 commits December 11, 2024 10:18

Double check in BlockHash builder for single categorize

5121e9a

Reuse blocks array

3c0325a

More CSV tests

e05f860

jan-elastic requested a review from ivancea December 11, 2024 10:33

Merge branch 'main' of github.com:elastic/elasticsearch into esql-cat…

abfc211

…egorize-multiple-groupings

ivancea approved these changes Dec 11, 2024

View reviewed changes

alex-spies reviewed Dec 11, 2024

View reviewed changes

alex-spies requested changes Dec 11, 2024

View reviewed changes

alex-spies reviewed Dec 11, 2024

View reviewed changes

jan-elastic added 7 commits December 11, 2024 16:10

Remove assumeTrue categorize_v5

54ac1bf

Rename test

5eb5189

Two more verifier tests

1deb2d4

more CSV tests

a633806

Add JavaDocs/comments

e639268

spotless

5a4c8bb

Refactor/unify recategorize

5ecc9f9

alex-spies approved these changes Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ES|QL categorize with multiple groupings #118173

ES|QL categorize with multiple groupings #118173

jan-elastic commented Dec 6, 2024

elasticsearchmachine commented Dec 6, 2024

elasticsearchmachine commented Dec 6, 2024

elasticsearchmachine commented Dec 9, 2024

nik9000 left a comment

ivancea left a comment

ivancea Dec 10, 2024

jan-elastic Dec 11, 2024

ivancea Dec 11, 2024

jan-elastic Dec 11, 2024

ivancea Dec 11, 2024

jan-elastic commented Dec 11, 2024

ivancea left a comment

alex-spies left a comment •

edited

Loading

alex-spies Dec 11, 2024

jan-elastic Dec 11, 2024

alex-spies Dec 11, 2024

alex-spies Dec 11, 2024

jan-elastic Dec 11, 2024 •

edited

Loading

alex-spies Dec 11, 2024

alex-spies Dec 11, 2024

ivancea Dec 11, 2024

nik9000 Dec 11, 2024

nik9000 Dec 11, 2024

alex-spies Dec 11, 2024 •

edited

Loading

jan-elastic Dec 11, 2024

alex-spies left a comment

alex-spies Dec 11, 2024

jan-elastic commented Dec 11, 2024 •

edited

Loading

alex-spies left a comment

alex-spies Dec 11, 2024

alex-spies Dec 11, 2024

		assert groups.get(0).isCategorize();
		assert groups.subList(1, groups.size()).stream().noneMatch(GroupSpec::isCategorize);

		@@ -1897,28 +1897,21 @@ public void testIntervalAsString() {
		public void testCategorizeSingleGrouping() {
		assumeTrue("requires Categorize capability", EsqlCapabilities.Cap.CATEGORIZE_V5.isEnabled());

ES|QL categorize with multiple groupings #118173

Are you sure you want to change the base?

ES|QL categorize with multiple groupings #118173

Conversation

jan-elastic commented Dec 6, 2024

elasticsearchmachine commented Dec 6, 2024

elasticsearchmachine commented Dec 6, 2024

elasticsearchmachine commented Dec 9, 2024

nik9000 left a comment

Choose a reason for hiding this comment

ivancea left a 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

jan-elastic commented Dec 11, 2024

ivancea left a comment

Choose a reason for hiding this comment

alex-spies left a comment • 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

jan-elastic Dec 11, 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

alex-spies Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-spies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-elastic commented Dec 11, 2024 • edited Loading

alex-spies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-spies left a comment •

edited

Loading

jan-elastic Dec 11, 2024 •

edited

Loading

alex-spies Dec 11, 2024 •

edited

Loading

jan-elastic commented Dec 11, 2024 •

edited

Loading