ES|QL categorize with multiple groupings by jan-elastic · Pull Request #118173 · elastic/elasticsearch

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

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.

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

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

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

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

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 :)

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

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

…egorize-multiple-groupings

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

…egorize-multiple-groupings

elasticsearchmachine · 2024-12-12T15:52:01Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 118173

* ES|QL categorize with multiple groupings. * Fix VerifierTests * Close stuff when constructing CategorizePackedValuesBlockHash fails * CategorizePackedValuesBlockHashTests * Improve categorize javadocs * Update docs/changelog/118173.yaml * Create CategorizePackedValuesBlockHash's deletegate page differently * Double check in BlockHash builder for single categorize * Reuse blocks array * More CSV tests * Remove assumeTrue categorize_v5 * Rename test * Two more verifier tests * more CSV tests * Add JavaDocs/comments * spotless * Refactor/unify recategorize * Better memory accounting * fix csv test * randomize CategorizePackedValuesBlockHashTests * Add TODO

* ES|QL categorize with multiple groupings. * Fix VerifierTests * Close stuff when constructing CategorizePackedValuesBlockHash fails * CategorizePackedValuesBlockHashTests * Improve categorize javadocs * Update docs/changelog/118173.yaml * Create CategorizePackedValuesBlockHash's deletegate page differently * Double check in BlockHash builder for single categorize * Reuse blocks array * More CSV tests * Remove assumeTrue categorize_v5 * Rename test * Two more verifier tests * more CSV tests * Add JavaDocs/comments * spotless * Refactor/unify recategorize * Better memory accounting * fix csv test * randomize CategorizePackedValuesBlockHashTests * Add TODO Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…8590) * ES|QL categorize with multiple groupings. * Fix VerifierTests * Close stuff when constructing CategorizePackedValuesBlockHash fails * CategorizePackedValuesBlockHashTests * Improve categorize javadocs * Update docs/changelog/118173.yaml * Create CategorizePackedValuesBlockHash's deletegate page differently * Double check in BlockHash builder for single categorize * Reuse blocks array * More CSV tests * Remove assumeTrue categorize_v5 * Rename test * Two more verifier tests * more CSV tests * Add JavaDocs/comments * spotless * Refactor/unify recategorize * Better memory accounting * fix csv test * randomize CategorizePackedValuesBlockHashTests * Add TODO Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

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

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

...va/org/elasticsearch/compute/aggregation/blockhash/CategorizePackedValuesBlockHashTests.java Show resolved Hide resolved

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

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

...va/org/elasticsearch/compute/aggregation/blockhash/CategorizePackedValuesBlockHashTests.java Show resolved Hide resolved

jan-elastic added 3 commits December 12, 2024 10:34

Better memory accounting

16680d2

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

9588e10

…egorize-multiple-groupings

fix csv test

a9c3ed7

ivancea reviewed Dec 12, 2024

View reviewed changes

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

jan-elastic added 4 commits December 12, 2024 12:48

randomize CategorizePackedValuesBlockHashTests

8dd15ac

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

7f82b43

…egorize-multiple-groupings

Add TODO

0b0be87

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

3bccc78

…egorize-multiple-groupings

jan-elastic merged commit d270158 into main Dec 12, 2024

jan-elastic deleted the esql-categorize-multiple-groupings branch December 12, 2024 15:50

elasticsearchmachine added the backport pending label Dec 12, 2024

jan-elastic mentioned this pull request Dec 12, 2024

[8.x] ES|QL categorize with multiple groupings (#118173) #118590

Merged

Conversation

jan-elastic commented Dec 6, 2024

elasticsearchmachine commented Dec 6, 2024

elasticsearchmachine commented Dec 6, 2024

Uh oh!

elasticsearchmachine commented Dec 9, 2024

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ivancea left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ivancea Dec 10, 2024

Choose a reason for hiding this comment

jan-elastic Dec 11, 2024

Choose a reason for hiding this comment

ivancea Dec 11, 2024

Choose a reason for hiding this comment

jan-elastic Dec 11, 2024

Choose a reason for hiding this comment

ivancea Dec 11, 2024

Choose a reason for hiding this comment

ivancea left a comment

Choose a reason for hiding this comment

alex-spies left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

jan-elastic commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented Dec 12, 2024

💔 Backport failed

Labels

5 participants

alex-spies left a comment •

edited

Loading

jan-elastic commented Dec 11, 2024 •

edited

Loading