Skip to content

Commit a383fce

Browse files
authored
[ES|QL] Change equals and hashcode for ConstantNullBlock (#131817) (#131892)
Currently in ES|QL if you have a ConstantNullBlock and a DoubleBlock (or any other standard block type: Boolean, BytesRef, Float, Int, Long) and your DoubleBlock can be represented as a ConstantNullBlock, then `doubleBlock.equals(constantNullBlock)` can evaluate as `true` (if position count is the same). However, `constantNullBlock.equals(anyDoubleBlock)` is always false. Likewise, the hashcodes of these two blocks are different, even if `doubleBlock.equals(constantNullBlock)` returns true. This PR addresses that by making the hashcodes equivalent and the equals functions symmetric in returning true.
1 parent bde1a24 commit a383fce

File tree

8 files changed

+75
-15
lines changed

8 files changed

+75
-15
lines changed

docs/changelog/131817.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 131817
2+
summary: Change equals and hashcode for `ConstantNullBlock`
3+
area: ES|QL
4+
type: bug
5+
issues: []

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

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
import org.elasticsearch.core.ReleasableIterator;
1515

1616
import java.io.IOException;
17-
import java.util.Objects;
1817

1918
/**
2019
* Block implementation representing a constant null value.
@@ -120,15 +119,28 @@ public long ramBytesUsed() {
120119

121120
@Override
122121
public boolean equals(Object obj) {
123-
if (obj instanceof ConstantNullBlock that) {
124-
return this.getPositionCount() == that.getPositionCount();
122+
if (obj instanceof Block that) {
123+
return this.getPositionCount() == 0 && that.getPositionCount() == 0
124+
|| this.getPositionCount() == that.getPositionCount() && that.areAllValuesNull();
125+
}
126+
if (obj instanceof Vector that) {
127+
return this.getPositionCount() == 0 && that.getPositionCount() == 0;
125128
}
126129
return false;
127130
}
128131

129132
@Override
130133
public int hashCode() {
131-
return Objects.hash(getPositionCount());
134+
// The hashcode for ConstantNullBlock is calculated in this way so that
135+
// we return the same hashcode for ConstantNullBlock as we would for block
136+
// types that ConstantNullBlock implements that contain only null values.
137+
// Example: a DoubleBlock with 8 positions that are all null will return
138+
// the same hashcode as a ConstantNullBlock with a positionCount of 8.
139+
int result = 1;
140+
for (int pos = 0; pos < positionCount; pos++) {
141+
result = 31 * result - 1;
142+
}
143+
return result;
132144
}
133145

134146
@Override

x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/data/BooleanBlockEqualityTests.java

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,8 @@ public void testEmptyBlock() {
5252
blockFactory.newConstantBooleanBlockWith(randomBoolean(), 0),
5353
blockFactory.newBooleanBlockBuilder(0).build(),
5454
blockFactory.newBooleanBlockBuilder(0).appendBoolean(randomBoolean()).build().filter(),
55-
blockFactory.newBooleanBlockBuilder(0).appendNull().build().filter()
55+
blockFactory.newBooleanBlockBuilder(0).appendNull().build().filter(),
56+
(ConstantNullBlock) blockFactory.newConstantNullBlock(0)
5657
);
5758
assertAllEquals(blocks);
5859
}
@@ -252,6 +253,28 @@ public void testBlockInequality() {
252253
assertAllNotEquals(notEqualBlocks);
253254
}
254255

256+
public void testSimpleBlockWithManyNulls() {
257+
int positions = randomIntBetween(1, 256);
258+
boolean grow = randomBoolean();
259+
BooleanBlock.Builder builder1 = blockFactory.newBooleanBlockBuilder(grow ? 0 : positions);
260+
BooleanBlock.Builder builder2 = blockFactory.newBooleanBlockBuilder(grow ? 0 : positions);
261+
ConstantNullBlock.Builder builder3 = new ConstantNullBlock.Builder(blockFactory);
262+
for (int p = 0; p < positions; p++) {
263+
builder1.appendNull();
264+
builder2.appendNull();
265+
builder3.appendNull();
266+
}
267+
BooleanBlock block1 = builder1.build();
268+
BooleanBlock block2 = builder2.build();
269+
Block block3 = builder3.build();
270+
assertEquals(positions, block1.getPositionCount());
271+
assertTrue(block1.mayHaveNulls());
272+
assertTrue(block1.isNull(0));
273+
274+
List<Block> blocks = List.of(block1, block2, block3);
275+
assertAllEquals(blocks);
276+
}
277+
255278
static void assertAllEquals(List<?> objs) {
256279
for (Object obj1 : objs) {
257280
for (Object obj2 : objs) {

x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/data/BytesRefBlockEqualityTests.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,8 @@ public void testEmptyBlock() {
6363
blockFactory.newConstantBytesRefBlockWith(new BytesRef(), 0),
6464
blockFactory.newBytesRefBlockBuilder(0).build(),
6565
blockFactory.newBytesRefBlockBuilder(0).appendBytesRef(new BytesRef()).build().filter(),
66-
blockFactory.newBytesRefBlockBuilder(0).appendNull().build().filter()
66+
blockFactory.newBytesRefBlockBuilder(0).appendNull().build().filter(),
67+
(ConstantNullBlock) blockFactory.newConstantNullBlock(0)
6768
);
6869
assertAllEquals(blocks);
6970
}
@@ -357,17 +358,20 @@ public void testSimpleBlockWithManyNulls() {
357358
boolean grow = randomBoolean();
358359
BytesRefBlock.Builder builder1 = blockFactory.newBytesRefBlockBuilder(grow ? 0 : positions);
359360
BytesRefBlock.Builder builder2 = blockFactory.newBytesRefBlockBuilder(grow ? 0 : positions);
361+
ConstantNullBlock.Builder builder3 = new ConstantNullBlock.Builder(blockFactory);
360362
for (int p = 0; p < positions; p++) {
361363
builder1.appendNull();
362364
builder2.appendNull();
365+
builder3.appendNull();
363366
}
364367
BytesRefBlock block1 = builder1.build();
365368
BytesRefBlock block2 = builder2.build();
369+
Block block3 = builder3.build();
366370
assertEquals(positions, block1.getPositionCount());
367371
assertTrue(block1.mayHaveNulls());
368372
assertTrue(block1.isNull(0));
369373

370-
List<BytesRefBlock> blocks = List.of(block1, block2);
374+
List<Block> blocks = List.of(block1, block2, block3);
371375
assertAllEquals(blocks);
372376
}
373377

x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/data/DoubleBlockEqualityTests.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ public void testEmptyBlock() {
5151
blockFactory.newConstantDoubleBlockWith(0, 0),
5252
blockFactory.newDoubleBlockBuilder(0).build(),
5353
blockFactory.newDoubleBlockBuilder(0).appendDouble(1).build().filter(),
54-
blockFactory.newDoubleBlockBuilder(0).appendNull().build().filter()
54+
blockFactory.newDoubleBlockBuilder(0).appendNull().build().filter(),
55+
(ConstantNullBlock) blockFactory.newConstantNullBlock(0)
5556
);
5657
assertAllEquals(blocks);
5758
Releasables.close(blocks);
@@ -234,17 +235,20 @@ public void testSimpleBlockWithManyNulls() {
234235
boolean grow = randomBoolean();
235236
DoubleBlock.Builder builder1 = blockFactory.newDoubleBlockBuilder(grow ? 0 : positions);
236237
DoubleBlock.Builder builder2 = blockFactory.newDoubleBlockBuilder(grow ? 0 : positions);
238+
ConstantNullBlock.Builder builder3 = new ConstantNullBlock.Builder(blockFactory);
237239
for (int p = 0; p < positions; p++) {
238240
builder1.appendNull();
239241
builder2.appendNull();
242+
builder3.appendNull();
240243
}
241244
DoubleBlock block1 = builder1.build();
242245
DoubleBlock block2 = builder2.build();
246+
Block block3 = builder3.build();
243247
assertEquals(positions, block1.getPositionCount());
244248
assertTrue(block1.mayHaveNulls());
245249
assertTrue(block1.isNull(0));
246250

247-
List<DoubleBlock> blocks = List.of(block1, block2);
251+
List<Block> blocks = List.of(block1, block2, block3);
248252
assertAllEquals(blocks);
249253
}
250254

x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/data/FloatBlockEqualityTests.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ public void testEmptyBlock() {
5151
blockFactory.newConstantFloatBlockWith(0, 0),
5252
blockFactory.newFloatBlockBuilder(0).build(),
5353
blockFactory.newFloatBlockBuilder(0).appendFloat(1).build().filter(),
54-
blockFactory.newFloatBlockBuilder(0).appendNull().build().filter()
54+
blockFactory.newFloatBlockBuilder(0).appendNull().build().filter(),
55+
(ConstantNullBlock) blockFactory.newConstantNullBlock(0)
5556
);
5657
assertAllEquals(blocks);
5758
Releasables.close(blocks);
@@ -234,17 +235,20 @@ public void testSimpleBlockWithManyNulls() {
234235
boolean grow = randomBoolean();
235236
FloatBlock.Builder builder1 = blockFactory.newFloatBlockBuilder(grow ? 0 : positions);
236237
FloatBlock.Builder builder2 = blockFactory.newFloatBlockBuilder(grow ? 0 : positions);
238+
ConstantNullBlock.Builder builder3 = new ConstantNullBlock.Builder(blockFactory);
237239
for (int p = 0; p < positions; p++) {
238240
builder1.appendNull();
239241
builder2.appendNull();
242+
builder3.appendNull();
240243
}
241244
FloatBlock block1 = builder1.build();
242245
FloatBlock block2 = builder2.build();
246+
Block block3 = builder3.build();
243247
assertEquals(positions, block1.getPositionCount());
244248
assertTrue(block1.mayHaveNulls());
245249
assertTrue(block1.isNull(0));
246250

247-
List<FloatBlock> blocks = List.of(block1, block2);
251+
List<Block> blocks = List.of(block1, block2, block3);
248252
assertAllEquals(blocks);
249253
}
250254

x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/data/IntBlockEqualityTests.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,8 @@ public void testEmptyBlock() {
5050
blockFactory.newConstantIntBlockWith(0, 0),
5151
blockFactory.newIntBlockBuilder(0).build(),
5252
blockFactory.newIntBlockBuilder(0).appendInt(1).build().filter(),
53-
blockFactory.newIntBlockBuilder(0).appendNull().build().filter()
53+
blockFactory.newIntBlockBuilder(0).appendNull().build().filter(),
54+
(ConstantNullBlock) blockFactory.newConstantNullBlock(0)
5455
);
5556
assertAllEquals(blocks);
5657
}
@@ -203,17 +204,20 @@ public void testSimpleBlockWithManyNulls() {
203204
boolean grow = randomBoolean();
204205
IntBlock.Builder builder1 = blockFactory.newIntBlockBuilder(grow ? 0 : positions);
205206
IntBlock.Builder builder2 = blockFactory.newIntBlockBuilder(grow ? 0 : positions);
207+
ConstantNullBlock.Builder builder3 = new ConstantNullBlock.Builder(blockFactory);
206208
for (int p = 0; p < positions; p++) {
207209
builder1.appendNull();
208210
builder2.appendNull();
211+
builder3.appendNull();
209212
}
210213
IntBlock block1 = builder1.build();
211214
IntBlock block2 = builder2.build();
215+
Block block3 = builder3.build();
212216
assertEquals(positions, block1.getPositionCount());
213217
assertTrue(block1.mayHaveNulls());
214218
assertTrue(block1.isNull(0));
215219

216-
List<IntBlock> blocks = List.of(block1, block2);
220+
List<Block> blocks = List.of(block1, block2, block3);
217221
assertAllEquals(blocks);
218222
}
219223

x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/data/LongBlockEqualityTests.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,8 @@ public void testEmptyBlock() {
5050
blockFactory.newConstantLongBlockWith(0, 0),
5151
blockFactory.newLongBlockBuilder(0).build(),
5252
blockFactory.newLongBlockBuilder(0).appendLong(1).build().filter(),
53-
blockFactory.newLongBlockBuilder(0).appendNull().build().filter()
53+
blockFactory.newLongBlockBuilder(0).appendNull().build().filter(),
54+
(ConstantNullBlock) blockFactory.newConstantNullBlock(0)
5455
);
5556
assertAllEquals(blocks);
5657
}
@@ -201,17 +202,20 @@ public void testSimpleBlockWithManyNulls() {
201202
boolean grow = randomBoolean();
202203
LongBlock.Builder builder1 = blockFactory.newLongBlockBuilder(grow ? 0 : positions);
203204
LongBlock.Builder builder2 = blockFactory.newLongBlockBuilder(grow ? 0 : positions);
205+
ConstantNullBlock.Builder builder3 = new ConstantNullBlock.Builder(blockFactory);
204206
for (int p = 0; p < positions; p++) {
205207
builder1.appendNull();
206208
builder2.appendNull();
209+
builder3.appendNull();
207210
}
208211
LongBlock block1 = builder1.build();
209212
LongBlock block2 = builder2.build();
213+
Block block3 = builder3.build();
210214
assertEquals(positions, block1.getPositionCount());
211215
assertTrue(block1.mayHaveNulls());
212216
assertTrue(block1.isNull(0));
213217

214-
List<LongBlock> blocks = List.of(block1, block2);
218+
List<Block> blocks = List.of(block1, block2, block3);
215219
assertAllEquals(blocks);
216220
}
217221

0 commit comments

Comments
 (0)