ES|QL: Fix BytesRef2BlockHash #130705

luigidellaquila · 2025-07-07T09:56:57Z

Fix retrieval of keys in BytesRef2BlockHash

Fixes: #130644

elasticsearchmachine · 2025-07-07T09:57:21Z

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

elasticsearchmachine · 2025-07-07T09:57:44Z

Hi @luigidellaquila, I've created a changelog YAML for you.

luigidellaquila · 2025-07-07T09:59:37Z

...ql/compute/src/test/java/org/elasticsearch/compute/aggregation/blockhash/BlockHashTests.java

+        }
+    }
+
+    public void test2BytesRefs() {


This is the same test we had for BytesRef3BlockHash

luigidellaquila · 2025-07-07T09:59:54Z

...ql/compute/src/test/java/org/elasticsearch/compute/aggregation/blockhash/BlockHashTests.java

@@ -1326,7 +1513,7 @@ public void close() {
                    fail("hashes should not close AddInput");
                }
            });
-            assertThat(output1.size(), equalTo(output1.size()));


A typo probably

luigidellaquila · 2025-07-07T10:00:33Z

...ql/compute/src/test/java/org/elasticsearch/compute/aggregation/blockhash/BlockHashTests.java

+                    builder.append(BytesRefs.toString(block1.getBytesRef(i, scratch)));
+                    distinctKeys.add(builder.toString());
+                }
+                assertThat(distinctKeys.size(), equalTo(totalPositions));


Revert the fix and you'll see a failure here

bpintea · 2025-07-07T11:36:15Z

...ompute/src/main/java/org/elasticsearch/compute/aggregation/blockhash/BytesRef2BlockHash.java

