Browse Source

[ES|QL] Change equals and hashcode for ConstantNullBlock (#131817) (#131893)

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.
Larisa Motova 2 months ago
parent
commit
c5d372227b

+ 5 - 0
docs/changelog/131817.yaml

@@ -0,0 +1,5 @@
+pr: 131817
+summary: Change equals and hashcode for `ConstantNullBlock`
+area: ES|QL
+type: bug
+issues: []

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

@@ -14,7 +14,6 @@ import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.core.ReleasableIterator;
 import org.elasticsearch.core.ReleasableIterator;
 
 
 import java.io.IOException;
 import java.io.IOException;
-import java.util.Objects;
 
 
 /**
 /**
  * Block implementation representing a constant null value.
  * Block implementation representing a constant null value.
@@ -120,15 +119,28 @@ final class ConstantNullBlock extends AbstractNonThreadSafeRefCounted
 
 
     @Override
     @Override
     public boolean equals(Object obj) {
     public boolean equals(Object obj) {
-        if (obj instanceof ConstantNullBlock that) {
-            return this.getPositionCount() == that.getPositionCount();
+        if (obj instanceof Block that) {
+            return this.getPositionCount() == 0 && that.getPositionCount() == 0
+                || this.getPositionCount() == that.getPositionCount() && that.areAllValuesNull();
+        }
+        if (obj instanceof Vector that) {
+            return this.getPositionCount() == 0 && that.getPositionCount() == 0;
         }
         }
         return false;
         return false;
     }
     }
 
 
     @Override
     @Override
     public int hashCode() {
     public int hashCode() {
-        return Objects.hash(getPositionCount());
+        // The hashcode for ConstantNullBlock is calculated in this way so that
+        // we return the same hashcode for ConstantNullBlock as we would for block
+        // types that ConstantNullBlock implements that contain only null values.
+        // Example: a DoubleBlock with 8 positions that are all null will return
+        // the same hashcode as a ConstantNullBlock with a positionCount of 8.
+        int result = 1;
+        for (int pos = 0; pos < positionCount; pos++) {
+            result = 31 * result - 1;
+        }
+        return result;
     }
     }
 
 
     @Override
     @Override

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

@@ -52,7 +52,8 @@ public class BooleanBlockEqualityTests extends ESTestCase {
             blockFactory.newConstantBooleanBlockWith(randomBoolean(), 0),
             blockFactory.newConstantBooleanBlockWith(randomBoolean(), 0),
             blockFactory.newBooleanBlockBuilder(0).build(),
             blockFactory.newBooleanBlockBuilder(0).build(),
             blockFactory.newBooleanBlockBuilder(0).appendBoolean(randomBoolean()).build().filter(),
             blockFactory.newBooleanBlockBuilder(0).appendBoolean(randomBoolean()).build().filter(),
-            blockFactory.newBooleanBlockBuilder(0).appendNull().build().filter()
+            blockFactory.newBooleanBlockBuilder(0).appendNull().build().filter(),
+            (ConstantNullBlock) blockFactory.newConstantNullBlock(0)
         );
         );
         assertAllEquals(blocks);
         assertAllEquals(blocks);
     }
     }
@@ -252,6 +253,28 @@ public class BooleanBlockEqualityTests extends ESTestCase {
         assertAllNotEquals(notEqualBlocks);
         assertAllNotEquals(notEqualBlocks);
     }
     }
 
 
+    public void testSimpleBlockWithManyNulls() {
+        int positions = randomIntBetween(1, 256);
+        boolean grow = randomBoolean();
+        BooleanBlock.Builder builder1 = blockFactory.newBooleanBlockBuilder(grow ? 0 : positions);
+        BooleanBlock.Builder builder2 = blockFactory.newBooleanBlockBuilder(grow ? 0 : positions);
+        ConstantNullBlock.Builder builder3 = new ConstantNullBlock.Builder(blockFactory);
+        for (int p = 0; p < positions; p++) {
+            builder1.appendNull();
+            builder2.appendNull();
+            builder3.appendNull();
+        }
+        BooleanBlock block1 = builder1.build();
+        BooleanBlock block2 = builder2.build();
+        Block block3 = builder3.build();
+        assertEquals(positions, block1.getPositionCount());
+        assertTrue(block1.mayHaveNulls());
+        assertTrue(block1.isNull(0));
+
+        List<Block> blocks = List.of(block1, block2, block3);
+        assertAllEquals(blocks);
+    }
+
     static void assertAllEquals(List<?> objs) {
     static void assertAllEquals(List<?> objs) {
         for (Object obj1 : objs) {
         for (Object obj1 : objs) {
             for (Object obj2 : objs) {
             for (Object obj2 : objs) {

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

@@ -63,7 +63,8 @@ public class BytesRefBlockEqualityTests extends ComputeTestCase {
                 blockFactory.newConstantBytesRefBlockWith(new BytesRef(), 0),
                 blockFactory.newConstantBytesRefBlockWith(new BytesRef(), 0),
                 blockFactory.newBytesRefBlockBuilder(0).build(),
                 blockFactory.newBytesRefBlockBuilder(0).build(),
                 blockFactory.newBytesRefBlockBuilder(0).appendBytesRef(new BytesRef()).build().filter(),
                 blockFactory.newBytesRefBlockBuilder(0).appendBytesRef(new BytesRef()).build().filter(),
-                blockFactory.newBytesRefBlockBuilder(0).appendNull().build().filter()
+                blockFactory.newBytesRefBlockBuilder(0).appendNull().build().filter(),
+                (ConstantNullBlock) blockFactory.newConstantNullBlock(0)
             );
             );
             assertAllEquals(blocks);
             assertAllEquals(blocks);
         }
         }
@@ -357,17 +358,20 @@ public class BytesRefBlockEqualityTests extends ComputeTestCase {
         boolean grow = randomBoolean();
         boolean grow = randomBoolean();
         BytesRefBlock.Builder builder1 = blockFactory.newBytesRefBlockBuilder(grow ? 0 : positions);
         BytesRefBlock.Builder builder1 = blockFactory.newBytesRefBlockBuilder(grow ? 0 : positions);
         BytesRefBlock.Builder builder2 = blockFactory.newBytesRefBlockBuilder(grow ? 0 : positions);
         BytesRefBlock.Builder builder2 = blockFactory.newBytesRefBlockBuilder(grow ? 0 : positions);
+        ConstantNullBlock.Builder builder3 = new ConstantNullBlock.Builder(blockFactory);
         for (int p = 0; p < positions; p++) {
         for (int p = 0; p < positions; p++) {
             builder1.appendNull();
             builder1.appendNull();
             builder2.appendNull();
             builder2.appendNull();
+            builder3.appendNull();
         }
         }
         BytesRefBlock block1 = builder1.build();
         BytesRefBlock block1 = builder1.build();
         BytesRefBlock block2 = builder2.build();
         BytesRefBlock block2 = builder2.build();
+        Block block3 = builder3.build();
         assertEquals(positions, block1.getPositionCount());
         assertEquals(positions, block1.getPositionCount());
         assertTrue(block1.mayHaveNulls());
         assertTrue(block1.mayHaveNulls());
         assertTrue(block1.isNull(0));
         assertTrue(block1.isNull(0));
 
 
-        List<BytesRefBlock> blocks = List.of(block1, block2);
+        List<Block> blocks = List.of(block1, block2, block3);
         assertAllEquals(blocks);
         assertAllEquals(blocks);
     }
     }
 
 

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

@@ -51,7 +51,8 @@ public class DoubleBlockEqualityTests extends ComputeTestCase {
             blockFactory.newConstantDoubleBlockWith(0, 0),
             blockFactory.newConstantDoubleBlockWith(0, 0),
             blockFactory.newDoubleBlockBuilder(0).build(),
             blockFactory.newDoubleBlockBuilder(0).build(),
             blockFactory.newDoubleBlockBuilder(0).appendDouble(1).build().filter(),
             blockFactory.newDoubleBlockBuilder(0).appendDouble(1).build().filter(),
-            blockFactory.newDoubleBlockBuilder(0).appendNull().build().filter()
+            blockFactory.newDoubleBlockBuilder(0).appendNull().build().filter(),
+            (ConstantNullBlock) blockFactory.newConstantNullBlock(0)
         );
         );
         assertAllEquals(blocks);
         assertAllEquals(blocks);
         Releasables.close(blocks);
         Releasables.close(blocks);
@@ -234,17 +235,20 @@ public class DoubleBlockEqualityTests extends ComputeTestCase {
         boolean grow = randomBoolean();
         boolean grow = randomBoolean();
         DoubleBlock.Builder builder1 = blockFactory.newDoubleBlockBuilder(grow ? 0 : positions);
         DoubleBlock.Builder builder1 = blockFactory.newDoubleBlockBuilder(grow ? 0 : positions);
         DoubleBlock.Builder builder2 = blockFactory.newDoubleBlockBuilder(grow ? 0 : positions);
         DoubleBlock.Builder builder2 = blockFactory.newDoubleBlockBuilder(grow ? 0 : positions);
+        ConstantNullBlock.Builder builder3 = new ConstantNullBlock.Builder(blockFactory);
         for (int p = 0; p < positions; p++) {
         for (int p = 0; p < positions; p++) {
             builder1.appendNull();
             builder1.appendNull();
             builder2.appendNull();
             builder2.appendNull();
+            builder3.appendNull();
         }
         }
         DoubleBlock block1 = builder1.build();
         DoubleBlock block1 = builder1.build();
         DoubleBlock block2 = builder2.build();
         DoubleBlock block2 = builder2.build();
+        Block block3 = builder3.build();
         assertEquals(positions, block1.getPositionCount());
         assertEquals(positions, block1.getPositionCount());
         assertTrue(block1.mayHaveNulls());
         assertTrue(block1.mayHaveNulls());
         assertTrue(block1.isNull(0));
         assertTrue(block1.isNull(0));
 
 
-        List<DoubleBlock> blocks = List.of(block1, block2);
+        List<Block> blocks = List.of(block1, block2, block3);
         assertAllEquals(blocks);
         assertAllEquals(blocks);
     }
     }
 
 

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

@@ -51,7 +51,8 @@ public class FloatBlockEqualityTests extends ComputeTestCase {
             blockFactory.newConstantFloatBlockWith(0, 0),
             blockFactory.newConstantFloatBlockWith(0, 0),
             blockFactory.newFloatBlockBuilder(0).build(),
             blockFactory.newFloatBlockBuilder(0).build(),
             blockFactory.newFloatBlockBuilder(0).appendFloat(1).build().filter(),
             blockFactory.newFloatBlockBuilder(0).appendFloat(1).build().filter(),
-            blockFactory.newFloatBlockBuilder(0).appendNull().build().filter()
+            blockFactory.newFloatBlockBuilder(0).appendNull().build().filter(),
+            (ConstantNullBlock) blockFactory.newConstantNullBlock(0)
         );
         );
         assertAllEquals(blocks);
         assertAllEquals(blocks);
         Releasables.close(blocks);
         Releasables.close(blocks);
@@ -234,17 +235,20 @@ public class FloatBlockEqualityTests extends ComputeTestCase {
         boolean grow = randomBoolean();
         boolean grow = randomBoolean();
         FloatBlock.Builder builder1 = blockFactory.newFloatBlockBuilder(grow ? 0 : positions);
         FloatBlock.Builder builder1 = blockFactory.newFloatBlockBuilder(grow ? 0 : positions);
         FloatBlock.Builder builder2 = blockFactory.newFloatBlockBuilder(grow ? 0 : positions);
         FloatBlock.Builder builder2 = blockFactory.newFloatBlockBuilder(grow ? 0 : positions);
+        ConstantNullBlock.Builder builder3 = new ConstantNullBlock.Builder(blockFactory);
         for (int p = 0; p < positions; p++) {
         for (int p = 0; p < positions; p++) {
             builder1.appendNull();
             builder1.appendNull();
             builder2.appendNull();
             builder2.appendNull();
+            builder3.appendNull();
         }
         }
         FloatBlock block1 = builder1.build();
         FloatBlock block1 = builder1.build();
         FloatBlock block2 = builder2.build();
         FloatBlock block2 = builder2.build();
+        Block block3 = builder3.build();
         assertEquals(positions, block1.getPositionCount());
         assertEquals(positions, block1.getPositionCount());
         assertTrue(block1.mayHaveNulls());
         assertTrue(block1.mayHaveNulls());
         assertTrue(block1.isNull(0));
         assertTrue(block1.isNull(0));
 
 
-        List<FloatBlock> blocks = List.of(block1, block2);
+        List<Block> blocks = List.of(block1, block2, block3);
         assertAllEquals(blocks);
         assertAllEquals(blocks);
     }
     }
 
 

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

@@ -50,7 +50,8 @@ public class IntBlockEqualityTests extends ComputeTestCase {
             blockFactory.newConstantIntBlockWith(0, 0),
             blockFactory.newConstantIntBlockWith(0, 0),
             blockFactory.newIntBlockBuilder(0).build(),
             blockFactory.newIntBlockBuilder(0).build(),
             blockFactory.newIntBlockBuilder(0).appendInt(1).build().filter(),
             blockFactory.newIntBlockBuilder(0).appendInt(1).build().filter(),
-            blockFactory.newIntBlockBuilder(0).appendNull().build().filter()
+            blockFactory.newIntBlockBuilder(0).appendNull().build().filter(),
+            (ConstantNullBlock) blockFactory.newConstantNullBlock(0)
         );
         );
         assertAllEquals(blocks);
         assertAllEquals(blocks);
     }
     }
@@ -203,17 +204,20 @@ public class IntBlockEqualityTests extends ComputeTestCase {
         boolean grow = randomBoolean();
         boolean grow = randomBoolean();
         IntBlock.Builder builder1 = blockFactory.newIntBlockBuilder(grow ? 0 : positions);
         IntBlock.Builder builder1 = blockFactory.newIntBlockBuilder(grow ? 0 : positions);
         IntBlock.Builder builder2 = blockFactory.newIntBlockBuilder(grow ? 0 : positions);
         IntBlock.Builder builder2 = blockFactory.newIntBlockBuilder(grow ? 0 : positions);
+        ConstantNullBlock.Builder builder3 = new ConstantNullBlock.Builder(blockFactory);
         for (int p = 0; p < positions; p++) {
         for (int p = 0; p < positions; p++) {
             builder1.appendNull();
             builder1.appendNull();
             builder2.appendNull();
             builder2.appendNull();
+            builder3.appendNull();
         }
         }
         IntBlock block1 = builder1.build();
         IntBlock block1 = builder1.build();
         IntBlock block2 = builder2.build();
         IntBlock block2 = builder2.build();
+        Block block3 = builder3.build();
         assertEquals(positions, block1.getPositionCount());
         assertEquals(positions, block1.getPositionCount());
         assertTrue(block1.mayHaveNulls());
         assertTrue(block1.mayHaveNulls());
         assertTrue(block1.isNull(0));
         assertTrue(block1.isNull(0));
 
 
-        List<IntBlock> blocks = List.of(block1, block2);
+        List<Block> blocks = List.of(block1, block2, block3);
         assertAllEquals(blocks);
         assertAllEquals(blocks);
     }
     }
 
 

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

@@ -50,7 +50,8 @@ public class LongBlockEqualityTests extends ComputeTestCase {
             blockFactory.newConstantLongBlockWith(0, 0),
             blockFactory.newConstantLongBlockWith(0, 0),
             blockFactory.newLongBlockBuilder(0).build(),
             blockFactory.newLongBlockBuilder(0).build(),
             blockFactory.newLongBlockBuilder(0).appendLong(1).build().filter(),
             blockFactory.newLongBlockBuilder(0).appendLong(1).build().filter(),
-            blockFactory.newLongBlockBuilder(0).appendNull().build().filter()
+            blockFactory.newLongBlockBuilder(0).appendNull().build().filter(),
+            (ConstantNullBlock) blockFactory.newConstantNullBlock(0)
         );
         );
         assertAllEquals(blocks);
         assertAllEquals(blocks);
     }
     }
@@ -201,17 +202,20 @@ public class LongBlockEqualityTests extends ComputeTestCase {
         boolean grow = randomBoolean();
         boolean grow = randomBoolean();
         LongBlock.Builder builder1 = blockFactory.newLongBlockBuilder(grow ? 0 : positions);
         LongBlock.Builder builder1 = blockFactory.newLongBlockBuilder(grow ? 0 : positions);
         LongBlock.Builder builder2 = blockFactory.newLongBlockBuilder(grow ? 0 : positions);
         LongBlock.Builder builder2 = blockFactory.newLongBlockBuilder(grow ? 0 : positions);
+        ConstantNullBlock.Builder builder3 = new ConstantNullBlock.Builder(blockFactory);
         for (int p = 0; p < positions; p++) {
         for (int p = 0; p < positions; p++) {
             builder1.appendNull();
             builder1.appendNull();
             builder2.appendNull();
             builder2.appendNull();
+            builder3.appendNull();
         }
         }
         LongBlock block1 = builder1.build();
         LongBlock block1 = builder1.build();
         LongBlock block2 = builder2.build();
         LongBlock block2 = builder2.build();
+        Block block3 = builder3.build();
         assertEquals(positions, block1.getPositionCount());
         assertEquals(positions, block1.getPositionCount());
         assertTrue(block1.mayHaveNulls());
         assertTrue(block1.mayHaveNulls());
         assertTrue(block1.isNull(0));
         assertTrue(block1.isNull(0));
 
 
-        List<LongBlock> blocks = List.of(block1, block2);
+        List<Block> blocks = List.of(block1, block2, block3);
         assertAllEquals(blocks);
         assertAllEquals(blocks);
     }
     }