From 6308cea31d821c8cc1549b1102fcd9ddd80feec2 Mon Sep 17 00:00:00 2001 From: Amit Dutta Date: Mon, 31 Jan 2022 20:57:27 -0800 Subject: [PATCH] Adding checkArgument function to properly throw Presto Exception when argument is invalid. --- .../operator/MultiChannelGroupByHash.java | 2 +- .../aggregation/StatisticalDigestFactory.java | 24 ++++++++++++++----- .../sanity/ValidateStreamingAggregations.java | 2 +- .../com/facebook/presto/tdigest/TDigest.java | 2 +- .../com/facebook/presto/util/Failures.java | 14 +++++++++++ .../TestValidateStreamingAggregations.java | 3 ++- 6 files changed, 37 insertions(+), 10 deletions(-) diff --git a/presto-main/src/main/java/com/facebook/presto/operator/MultiChannelGroupByHash.java b/presto-main/src/main/java/com/facebook/presto/operator/MultiChannelGroupByHash.java index 304c3b193ad8a..44d797995e286 100644 --- a/presto-main/src/main/java/com/facebook/presto/operator/MultiChannelGroupByHash.java +++ b/presto-main/src/main/java/com/facebook/presto/operator/MultiChannelGroupByHash.java @@ -40,8 +40,8 @@ import static com.facebook.presto.operator.SyntheticAddress.encodeSyntheticAddress; import static com.facebook.presto.spi.StandardErrorCode.GENERIC_INSUFFICIENT_RESOURCES; import static com.facebook.presto.sql.gen.JoinCompiler.PagesHashStrategyFactory; +import static com.facebook.presto.util.Failures.checkArgument; import static com.facebook.presto.util.HashCollisionsEstimator.estimateNumberOfHashCollisions; -import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Verify.verify; import static io.airlift.slice.SizeOf.sizeOf; diff --git a/presto-main/src/main/java/com/facebook/presto/operator/aggregation/StatisticalDigestFactory.java b/presto-main/src/main/java/com/facebook/presto/operator/aggregation/StatisticalDigestFactory.java index ea07dce63c61b..a7c24f8e507bb 100644 --- a/presto-main/src/main/java/com/facebook/presto/operator/aggregation/StatisticalDigestFactory.java +++ b/presto-main/src/main/java/com/facebook/presto/operator/aggregation/StatisticalDigestFactory.java @@ -14,9 +14,11 @@ package com.facebook.presto.operator.aggregation; import com.facebook.airlift.stats.QuantileDigest; +import com.facebook.presto.spi.PrestoException; import com.facebook.presto.tdigest.TDigest; import io.airlift.slice.Slice; +import static com.facebook.presto.spi.StandardErrorCode.INVALID_FUNCTION_ARGUMENT; import static com.google.common.base.Preconditions.checkArgument; public class StatisticalDigestFactory @@ -54,9 +56,14 @@ public void add(double value, long weight) @Override public void merge(StatisticalDigest other) { - checkArgument(other instanceof StatisticalTDigest); - StatisticalTDigest toMerge = (StatisticalTDigest) other; - tDigest.merge(toMerge.tDigest); + try { + checkArgument(other instanceof StatisticalTDigest); + StatisticalTDigest toMerge = (StatisticalTDigest) other; + tDigest.merge(toMerge.tDigest); + } + catch (IllegalArgumentException ex) { + throw new PrestoException(INVALID_FUNCTION_ARGUMENT, ex.getMessage(), ex); + } } @Override @@ -103,9 +110,14 @@ public void add(long value, long weight) @Override public void merge(StatisticalDigest other) { - checkArgument(other instanceof StatisticalQuantileDigest); - StatisticalQuantileDigest toMerge = (StatisticalQuantileDigest) other; - this.qdigest.merge(toMerge.qdigest); + try { + checkArgument(other instanceof StatisticalQuantileDigest); + StatisticalQuantileDigest toMerge = (StatisticalQuantileDigest) other; + this.qdigest.merge(toMerge.qdigest); + } + catch (IllegalArgumentException ex) { + throw new PrestoException(INVALID_FUNCTION_ARGUMENT, ex.getMessage(), ex); + } } @Override diff --git a/presto-main/src/main/java/com/facebook/presto/sql/planner/sanity/ValidateStreamingAggregations.java b/presto-main/src/main/java/com/facebook/presto/sql/planner/sanity/ValidateStreamingAggregations.java index 14d7a543e750f..83253754282ea 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/planner/sanity/ValidateStreamingAggregations.java +++ b/presto-main/src/main/java/com/facebook/presto/sql/planner/sanity/ValidateStreamingAggregations.java @@ -35,7 +35,7 @@ import java.util.Optional; import static com.facebook.presto.sql.planner.optimizations.StreamPropertyDerivations.derivePropertiesRecursively; -import static com.google.common.base.Preconditions.checkArgument; +import static com.facebook.presto.util.Failures.checkArgument; /** * Verifies that input of streaming aggregations is grouped on the grouping keys diff --git a/presto-main/src/main/java/com/facebook/presto/tdigest/TDigest.java b/presto-main/src/main/java/com/facebook/presto/tdigest/TDigest.java index fd4e2410e443b..a079b76dffbdb 100644 --- a/presto-main/src/main/java/com/facebook/presto/tdigest/TDigest.java +++ b/presto-main/src/main/java/com/facebook/presto/tdigest/TDigest.java @@ -93,7 +93,7 @@ public class TDigest { private static final int INSTANCE_SIZE = ClassLayout.parseClass(TDigest.class).instanceSize(); - private static final double MAX_COMPRESSION_FACTOR = 1_000; + static final double MAX_COMPRESSION_FACTOR = 1_000; private static final double sizeFudge = 30; private static final double EPSILON = 0.001; diff --git a/presto-main/src/main/java/com/facebook/presto/util/Failures.java b/presto-main/src/main/java/com/facebook/presto/util/Failures.java index 530ade0d48ffe..ff3e6e3572933 100644 --- a/presto-main/src/main/java/com/facebook/presto/util/Failures.java +++ b/presto-main/src/main/java/com/facebook/presto/util/Failures.java @@ -62,6 +62,20 @@ public static ExecutionFailureInfo toFailure(Throwable failure) return toFailure(failure, newIdentityHashSet()); } + public static void checkArgument(boolean expression, String errorMessage) + { + if (!expression) { + throw new PrestoException(StandardErrorCode.INVALID_ARGUMENTS, errorMessage); + } + } + + public static void checkArgument(boolean expression, String errorMessageTemplate, Object... errorMessageArgs) + { + if (!expression) { + throw new PrestoException(StandardErrorCode.INVALID_ARGUMENTS, String.format(errorMessageTemplate, errorMessageArgs)); + } + } + public static void checkCondition(boolean condition, ErrorCodeSupplier errorCode, String formatString, Object... args) { if (!condition) { diff --git a/presto-main/src/test/java/com/facebook/presto/sql/planner/sanity/TestValidateStreamingAggregations.java b/presto-main/src/test/java/com/facebook/presto/sql/planner/sanity/TestValidateStreamingAggregations.java index e69dd8ae40d92..2d30517843178 100644 --- a/presto-main/src/test/java/com/facebook/presto/sql/planner/sanity/TestValidateStreamingAggregations.java +++ b/presto-main/src/test/java/com/facebook/presto/sql/planner/sanity/TestValidateStreamingAggregations.java @@ -16,6 +16,7 @@ import com.facebook.presto.common.predicate.TupleDomain; import com.facebook.presto.metadata.Metadata; import com.facebook.presto.spi.ConnectorId; +import com.facebook.presto.spi.PrestoException; import com.facebook.presto.spi.TableHandle; import com.facebook.presto.spi.WarningCollector; import com.facebook.presto.spi.plan.PlanNode; @@ -90,7 +91,7 @@ public void testValidateSuccessful() ImmutableMap.of(p.variable("nationkey", BIGINT), new TpchColumnHandle("nationkey", BIGINT))))))); } - @Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "Streaming aggregation with input not grouped on the grouping keys") + @Test(expectedExceptions = PrestoException.class, expectedExceptionsMessageRegExp = "Streaming aggregation with input not grouped on the grouping keys") public void testValidateFailed() { validatePlan(