@@ -145,7 +145,7 @@ public Block[] getKeys() {
        try {
            try (BytesRefBlock.Builder b1 = blockFactory.newBytesRefBlockBuilder(positions)) {
                for (int i = 0; i < positions; i++) {
-                    int k1 = (int) (finalHash.get(i) & 0xffffL);
+                    int k1 = (int) (finalHash.get(i) & 0xffffffffL);
                    if (k1 == 0) {


Shouldn't this (and below) be if (k <= 0) { ?

It can't be 0 because we grab the low bits. Right? I haven't had enough coffee for bit math.

Could the finalHash.get(i) not return smth like 0xe0000000L (or some other value with the 32nd bit set)?
The hash1.hash.get() method seems to expect a non-negative argument. It might all be safe, it just stood out to me since we're truncating a long.

These are always positive, they are generated by BytesRefBlockHash, see hashOrdToGroupNullReserved

I guess one of the implicit assumptions is that a block hash will never have more than 2B positions. If it does, we'll need a larger data structure anyway I guess.

These are always positive

To make sure: 0xe0000000L is a positive long. But (int) (0xe0000000L & 0xffffffffL) is a negative int.

I guess one of the implicit assumptions is that a block hash will never have more than 2B positions.

Yeh, this is why I was thinking that this might still be safe. 👍 (At least if these are positions.)

I think @nik9000 should probably comment here as well - and whatever we end up with should probably become a Java comment here, that's too many assumptions for my head to infer, at least when skimming the file :D

alex-spies

Looks good, only a minor remark from me. Thanks @luigidellaquila !

It'd be good if @nik9000 could also give this a look as he's way more familiar with the block hashes than me.

alex-spies · 2025-07-07T11:28:41Z

...ql/compute/src/test/java/org/elasticsearch/compute/aggregation/blockhash/BlockHashTests.java

@@ -1232,6 +1235,190 @@ public void testLongNull() {
        }, blockFactory.newLongArrayVector(values, values.length).asBlock(), blockFactory.newConstantNullBlock(values.length));
    }

+    public void test2BytesRefsKeys() {


nit: maybe a more descriptive name? Like test2BytesRefsHighCardinalityKey or so?

👍 on HighCardinality.

This is java. Lean into the memes about long method names....

alex-spies · 2025-07-07T11:30:30Z

...ql/compute/src/test/java/org/elasticsearch/compute/aggregation/blockhash/BlockHashTests.java

+        }
+        List<Output> output = new ArrayList<>();
+
+        try (BlockHash hash1 = new BytesRef2BlockHash(blockFactory, 0, 1, totalPositions);) {


Can we randomize which key has high cardinality? It looks like this test only stuffs the high cardinality key into the first position, which repros the bug, but in principle the other position might have the same problem (if we implemented things differently).

nik9000 · 2025-07-07T12:41:40Z

...ompute/src/main/java/org/elasticsearch/compute/aggregation/blockhash/BytesRef2BlockHash.java

@@ -145,7 +145,7 @@ public Block[] getKeys() {
        try {
            try (BytesRefBlock.Builder b1 = blockFactory.newBytesRefBlockBuilder(positions)) {
                for (int i = 0; i < positions; i++) {
-                    int k1 = (int) (finalHash.get(i) & 0xffffL);
+                    int k1 = (int) (finalHash.get(i) & 0xffffffffL);
                    if (k1 == 0) {


It can't be 0 because we grab the low bits. Right? I haven't had enough coffee for bit math.

nik9000 · 2025-07-07T12:41:53Z

...ql/compute/src/test/java/org/elasticsearch/compute/aggregation/blockhash/BlockHashTests.java

@@ -1232,6 +1235,190 @@ public void testLongNull() {
        }, blockFactory.newLongArrayVector(values, values.length).asBlock(), blockFactory.newConstantNullBlock(values.length));
    }

+    public void test2BytesRefsKeys() {


👍 on HighCardinality.

nik9000 · 2025-07-07T12:42:09Z

...ql/compute/src/test/java/org/elasticsearch/compute/aggregation/blockhash/BlockHashTests.java

+        }
+        List<Output> output = new ArrayList<>();
+
+        try (BlockHash hash1 = new BytesRef2BlockHash(blockFactory, 0, 1, totalPositions);) {


nik9000 · 2025-07-07T12:46:14Z

...ql/compute/src/test/java/org/elasticsearch/compute/aggregation/blockhash/BlockHashTests.java

@@ -1232,6 +1235,190 @@ public void testLongNull() {
        }, blockFactory.newLongArrayVector(values, values.length).asBlock(), blockFactory.newConstantNullBlock(values.length));
    }

+    public void test2BytesRefsKeys() {


This is java. Lean into the memes about long method names....

nik9000 · 2025-07-07T12:47:29Z

...ql/compute/src/test/java/org/elasticsearch/compute/aggregation/blockhash/BlockHashTests.java

+                }
+            }
+            page = new Page(builder1.build(), builder2.build());
+        }


Do we want this all in one page? I know the BlockHash needs it in memory. And we build that HashMap with it in memory below. But maybe we make Pages of a reasonable size here?

Meh. I guess it's only like 9mb. Probably not worth changing it unless you need to go bigger.

Yeah, it's small enough, and it reproduced the problem pretty consistently, so I guess it's not worth the effort and complication.

ES|QL: Fix BytesRef2BlockHash

16dde0e

luigidellaquila added >bug :Analytics/ES|QL AKA ESQL labels Jul 7, 2025

elasticsearchmachine added the v9.2.0 label Jul 7, 2025

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 7, 2025

Update docs/changelog/130705.yaml

f8f1805

luigidellaquila added v8.19.0 v9.1.0 v8.16.7 v9.0.4 v8.18.4 v8.17.9 and removed Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.2.0 labels Jul 7, 2025

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 7, 2025

luigidellaquila added v9.2.0 and removed Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Jul 7, 2025

github-actions bot deployed to docs-preview July 7, 2025 09:58 View deployment

luigidellaquila added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) auto-backport Automatically create backport pull requests when merged labels Jul 7, 2025

luigidellaquila commented Jul 7, 2025

View reviewed changes

idegtiarenko approved these changes Jul 7, 2025

View reviewed changes

bpintea reviewed Jul 7, 2025

View reviewed changes

alex-spies approved these changes Jul 7, 2025

View reviewed changes

nik9000 approved these changes Jul 7, 2025

View reviewed changes

ES|QL: Fix BytesRef2BlockHash #130705

Are you sure you want to change the base?

ES|QL: Fix BytesRef2BlockHash #130705

Uh oh!

Conversation

luigidellaquila commented Jul 7, 2025

Uh oh!

elasticsearchmachine commented Jul 7, 2025

Uh oh!

elasticsearchmachine commented Jul 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpintea Jul 7, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpintea Jul 7, 2025 • 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!

alex-spies Jul 7, 2025 • 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!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bpintea Jul 7, 2025 •

edited

Loading

bpintea Jul 7, 2025 •

edited

Loading

alex-spies Jul 7, 2025 •

edited

Loading