From 7cf6fc7183df09995d98e92f7f0e17d44c6398f2 Mon Sep 17 00:00:00 2001 From: Ketan Verma Date: Tue, 31 Oct 2023 13:48:29 +0530 Subject: [PATCH 1/4] Refactor common parts from the Rounding class into a separate 'round' package Signed-off-by: Ketan Verma --- CHANGELOG.md | 3 +- .../RoundableBenchmark.java} | 16 +- .../round/BidirectionalLinearSearcher.java | 57 +++++++ .../common/round/BinarySearcher.java | 41 +++++ .../opensearch/common/round/Roundable.java | 28 ++++ .../common/round/RoundableFactory.java | 34 ++++ .../opensearch/common/round/package-info.java | 12 ++ .../java/org/opensearch/common/Rounding.java | 148 ++++-------------- .../org/opensearch/common/RoundingTests.java | 22 --- .../common/round/RoundableTests.java | 57 +++++++ 10 files changed, 271 insertions(+), 147 deletions(-) rename benchmarks/src/main/java/org/opensearch/common/{ArrayRoundingBenchmark.java => round/RoundableBenchmark.java} (89%) create mode 100644 libs/common/src/main/java/org/opensearch/common/round/BidirectionalLinearSearcher.java create mode 100644 libs/common/src/main/java/org/opensearch/common/round/BinarySearcher.java create mode 100644 libs/common/src/main/java/org/opensearch/common/round/Roundable.java create mode 100644 libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java create mode 100644 libs/common/src/main/java/org/opensearch/common/round/package-info.java create mode 100644 server/src/test/java/org/opensearch/common/round/RoundableTests.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 046693421d07b..d95ffef88a61f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -125,6 +125,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add telemetry tracer/metric enable flag and integ test. ([#10395](https://github.com/opensearch-project/OpenSearch/pull/10395)) - Add instrumentation for indexing in transport bulk action and transport shard bulk action. ([#10273](https://github.com/opensearch-project/OpenSearch/pull/10273)) - [BUG] Disable sort optimization for HALF_FLOAT ([#10999](https://github.com/opensearch-project/OpenSearch/pull/10999)) +- Refactor common parts from the Rounding class into a separate 'round' package ([#11023](https://github.com/opensearch-project/OpenSearch/issues/11023)) ### Deprecated @@ -141,4 +142,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Security [Unreleased 3.0]: https://github.com/opensearch-project/OpenSearch/compare/2.x...HEAD -[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.12...2.x \ No newline at end of file +[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.12...2.x diff --git a/benchmarks/src/main/java/org/opensearch/common/ArrayRoundingBenchmark.java b/benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java similarity index 89% rename from benchmarks/src/main/java/org/opensearch/common/ArrayRoundingBenchmark.java rename to benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java index 64c0a9e1d7aa6..4e07af452968b 100644 --- a/benchmarks/src/main/java/org/opensearch/common/ArrayRoundingBenchmark.java +++ b/benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java @@ -6,7 +6,7 @@ * compatible open source license. */ -package org.opensearch.common; +package org.opensearch.common.round; import org.openjdk.jmh.annotations.Benchmark; import org.openjdk.jmh.annotations.BenchmarkMode; @@ -27,13 +27,13 @@ @Warmup(iterations = 3, time = 1) @Measurement(iterations = 1, time = 1) @BenchmarkMode(Mode.Throughput) -public class ArrayRoundingBenchmark { +public class RoundableBenchmark { @Benchmark - public void round(Blackhole bh, Options opts) { - Rounding.Prepared rounding = opts.supplier.get(); + public void floor(Blackhole bh, Options opts) { + Roundable roundable = opts.supplier.get(); for (long key : opts.queries) { - bh.consume(rounding.round(key)); + bh.consume(roundable.floor(key)); } } @@ -90,7 +90,7 @@ public static class Options { public String distribution; public long[] queries; - public Supplier supplier; + public Supplier supplier; @Setup public void setup() { @@ -130,10 +130,10 @@ public void setup() { switch (type) { case "binary": - supplier = () -> new Rounding.BinarySearchArrayRounding(values, size, null); + supplier = () -> new BinarySearcher(values, size); break; case "linear": - supplier = () -> new Rounding.BidirectionalLinearSearchArrayRounding(values, size, null); + supplier = () -> new BidirectionalLinearSearcher(values, size); break; default: throw new IllegalArgumentException("invalid type: " + type); diff --git a/libs/common/src/main/java/org/opensearch/common/round/BidirectionalLinearSearcher.java b/libs/common/src/main/java/org/opensearch/common/round/BidirectionalLinearSearcher.java new file mode 100644 index 0000000000000..4aee99aa1743b --- /dev/null +++ b/libs/common/src/main/java/org/opensearch/common/round/BidirectionalLinearSearcher.java @@ -0,0 +1,57 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.round; + +import org.opensearch.common.annotation.InternalApi; + +/** + * It uses linear search on a sorted array of pre-computed round-down points. + * For small inputs (≤ 64 elements), this can be much faster than binary search as it avoids the penalty of + * branch mispredictions and pipeline stalls, and accesses memory sequentially. + * + *

+ * It uses "meet in the middle" linear search to avoid the worst case scenario when the desired element is present + * at either side of the array. This is helpful for time-series data where velocity increases over time, so more + * documents are likely to find a greater timestamp which is likely to be present on the right end of the array. + * + * @opensearch.internal + */ +@InternalApi +class BidirectionalLinearSearcher implements Roundable { + private final long[] ascending; + private final long[] descending; + + public BidirectionalLinearSearcher(long[] values, int size) { + assert size > 0 : "at least one value must be present"; + + int len = (size + 1) >>> 1; // rounded-up to handle odd number of values + ascending = new long[len]; + descending = new long[len]; + + for (int i = 0; i < len; i++) { + ascending[i] = values[i]; + descending[i] = values[size - i - 1]; + } + } + + @Override + public long floor(long key) { + int i = 0; + for (; i < ascending.length; i++) { + if (descending[i] <= key) { + return descending[i]; + } + if (ascending[i] > key) { + assert i > 0 : "key must be greater than or equal to " + ascending[0]; + return ascending[i - 1]; + } + } + return ascending[i - 1]; + } +} diff --git a/libs/common/src/main/java/org/opensearch/common/round/BinarySearcher.java b/libs/common/src/main/java/org/opensearch/common/round/BinarySearcher.java new file mode 100644 index 0000000000000..38d4b24a3aeaf --- /dev/null +++ b/libs/common/src/main/java/org/opensearch/common/round/BinarySearcher.java @@ -0,0 +1,41 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.round; + +import org.opensearch.common.annotation.InternalApi; + +import java.util.Arrays; + +/** + * It uses binary search on a sorted array of pre-computed round-down points. + * + * @opensearch.internal + */ +@InternalApi +class BinarySearcher implements Roundable { + private final long[] values; + private final int size; + + public BinarySearcher(long[] values, int size) { + assert size > 0 : "at least one value must be present"; + + this.values = values; + this.size = size; + } + + @Override + public long floor(long key) { + int idx = Arrays.binarySearch(values, 0, size, key); + assert idx != -1 : "key must be greater than or equal to " + values[0]; + if (idx < 0) { + idx = -2 - idx; + } + return values[idx]; + } +} diff --git a/libs/common/src/main/java/org/opensearch/common/round/Roundable.java b/libs/common/src/main/java/org/opensearch/common/round/Roundable.java new file mode 100644 index 0000000000000..ae6f9b787c1e9 --- /dev/null +++ b/libs/common/src/main/java/org/opensearch/common/round/Roundable.java @@ -0,0 +1,28 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.round; + +import org.opensearch.common.annotation.InternalApi; + +/** + * Interface to round-off values. + * + * @opensearch.internal + */ +@InternalApi +@FunctionalInterface +public interface Roundable { + /** + * Returns the greatest lower bound of the given key. + * In other words, it returns the largest value such that {@code value <= key}. + * @param key to floor + * @return the floored value + */ + long floor(long key); +} diff --git a/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java b/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java new file mode 100644 index 0000000000000..caa23f04a49af --- /dev/null +++ b/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java @@ -0,0 +1,34 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.round; + +/** + * Factory class to create and return the fastest implementation of {@link Roundable}. + */ +public class RoundableFactory { + /** + * The maximum limit up to which linear search is used, otherwise binary search is used. + * This is because linear search is much faster on small arrays. + * Benchmark results: PR #9727 + */ + private static final int LINEAR_SEARCH_MAX_SIZE = 64; + + private RoundableFactory() {} + + /** + * Creates and returns the fastest implementation of {@link Roundable}. + */ + public static Roundable create(long[] values, int size) { + if (size <= LINEAR_SEARCH_MAX_SIZE) { + return new BidirectionalLinearSearcher(values, size); + } else { + return new BinarySearcher(values, size); + } + } +} diff --git a/libs/common/src/main/java/org/opensearch/common/round/package-info.java b/libs/common/src/main/java/org/opensearch/common/round/package-info.java new file mode 100644 index 0000000000000..e79c4017de31b --- /dev/null +++ b/libs/common/src/main/java/org/opensearch/common/round/package-info.java @@ -0,0 +1,12 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +/** + * Contains classes to round-off values. + */ +package org.opensearch.common.round; diff --git a/server/src/main/java/org/opensearch/common/Rounding.java b/server/src/main/java/org/opensearch/common/Rounding.java index 667eb4529fe38..f30e64cdba10d 100644 --- a/server/src/main/java/org/opensearch/common/Rounding.java +++ b/server/src/main/java/org/opensearch/common/Rounding.java @@ -37,7 +37,8 @@ import org.opensearch.OpenSearchException; import org.opensearch.common.LocalTimeOffset.Gap; import org.opensearch.common.LocalTimeOffset.Overlap; -import org.opensearch.common.annotation.InternalApi; +import org.opensearch.common.round.Roundable; +import org.opensearch.common.round.RoundableFactory; import org.opensearch.common.time.DateUtils; import org.opensearch.common.unit.TimeValue; import org.opensearch.core.common.io.stream.StreamInput; @@ -59,7 +60,6 @@ import java.time.temporal.TemporalQueries; import java.time.zone.ZoneOffsetTransition; import java.time.zone.ZoneRules; -import java.util.Arrays; import java.util.List; import java.util.Locale; import java.util.Objects; @@ -421,13 +421,6 @@ private abstract class PreparedRounding implements Prepared { */ private static final int DEFAULT_ARRAY_ROUNDING_MAX_THRESHOLD = 128; - /** - * The maximum limit up to which linear search is used, otherwise binary search is used. - * This is because linear search is much faster on small arrays. - * Benchmark results: PR #9727 - */ - private static final int LINEAR_SEARCH_ARRAY_ROUNDING_MAX_THRESHOLD = 64; - /** * Attempt to build a {@link Prepared} implementation that relies on pre-calcuated * "round down" points. If there would be more than {@code max} points then return @@ -451,9 +444,36 @@ protected Prepared maybeUseArray(long minUtcMillis, long maxUtcMillis, int max) values = ArrayUtil.grow(values, i + 1); values[i++] = rounded; } - return i <= LINEAR_SEARCH_ARRAY_ROUNDING_MAX_THRESHOLD - ? new BidirectionalLinearSearchArrayRounding(values, i, this) - : new BinarySearchArrayRounding(values, i, this); + return new ArrayRounding(values, i, this); + } + } + + /** + * ArrayRounding is an implementation of {@link Prepared} which uses + * pre-calculated round-down points to speed up lookups. + */ + private static class ArrayRounding implements Prepared { + private final Roundable roundable; + private final Prepared delegate; + + public ArrayRounding(long[] values, int size, Prepared delegate) { + this.roundable = RoundableFactory.create(values, size); + this.delegate = delegate; + } + + @Override + public long round(long utcMillis) { + return roundable.floor(utcMillis); + } + + @Override + public long nextRoundingValue(long utcMillis) { + return delegate.nextRoundingValue(utcMillis); + } + + @Override + public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { + return delegate.roundingSize(utcMillis, timeUnit); } } @@ -1343,108 +1363,4 @@ public static Rounding read(StreamInput in) throws IOException { throw new OpenSearchException("unknown rounding id [" + id + "]"); } } - - /** - * Implementation of {@link Prepared} using pre-calculated "round down" points. - * - *

- * It uses binary search to find the greatest round-down point less than or equal to the given timestamp. - * - * @opensearch.internal - */ - @InternalApi - static class BinarySearchArrayRounding implements Prepared { - private final long[] values; - private final int max; - private final Prepared delegate; - - BinarySearchArrayRounding(long[] values, int max, Prepared delegate) { - assert max > 0 : "at least one round-down point must be present"; - this.values = values; - this.max = max; - this.delegate = delegate; - } - - @Override - public long round(long utcMillis) { - assert values[0] <= utcMillis : "utcMillis must be after " + values[0]; - int idx = Arrays.binarySearch(values, 0, max, utcMillis); - assert idx != -1 : "The insertion point is before the array! This should have tripped the assertion above."; - assert -1 - idx <= values.length : "This insertion point is after the end of the array."; - if (idx < 0) { - idx = -2 - idx; - } - return values[idx]; - } - - @Override - public long nextRoundingValue(long utcMillis) { - return delegate.nextRoundingValue(utcMillis); - } - - @Override - public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { - return delegate.roundingSize(utcMillis, timeUnit); - } - } - - /** - * Implementation of {@link Prepared} using pre-calculated "round down" points. - * - *

- * It uses linear search to find the greatest round-down point less than or equal to the given timestamp. - * For small inputs (≤ 64 elements), this can be much faster than binary search as it avoids the penalty of - * branch mispredictions and pipeline stalls, and accesses memory sequentially. - * - *

- * It uses "meet in the middle" linear search to avoid the worst case scenario when the desired element is present - * at either side of the array. This is helpful for time-series data where velocity increases over time, so more - * documents are likely to find a greater timestamp which is likely to be present on the right end of the array. - * - * @opensearch.internal - */ - @InternalApi - static class BidirectionalLinearSearchArrayRounding implements Prepared { - private final long[] ascending; - private final long[] descending; - private final Prepared delegate; - - BidirectionalLinearSearchArrayRounding(long[] values, int max, Prepared delegate) { - assert max > 0 : "at least one round-down point must be present"; - this.delegate = delegate; - int len = (max + 1) >>> 1; // rounded-up to handle odd number of values - ascending = new long[len]; - descending = new long[len]; - - for (int i = 0; i < len; i++) { - ascending[i] = values[i]; - descending[i] = values[max - i - 1]; - } - } - - @Override - public long round(long utcMillis) { - int i = 0; - for (; i < ascending.length; i++) { - if (descending[i] <= utcMillis) { - return descending[i]; - } - if (ascending[i] > utcMillis) { - assert i > 0 : "utcMillis must be after " + ascending[0]; - return ascending[i - 1]; - } - } - return ascending[i - 1]; - } - - @Override - public long nextRoundingValue(long utcMillis) { - return delegate.nextRoundingValue(utcMillis); - } - - @Override - public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { - return delegate.roundingSize(utcMillis, timeUnit); - } - } } diff --git a/server/src/test/java/org/opensearch/common/RoundingTests.java b/server/src/test/java/org/opensearch/common/RoundingTests.java index cc71ee08abcca..9a6e6a6ac54d0 100644 --- a/server/src/test/java/org/opensearch/common/RoundingTests.java +++ b/server/src/test/java/org/opensearch/common/RoundingTests.java @@ -1142,28 +1142,6 @@ public void testNonMillisecondsBasedUnitCalendarRoundingSize() { assertThat(prepared.roundingSize(thirdQuarter, Rounding.DateTimeUnit.HOUR_OF_DAY), closeTo(2208.0, 0.000001)); } - public void testArrayRoundingImplementations() { - int length = randomIntBetween(1, 256); - long[] values = new long[length]; - for (int i = 1; i < values.length; i++) { - values[i] = values[i - 1] + (randomNonNegativeLong() % 100); - } - - Rounding.Prepared binarySearchImpl = new Rounding.BinarySearchArrayRounding(values, length, null); - Rounding.Prepared linearSearchImpl = new Rounding.BidirectionalLinearSearchArrayRounding(values, length, null); - - for (int i = 0; i < 100000; i++) { - long key = values[0] + (randomNonNegativeLong() % (100 + values[length - 1] - values[0])); - assertEquals(binarySearchImpl.round(key), linearSearchImpl.round(key)); - } - - AssertionError exception = expectThrows(AssertionError.class, () -> { binarySearchImpl.round(values[0] - 1); }); - assertEquals("utcMillis must be after " + values[0], exception.getMessage()); - - exception = expectThrows(AssertionError.class, () -> { linearSearchImpl.round(values[0] - 1); }); - assertEquals("utcMillis must be after " + values[0], exception.getMessage()); - } - private void assertInterval(long rounded, long nextRoundingValue, Rounding rounding, int minutes, ZoneId tz) { assertInterval(rounded, dateBetween(rounded, nextRoundingValue), nextRoundingValue, rounding, tz); long millisPerMinute = 60_000; diff --git a/server/src/test/java/org/opensearch/common/round/RoundableTests.java b/server/src/test/java/org/opensearch/common/round/RoundableTests.java new file mode 100644 index 0000000000000..5623dc80d1a16 --- /dev/null +++ b/server/src/test/java/org/opensearch/common/round/RoundableTests.java @@ -0,0 +1,57 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.round; + +import org.opensearch.test.OpenSearchTestCase; + +public class RoundableTests extends OpenSearchTestCase { + + public void testFloor() { + int size = randomIntBetween(1, 256); + long[] values = new long[size]; + for (int i = 1; i < values.length; i++) { + values[i] = values[i - 1] + (randomNonNegativeLong() % 200) + 1; + } + + Roundable[] impls = { new BinarySearcher(values, size), new BidirectionalLinearSearcher(values, size) }; + + for (int i = 0; i < 100000; i++) { + // Index of the expected round-down point. + int idx = randomIntBetween(0, size - 1); + + // Value of the expected round-down point. + long expected = values[idx]; + + // Delta between the expected and the next round-down point. + long delta = (idx < size - 1) ? (values[idx + 1] - values[idx]) : 200; + + // Adding a random delta between 0 (inclusive) and delta (exclusive) to the expected + // round-down point, which will still floor to the same value. + long key = expected + (randomNonNegativeLong() % delta); + + for (Roundable roundable : impls) { + assertEquals(expected, roundable.floor(key)); + } + } + } + + public void testAssertions() { + AssertionError exception; + + exception = assertThrows(AssertionError.class, () -> new BinarySearcher(new long[0], 0)); + assertEquals("at least one value must be present", exception.getMessage()); + exception = assertThrows(AssertionError.class, () -> new BidirectionalLinearSearcher(new long[0], 0)); + assertEquals("at least one value must be present", exception.getMessage()); + + exception = assertThrows(AssertionError.class, () -> new BinarySearcher(new long[] { 100 }, 1).floor(50)); + assertEquals("key must be greater than or equal to 100", exception.getMessage()); + exception = assertThrows(AssertionError.class, () -> new BidirectionalLinearSearcher(new long[] { 100 }, 1).floor(50)); + assertEquals("key must be greater than or equal to 100", exception.getMessage()); + } +} From bf37f5f214ce6c4069f6b33304729e7bb30dfa08 Mon Sep 17 00:00:00 2001 From: Ketan Verma Date: Tue, 31 Oct 2023 21:44:39 +0530 Subject: [PATCH 2/4] Move RoundableTests from :server to :libs:opensearch-common module Signed-off-by: Ketan Verma --- .../src/test/java/org/opensearch/common/round/RoundableTests.java | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename {server => libs/common}/src/test/java/org/opensearch/common/round/RoundableTests.java (100%) diff --git a/server/src/test/java/org/opensearch/common/round/RoundableTests.java b/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java similarity index 100% rename from server/src/test/java/org/opensearch/common/round/RoundableTests.java rename to libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java From 3d4f7adf39bedf6d529fdb3454801ed112221e77 Mon Sep 17 00:00:00 2001 From: Ketan Verma Date: Wed, 1 Nov 2023 00:10:16 +0530 Subject: [PATCH 3/4] Address PR comments Signed-off-by: Ketan Verma --- .../common/round/BidirectionalLinearSearcher.java | 2 +- .../java/org/opensearch/common/round/BinarySearcher.java | 2 +- .../java/org/opensearch/common/round/RoundableFactory.java | 7 ++++++- server/src/main/java/org/opensearch/common/Rounding.java | 6 +++--- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/libs/common/src/main/java/org/opensearch/common/round/BidirectionalLinearSearcher.java b/libs/common/src/main/java/org/opensearch/common/round/BidirectionalLinearSearcher.java index 4aee99aa1743b..6d16b41e6a2de 100644 --- a/libs/common/src/main/java/org/opensearch/common/round/BidirectionalLinearSearcher.java +++ b/libs/common/src/main/java/org/opensearch/common/round/BidirectionalLinearSearcher.java @@ -27,7 +27,7 @@ class BidirectionalLinearSearcher implements Roundable { private final long[] ascending; private final long[] descending; - public BidirectionalLinearSearcher(long[] values, int size) { + BidirectionalLinearSearcher(long[] values, int size) { assert size > 0 : "at least one value must be present"; int len = (size + 1) >>> 1; // rounded-up to handle odd number of values diff --git a/libs/common/src/main/java/org/opensearch/common/round/BinarySearcher.java b/libs/common/src/main/java/org/opensearch/common/round/BinarySearcher.java index 38d4b24a3aeaf..f14b311a36edc 100644 --- a/libs/common/src/main/java/org/opensearch/common/round/BinarySearcher.java +++ b/libs/common/src/main/java/org/opensearch/common/round/BinarySearcher.java @@ -22,7 +22,7 @@ class BinarySearcher implements Roundable { private final long[] values; private final int size; - public BinarySearcher(long[] values, int size) { + BinarySearcher(long[] values, int size) { assert size > 0 : "at least one value must be present"; this.values = values; diff --git a/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java b/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java index caa23f04a49af..b7422694c3013 100644 --- a/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java +++ b/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java @@ -8,10 +8,15 @@ package org.opensearch.common.round; +import org.opensearch.common.annotation.InternalApi; + /** * Factory class to create and return the fastest implementation of {@link Roundable}. + * + * @opensearch.internal */ -public class RoundableFactory { +@InternalApi +public final class RoundableFactory { /** * The maximum limit up to which linear search is used, otherwise binary search is used. * This is because linear search is much faster on small arrays. diff --git a/server/src/main/java/org/opensearch/common/Rounding.java b/server/src/main/java/org/opensearch/common/Rounding.java index f30e64cdba10d..061934f9722f5 100644 --- a/server/src/main/java/org/opensearch/common/Rounding.java +++ b/server/src/main/java/org/opensearch/common/Rounding.java @@ -444,7 +444,7 @@ protected Prepared maybeUseArray(long minUtcMillis, long maxUtcMillis, int max) values = ArrayUtil.grow(values, i + 1); values[i++] = rounded; } - return new ArrayRounding(values, i, this); + return new ArrayRounding(RoundableFactory.create(values, i), this); } } @@ -456,8 +456,8 @@ private static class ArrayRounding implements Prepared { private final Roundable roundable; private final Prepared delegate; - public ArrayRounding(long[] values, int size, Prepared delegate) { - this.roundable = RoundableFactory.create(values, size); + public ArrayRounding(Roundable roundable, Prepared delegate) { + this.roundable = roundable; this.delegate = delegate; } From 370cdadd74e5e4e0e422351c02065172ea88ffd2 Mon Sep 17 00:00:00 2001 From: Ketan Verma Date: Wed, 15 Nov 2023 00:20:12 +0530 Subject: [PATCH 4/4] Replace assert with IllegalArgumentException for size checks Signed-off-by: Ketan Verma --- .../round/BidirectionalLinearSearcher.java | 4 +++- .../common/round/BinarySearcher.java | 4 +++- .../common/round/RoundableTests.java | 24 +++++++++---------- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/libs/common/src/main/java/org/opensearch/common/round/BidirectionalLinearSearcher.java b/libs/common/src/main/java/org/opensearch/common/round/BidirectionalLinearSearcher.java index 6d16b41e6a2de..5c3dcf2bd4708 100644 --- a/libs/common/src/main/java/org/opensearch/common/round/BidirectionalLinearSearcher.java +++ b/libs/common/src/main/java/org/opensearch/common/round/BidirectionalLinearSearcher.java @@ -28,7 +28,9 @@ class BidirectionalLinearSearcher implements Roundable { private final long[] descending; BidirectionalLinearSearcher(long[] values, int size) { - assert size > 0 : "at least one value must be present"; + if (size <= 0) { + throw new IllegalArgumentException("at least one value must be present"); + } int len = (size + 1) >>> 1; // rounded-up to handle odd number of values ascending = new long[len]; diff --git a/libs/common/src/main/java/org/opensearch/common/round/BinarySearcher.java b/libs/common/src/main/java/org/opensearch/common/round/BinarySearcher.java index f14b311a36edc..b9d76945115ed 100644 --- a/libs/common/src/main/java/org/opensearch/common/round/BinarySearcher.java +++ b/libs/common/src/main/java/org/opensearch/common/round/BinarySearcher.java @@ -23,7 +23,9 @@ class BinarySearcher implements Roundable { private final int size; BinarySearcher(long[] values, int size) { - assert size > 0 : "at least one value must be present"; + if (size <= 0) { + throw new IllegalArgumentException("at least one value must be present"); + } this.values = values; this.size = size; diff --git a/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java b/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java index 5623dc80d1a16..ae9f629c59024 100644 --- a/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java +++ b/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java @@ -41,17 +41,17 @@ public void testFloor() { } } - public void testAssertions() { - AssertionError exception; - - exception = assertThrows(AssertionError.class, () -> new BinarySearcher(new long[0], 0)); - assertEquals("at least one value must be present", exception.getMessage()); - exception = assertThrows(AssertionError.class, () -> new BidirectionalLinearSearcher(new long[0], 0)); - assertEquals("at least one value must be present", exception.getMessage()); - - exception = assertThrows(AssertionError.class, () -> new BinarySearcher(new long[] { 100 }, 1).floor(50)); - assertEquals("key must be greater than or equal to 100", exception.getMessage()); - exception = assertThrows(AssertionError.class, () -> new BidirectionalLinearSearcher(new long[] { 100 }, 1).floor(50)); - assertEquals("key must be greater than or equal to 100", exception.getMessage()); + public void testFailureCases() { + Throwable throwable; + + throwable = assertThrows(IllegalArgumentException.class, () -> new BinarySearcher(new long[0], 0)); + assertEquals("at least one value must be present", throwable.getMessage()); + throwable = assertThrows(IllegalArgumentException.class, () -> new BidirectionalLinearSearcher(new long[0], 0)); + assertEquals("at least one value must be present", throwable.getMessage()); + + throwable = assertThrows(AssertionError.class, () -> new BinarySearcher(new long[] { 100 }, 1).floor(50)); + assertEquals("key must be greater than or equal to 100", throwable.getMessage()); + throwable = assertThrows(AssertionError.class, () -> new BidirectionalLinearSearcher(new long[] { 100 }, 1).floor(50)); + assertEquals("key must be greater than or equal to 100", throwable.getMessage()); } }