Cleanup and enable more tests

JonasKunz · JonasKunz · commit ee1ebde69fd2 · 2025-09-05T10:53:01.000+02:00
diff --git a/docs/reference/query-languages/esql/images/functions/knn.svg b/docs/reference/query-languages/esql/images/functions/knn.svg
diff --git a/x-pack/plugin/esql/compute/src/main/java/module-info.java b/x-pack/plugin/esql/compute/src/main/java/module-info.java
@@ -21,7 +21,6 @@
     requires org.elasticsearch.geo;
     requires org.elasticsearch.xcore;
     requires hppc;
-    requires commons.math3;
     requires org.elasticsearch.exponentialhistogram;
 
     exports org.elasticsearch.compute;
diff --git a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramArrayBlock.java b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramArrayBlock.java
@@ -14,6 +14,7 @@
 import org.elasticsearch.core.Releasables;
 import org.elasticsearch.exponentialhistogram.AbstractExponentialHistogram;
 import org.elasticsearch.exponentialhistogram.ExponentialHistogram;
+import org.elasticsearch.exponentialhistogram.ExponentialHistogramCircuitBreaker;
 import org.elasticsearch.exponentialhistogram.ZeroBucket;
 
 import java.io.IOException;
@@ -22,7 +23,7 @@
 
 public final class ExponentialHistogramArrayBlock extends AbstractNonThreadSafeRefCounted implements ExponentialHistogramBlock {
 
-    // TODO: Add AggregateMetricDoubleBlock for min/max/sum/count
+    // TODO(b/133393): Add blocks for min/max/sum/count
     private final BytesRefBlock encodedHistograms;
     private boolean isClosed = false;
 
@@ -128,7 +129,6 @@ public ReleasableIterator<? extends Block> lookup(IntBlock positions, ByteSizeVa
 
     @Override
     public MvOrdering mvOrdering() {
-        // TODO: is this correct? histograms don't have a natural ordering but might be ordered by e.g. their maximum value?
         return MvOrdering.UNORDERED;
     }
 
@@ -169,9 +169,32 @@ void copyInto(BytesRefBlock.Builder encodedHistogramsBuilder, int beginInclusive
         encodedHistogramsBuilder.copyFrom(this.encodedHistograms, beginInclusive, endExclusive);
     }
 
+    @Override
+    public boolean equals(Object o) {
+        if (o instanceof ExponentialHistogramBlock block) {
+            return ExponentialHistogramBlock.equals(this, block);
+        }
+        return false;
+    }
+
+    boolean equalsAfterTypeCheck(ExponentialHistogramArrayBlock that) {
+        return this.encodedHistograms.equals(that.encodedHistograms);
+    }
+
+    @Override
+    public int hashCode() {
+        if (encodedHistograms.areAllValuesNull()) {
+            // ensure consistent hash code for all-null blocks
+            try (Block nullBlock = blockFactory().newConstantNullBlock(getPositionCount())) {
+                return nullBlock.hashCode();
+            }
+        }
+        return encodedHistograms.hashCode();
+    }
+
     private class BlockBackedHistogram extends AbstractExponentialHistogram implements ExponentialHistogram {
 
-        // TODO: encode all of the ExponentialHistogram data except for min/max/sum/count
+        // TODO(b/133393): encode all of the ExponentialHistogram data except for min/max/sum/count
         private static final int SCALE_OFFSET = 0;
 
         private final ByteBuffer data;
@@ -199,6 +222,7 @@ private static void resizeIfRequired(BytesRef growableBuffer, int totalSize) {
             if (growableBuffer.length >= totalSize) {
                 return;
             }
+            growableBuffer.length = Math.max(32, growableBuffer.length);
             while (growableBuffer.length < totalSize) {
                 growableBuffer.length *= 2;
             }
@@ -220,43 +244,43 @@ private void checkForUseAfterClose() {
 
         @Override
         public ZeroBucket zeroBucket() {
-            // TODO: implement
+            // TODO(b/133393): implement
             return ZeroBucket.minimalEmpty();
         }
 
         @Override
         public ExponentialHistogram.Buckets positiveBuckets() {
-            // TODO: implement
-            return ExponentialHistogram.empty().positiveBuckets();
+            // TODO(b/133393): implement
+            return ExponentialHistogram.builder(scale(), ExponentialHistogramCircuitBreaker.noop()).build().positiveBuckets();
         }
 
         @Override
         public ExponentialHistogram.Buckets negativeBuckets() {
-            // TODO: implement
-            return ExponentialHistogram.empty().negativeBuckets();
+            // TODO(b/133393): implement
+            return ExponentialHistogram.builder(scale(), ExponentialHistogramCircuitBreaker.noop()).build().negativeBuckets();
         }
 
         @Override
         public double sum() {
-            // TODO: implement
+            // TODO(b/133393): implement
             return 0;
         }
 
         @Override
         public long valueCount() {
-            // TODO: implement
+            // TODO(b/133393): implement
             return 0;
         }
 
         @Override
         public double min() {
-            // TODO: implement
+            // TODO(b/133393): implement
             return Double.NaN;
         }
 
         @Override
         public double max() {
-            // TODO: implement
+            // TODO(b/133393): implement
             return Double.NaN;
         }
 
@@ -266,27 +290,4 @@ public long ramBytesUsed() {
         }
 
     }
-
-    @Override
-    public boolean equals(Object o) {
-        if (o instanceof ExponentialHistogramBlock block) {
-            return ExponentialHistogramBlock.equals(this, block);
-        }
-        return false;
-    }
-
-    boolean equalsAfterTypeCheck(ExponentialHistogramArrayBlock that) {
-        return this.encodedHistograms.equals(that.encodedHistograms);
-    }
-
-    @Override
-    public int hashCode() {
-        if (encodedHistograms.areAllValuesNull()) {
-            // ensure consistent hash code for all-null blocks
-            try (Block nullBlock = blockFactory().newConstantNullBlock(getPositionCount())) {
-                return nullBlock.hashCode();
-            }
-        }
-        return encodedHistograms.hashCode();
-    }
 }
diff --git a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramBlock.java b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramBlock.java
@@ -13,16 +13,6 @@ public sealed interface ExponentialHistogramBlock extends Block permits Constant
 
     ExponentialHistogram getExponentialHistogram(int valueIndex);
 
-    /**
-     * Compares the given object with this block for equality. Returns {@code true} if and only if the
-     * given object is a BytesRefBlock, and both blocks are {@link #equals(ExponentialHistogramBlock, ExponentialHistogramBlock) equal}.
-     */
-    @Override
-    boolean equals(Object obj);
-
-    @Override
-    int hashCode();
-
     static boolean equals(ExponentialHistogramBlock blockA, ExponentialHistogramBlock blockB) {
         if (blockA == blockB) {
             return true;
diff --git a/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramBlockBuilder.java b/x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/ExponentialHistogramBlockBuilder.java
@@ -72,7 +72,6 @@ public ExponentialHistogramBlockBuilder copyFrom(Block block, int beginInclusive
 
     @Override
     public ExponentialHistogramBlockBuilder mvOrdering(Block.MvOrdering mvOrdering) {
-        // TODO: does this need implementation?
         return this;
     }
 
diff --git a/x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/data/BlockMultiValuedTests.java b/x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/data/BlockMultiValuedTests.java
@@ -49,7 +49,7 @@ public static List<Object[]> params() {
                 || e == ElementType.NULL
                 || e == ElementType.DOC
                 || e == ElementType.COMPOSITE
-                || e == ElementType.EXPONENTIAL_HISTOGRAM // TODO: implement
+                || e == ElementType.EXPONENTIAL_HISTOGRAM //TODO(b/133393): Enable tests once the block supports lookup
                 || e == ElementType.AGGREGATE_METRIC_DOUBLE) {
                 continue;
             }
diff --git a/x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/data/CompositeBlockTests.java b/x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/data/CompositeBlockTests.java
@@ -21,11 +21,7 @@ public class CompositeBlockTests extends ComputeTestCase {
 
     static List<ElementType> supportedSubElementTypes = Arrays.stream(ElementType.values())
         .filter(
-            e -> e != ElementType.COMPOSITE
-                && e != ElementType.UNKNOWN
-                && e != ElementType.DOC
-                && e != ElementType.AGGREGATE_METRIC_DOUBLE
-                && e != ElementType.EXPONENTIAL_HISTOGRAM
+            e -> e != ElementType.COMPOSITE && e != ElementType.UNKNOWN && e != ElementType.DOC && e != ElementType.AGGREGATE_METRIC_DOUBLE
         )
         .toList();
 
diff --git a/x-pack/plugin/esql/compute/test/src/main/java/org/elasticsearch/compute/test/BlockTestUtils.java b/x-pack/plugin/esql/compute/test/src/main/java/org/elasticsearch/compute/test/BlockTestUtils.java
@@ -28,6 +28,7 @@
 import org.elasticsearch.compute.data.Page;
 import org.elasticsearch.core.Releasables;
 import org.elasticsearch.exponentialhistogram.ExponentialHistogram;
+import org.elasticsearch.exponentialhistogram.ExponentialHistogramCircuitBreaker;
 import org.hamcrest.Matcher;
 
 import java.util.ArrayList;
@@ -371,8 +372,9 @@ public static Page convertBytesRefsToOrdinals(Page page) {
     }
 
     static ExponentialHistogram randomExponentialHistogram() {
-        // TODO: implement with actualy randomization and non-empty histograms
-        return ExponentialHistogram.empty();
+        // TODO(b/133393): populate the random histogram fully when supported by the block
+        int scale = randomIntBetween(ExponentialHistogram.MIN_SCALE, ExponentialHistogram.MAX_SCALE);
+        return ExponentialHistogram.builder(scale, ExponentialHistogramCircuitBreaker.noop()).build();
     }
 
     private static int dedupe(Map<BytesRef, Integer> dedupe, BytesRefVector.Builder bytes, BytesRef v) {
diff --git a/x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/RestEsqlIT.java b/x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/RestEsqlIT.java
@@ -669,7 +669,7 @@ public void testSuggestedCast() throws IOException {
         shouldBeSupported.remove(DataType.DOC_DATA_TYPE);
         shouldBeSupported.remove(DataType.TSID_DATA_TYPE);
         shouldBeSupported.remove(DataType.DENSE_VECTOR);
-        shouldBeSupported.remove(DataType.EXPONENTIAL_HISTOGRAM); // TODO: add support when blockloader is implemented
+        shouldBeSupported.remove(DataType.EXPONENTIAL_HISTOGRAM); //TODO(b/133393): add support when blockloader is implemented
         if (EsqlCorePlugin.AGGREGATE_METRIC_DOUBLE_FEATURE_FLAG.isEnabled() == false) {
             shouldBeSupported.remove(DataType.AGGREGATE_METRIC_DOUBLE);
         }
diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java
@@ -38,6 +38,7 @@
 import org.elasticsearch.core.SuppressForbidden;
 import org.elasticsearch.core.Tuple;
 import org.elasticsearch.exponentialhistogram.ExponentialHistogram;
+import org.elasticsearch.exponentialhistogram.ExponentialHistogramCircuitBreaker;
 import org.elasticsearch.geo.GeometryTestUtils;
 import org.elasticsearch.geo.ShapeTestUtils;
 import org.elasticsearch.geometry.utils.Geohash;
@@ -887,17 +888,17 @@ public static Literal randomLiteral(DataType type) {
                 }
             }
             case DENSE_VECTOR -> Arrays.asList(randomArray(10, 10, i -> new Float[10], ESTestCase::randomFloat));
-            case EXPONENTIAL_HISTOGRAM -> new WriteableExponentialHistogram(randomExponentialHistogram());
-            // TODO: implement
+            case EXPONENTIAL_HISTOGRAM -> new WriteableExponentialHistogram(EsqlTestUtils.randomExponentialHistogram());
             case UNSUPPORTED, OBJECT, DOC_DATA_TYPE, TSID_DATA_TYPE, PARTIAL_AGG -> throw new IllegalArgumentException(
                 "can't make random values for [" + type.typeName() + "]"
             );
         }, type);
     }
 
     private static ExponentialHistogram randomExponentialHistogram() {
-        // TODO: implement properly with an actual histogram from random data
-        return ExponentialHistogram.empty();
+        // TODO(b/133393): populate the random histogram fully when supported by the block
+        int scale = randomIntBetween(ExponentialHistogram.MIN_SCALE, ExponentialHistogram.MAX_SCALE);
+        return ExponentialHistogram.builder(scale, ExponentialHistogramCircuitBreaker.noop()).build();
     }
 
     static Version randomVersion() {
diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/WriteableExponentialHistogram.java b/x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/WriteableExponentialHistogram.java
@@ -22,7 +22,7 @@
 import java.io.IOException;
 
 /**
- * A wrapper around ExponentialHistogram to make it writeable to @link StreamOutput},
+ * A wrapper around ExponentialHistogram to make it writeable to @link StreamOutput}
  * so that it can be used e.g. in {@link org.elasticsearch.xpack.esql.core.expression.Literal}s.
  * Only intended for testing purposes.
  */
@@ -139,11 +139,11 @@ private static WriteableExponentialHistogram readFrom(StreamInput in) throws IOE
         builder.zeroBucket(ZeroBucket.create(in.readDouble(), in.readLong()));
         int negBucketCount = in.readInt();
         for (int i = 0; i < negBucketCount; i++) {
-            builder.addNegativeBucket(in.readLong(), in.readLong());
+            builder.setNegativeBucket(in.readLong(), in.readLong());
         }
         int posBucketCount = in.readInt();
         for (int i = 0; i < posBucketCount; i++) {
-            builder.addPositiveBucket(in.readLong(), in.readLong());
+            builder.setPositiveBucket(in.readLong(), in.readLong());
         }
         return new WriteableExponentialHistogram(builder.build());
     }
diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/nulls/Coalesce.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/nulls/Coalesce.java
@@ -211,6 +211,7 @@ public ExpressionEvaluator.Factory toEvaluator(ToEvaluator toEvaluator) {
             case UNSUPPORTED, SHORT, BYTE, DATE_PERIOD, OBJECT, DOC_DATA_TYPE, SOURCE, TIME_DURATION, FLOAT, HALF_FLOAT, TSID_DATA_TYPE,
                 SCALED_FLOAT, PARTIAL_AGG, AGGREGATE_METRIC_DOUBLE, EXPONENTIAL_HISTOGRAM, DENSE_VECTOR ->
                 throw new UnsupportedOperationException(dataType() + " can’t be coalesced");
+                //TODO(b/133393): Implement coalesce for exponential histograms
         };
     }
 }
diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryResponseTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryResponseTests.java
@@ -290,8 +290,8 @@ private Page randomPage(List<ColumnInfoImpl> columns) {
                 }
                 case EXPONENTIAL_HISTOGRAM -> {
                     ExponentialHistogramBlockBuilder expBuilder = (ExponentialHistogramBlockBuilder) builder;
-                    int valueCount = randomIntBetween(1, 100);
-                    int bucketCount = randomIntBetween(2, Math.max(2, valueCount));
+                    int valueCount = randomIntBetween(0, 500);
+                    int bucketCount = randomIntBetween(4, Math.max(4, valueCount));
                     ExponentialHistogram histo = ExponentialHistogram.create(
                         bucketCount,
                         ExponentialHistogramCircuitBreaker.noop(),
diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseTests.java
@@ -65,7 +65,7 @@ public class CaseTests extends AbstractScalarFunctionTestCase {
             t.addAll(
                 DataType.UNDER_CONSTRUCTION.keySet()
                     .stream()
-                    .filter(type -> type != DataType.EXPONENTIAL_HISTOGRAM) // TODO: implement
+                    .filter(type -> type != DataType.EXPONENTIAL_HISTOGRAM) // TODO(b/133393): implement
                     .filter(type -> type != DataType.AGGREGATE_METRIC_DOUBLE && type != DataType.DENSE_VECTOR)
                     .toList()
             );

Original file line number	Diff line number	Diff line change
`@@ -72,7 +72,6 @@ public ExponentialHistogramBlockBuilder copyFrom(Block block, int beginInclusive`
`72`	`72`
`73`	`73`	`@Override`
`74`	`74`	`public ExponentialHistogramBlockBuilder mvOrdering(Block.MvOrdering mvOrdering) {`
`75`		`- // TODO: does this need implementation?`
`76`	`75`	`return this;`
`77`	`76`	`}`
`78`	`77`
Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,7 @@ public static List<Object[]> params() {`
`49`	`49`	`\|\| e == ElementType.NULL`
`50`	`50`	`\|\| e == ElementType.DOC`
`51`	`51`	`\|\| e == ElementType.COMPOSITE`
`52`		`- \|\| e == ElementType.EXPONENTIAL_HISTOGRAM // TODO: implement`
	`52`	`+ \|\| e == ElementType.EXPONENTIAL_HISTOGRAM //TODO(b/133393): Enable tests once the block supports lookup`
`53`	`53`	`\|\| e == ElementType.AGGREGATE_METRIC_DOUBLE) {`
`54`	`54`	`continue;`
`55`	`55`	`}`
Original file line number	Diff line number	Diff line change
`@@ -669,7 +669,7 @@ public void testSuggestedCast() throws IOException {`
`669`	`669`	`shouldBeSupported.remove(DataType.DOC_DATA_TYPE);`
`670`	`670`	`shouldBeSupported.remove(DataType.TSID_DATA_TYPE);`
`671`	`671`	`shouldBeSupported.remove(DataType.DENSE_VECTOR);`
`672`		`- shouldBeSupported.remove(DataType.EXPONENTIAL_HISTOGRAM); // TODO: add support when blockloader is implemented`
	`672`	`+ shouldBeSupported.remove(DataType.EXPONENTIAL_HISTOGRAM); //TODO(b/133393): add support when blockloader is implemented`
`673`	`673`	`if (EsqlCorePlugin.AGGREGATE_METRIC_DOUBLE_FEATURE_FLAG.isEnabled() == false) {`
`674`	`674`	`shouldBeSupported.remove(DataType.AGGREGATE_METRIC_DOUBLE);`
`675`	`675`	`}`