This is an automated email from the ASF dual-hosted git repository.
abhishekrb pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/druid.git
The following commit(s) were added to refs/heads/master by this push:
new b20c3dbadfe Fix malformed period throwing `ADMIN` persona error
(#16626)
b20c3dbadfe is described below
commit b20c3dbadfe299a8d4715d4ecc924ccfab432fc0
Author: Abhishek Radhakrishnan <[email protected]>
AuthorDate: Thu Jun 20 08:40:28 2024 -0700
Fix malformed period throwing `ADMIN` persona error (#16626)
* Turn invalid periods into user-facing exception providing more context.
The current exception is targeting the ADMIN persona. Catch that and turn
it into a USER persona instead. Also, provide more context in the error
message.
* Review comment: pass the wrapping expression and stringify.
* Update
processing/src/main/java/org/apache/druid/query/expression/ExprUtils.java
Co-authored-by: Clint Wylie <[email protected]>
---------
Co-authored-by: Clint Wylie <[email protected]>
---
.../apache/druid/query/expression/ExprUtils.java | 17 ++++++++++++++--
.../query/expression/TimestampCeilExprMacro.java | 11 ++++++++---
.../query/expression/TimestampFloorExprMacro.java | 11 ++++++++---
.../druid/sql/calcite/planner/CalcitePlanner.java | 2 +-
.../druid/sql/calcite/CalciteSelectQueryTest.java | 23 ++++++++++++++++++++++
5 files changed, 55 insertions(+), 9 deletions(-)
diff --git
a/processing/src/main/java/org/apache/druid/query/expression/ExprUtils.java
b/processing/src/main/java/org/apache/druid/query/expression/ExprUtils.java
index e2bd808d7b9..be513b40248 100644
--- a/processing/src/main/java/org/apache/druid/query/expression/ExprUtils.java
+++ b/processing/src/main/java/org/apache/druid/query/expression/ExprUtils.java
@@ -20,6 +20,7 @@
package org.apache.druid.query.expression;
import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.error.InvalidInput;
import org.apache.druid.java.util.common.DateTimes;
import org.apache.druid.java.util.common.IAE;
import org.apache.druid.java.util.common.granularity.PeriodGranularity;
@@ -45,13 +46,25 @@ public class ExprUtils
}
static PeriodGranularity toPeriodGranularity(
+ final Expr wrappingExpr,
final Expr periodArg,
@Nullable final Expr originArg,
@Nullable final Expr timeZoneArg,
final Expr.ObjectBinding bindings
)
{
- final Period period = new Period(periodArg.eval(bindings).asString());
+ final Period period;
+ try {
+ period = new Period(periodArg.eval(bindings).asString());
+ }
+ catch (IllegalArgumentException iae) {
+ throw InvalidInput.exception(
+ "Invalid period[%s] specified for expression[%s]: [%s]",
+ periodArg.stringify(),
+ wrappingExpr.stringify(),
+ iae.getMessage()
+ );
+ }
final DateTime origin;
final DateTimeZone timeZone;
@@ -69,7 +82,7 @@ public class ExprUtils
final Object value = originArg.eval(bindings).valueOrDefault();
if (value instanceof String && NullHandling.isNullOrEquivalent((String)
value)) {
// We get a blank string here, when sql compatible null handling is
enabled
- // and expression contains empty string for for origin
+ // and expression contains empty string for origin
// e.g timestamp_floor(\"__time\",'PT1M','','UTC')
origin = null;
} else {
diff --git
a/processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java
b/processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java
index cfd63f1ea61..3c5102ae7a2 100644
---
a/processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java
+++
b/processing/src/main/java/org/apache/druid/query/expression/TimestampCeilExprMacro.java
@@ -63,7 +63,7 @@ public class TimestampCeilExprMacro implements
ExprMacroTable.ExprMacro
TimestampCeilExpr(final TimestampCeilExprMacro macro, final List<Expr>
args)
{
super(macro, args);
- this.granularity = getGranularity(args, InputBindings.nilBindings());
+ this.granularity = getGranularity(this, args,
InputBindings.nilBindings());
}
@Nonnull
@@ -113,9 +113,14 @@ public class TimestampCeilExprMacro implements
ExprMacroTable.ExprMacro
}
}
- private static PeriodGranularity getGranularity(final List<Expr> args, final
Expr.ObjectBinding bindings)
+ private static PeriodGranularity getGranularity(
+ final Expr expr,
+ final List<Expr> args,
+ final Expr.ObjectBinding bindings
+ )
{
return ExprUtils.toPeriodGranularity(
+ expr,
args.get(1),
args.size() > 2 ? args.get(2) : null,
args.size() > 3 ? args.get(3) : null,
@@ -135,7 +140,7 @@ public class TimestampCeilExprMacro implements
ExprMacroTable.ExprMacro
@Override
public ExprEval eval(final ObjectBinding bindings)
{
- final PeriodGranularity granularity = getGranularity(args, bindings);
+ final PeriodGranularity granularity = getGranularity(this, args,
bindings);
long argTime = args.get(0).eval(bindings).asLong();
long bucketStartTime = granularity.bucketStart(argTime);
if (argTime == bucketStartTime) {
diff --git
a/processing/src/main/java/org/apache/druid/query/expression/TimestampFloorExprMacro.java
b/processing/src/main/java/org/apache/druid/query/expression/TimestampFloorExprMacro.java
index a243273b8f0..02eed7327f1 100644
---
a/processing/src/main/java/org/apache/druid/query/expression/TimestampFloorExprMacro.java
+++
b/processing/src/main/java/org/apache/druid/query/expression/TimestampFloorExprMacro.java
@@ -56,9 +56,14 @@ public class TimestampFloorExprMacro implements
ExprMacroTable.ExprMacro
}
}
- private static PeriodGranularity computeGranularity(final List<Expr> args,
final Expr.ObjectBinding bindings)
+ private static PeriodGranularity computeGranularity(
+ final Expr expr,
+ final List<Expr> args,
+ final Expr.ObjectBinding bindings
+ )
{
return ExprUtils.toPeriodGranularity(
+ expr,
args.get(1),
args.size() > 2 ? args.get(2) : null,
args.size() > 3 ? args.get(3) : null,
@@ -73,7 +78,7 @@ public class TimestampFloorExprMacro implements
ExprMacroTable.ExprMacro
TimestampFloorExpr(final TimestampFloorExprMacro macro, final List<Expr>
args)
{
super(macro, args);
- this.granularity = computeGranularity(args, InputBindings.nilBindings());
+ this.granularity = computeGranularity(this, args,
InputBindings.nilBindings());
}
/**
@@ -170,7 +175,7 @@ public class TimestampFloorExprMacro implements
ExprMacroTable.ExprMacro
@Override
public ExprEval eval(final ObjectBinding bindings)
{
- final PeriodGranularity granularity = computeGranularity(args, bindings);
+ final PeriodGranularity granularity = computeGranularity(this, args,
bindings);
return
ExprEval.of(granularity.bucketStart(args.get(0).eval(bindings).asLong()));
}
diff --git
a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java
b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java
index 933baaac9ba..8eb9541961c 100644
--- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java
+++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java
@@ -200,7 +200,7 @@ public class CalcitePlanner implements Planner, ViewExpander
state = CalcitePlanner.State.STATE_2_READY;
- // If user specify own traitDef, instead of default default trait,
+ // If user specifies own traitDef, instead of default trait,
// register the trait def specified in traitDefs.
if (this.traitDefs == null) {
planner.addRelTraitDef(ConventionTraitDef.INSTANCE);
diff --git
a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java
b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java
index 3203ae9915e..d1f256207d5 100644
--- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java
+++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java
@@ -22,6 +22,7 @@ package org.apache.druid.sql.calcite;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import org.apache.druid.common.config.NullHandling;
+import org.apache.druid.error.DruidExceptionMatcher;
import org.apache.druid.java.util.common.DateTimes;
import org.apache.druid.java.util.common.Intervals;
import org.apache.druid.java.util.common.granularity.Granularities;
@@ -130,6 +131,28 @@ public class CalciteSelectQueryTest extends
BaseCalciteQueryTest
);
}
+ @Test
+ public void testTimeCeilExpressionContainingInvalidPeriod()
+ {
+ testQueryThrows(
+ "SELECT TIME_CEIL(__time, 'PT1Y') FROM foo",
+ DruidExceptionMatcher.invalidInput().expectMessageContains(
+ "Invalid period['PT1Y'] specified for
expression[timestamp_ceil(\"__time\", 'PT1Y', null, 'UTC')]"
+ )
+ );
+ }
+
+ @Test
+ public void testTimeFloorExpressionContainingInvalidPeriod()
+ {
+ testQueryThrows(
+ "SELECT TIME_FLOOR(TIMESTAMPADD(DAY, -1, __time), 'PT1D') FROM foo",
+ DruidExceptionMatcher.invalidInput().expectMessageContains(
+ "Invalid period['PT1D'] specified for
expression[timestamp_floor((\"__time\" + -86400000), 'PT1D', null, 'UTC')]"
+ )
+ );
+ }
+
@Test
public void testValuesContainingNull()
{
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
