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 <cjwylie@gmail.com> --------- Co-authored-by: Clint Wylie <cjwylie@gmail.com>
apache · Jun 20, 2024 · b20c3db · b20c3db
1 parent 7ac0862
commit b20c3db
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 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
@@ -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 @@ static DateTimeZone toTimeZone(final Expr timeZoneArg)
   }
 
   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 @@ static PeriodGranularity toPeriodGranularity(
       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
@@ -63,7 +63,7 @@ static class TimestampCeilExpr extends ExprMacroTable.BaseScalarMacroFunctionExp
     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 int hashCode()
     }
   }
 
-  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 @@ static class TimestampCeilDynamicExpr extends ExprMacroTable.BaseScalarMacroFunc
     @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
@@ -56,9 +56,14 @@ public Expr apply(final List<Expr> args)
     }
   }
 
-  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 static class TimestampFloorExpr extends ExprMacroTable.BaseScalarMacroFun
     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 static class TimestampFloorDynamicExpr extends ExprMacroTable.BaseScalarM
     @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
@@ -200,7 +200,7 @@ private void ready()
 
     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
@@ -22,6 +22,7 @@
 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 void testExpressionContainingNull()
     );
   }
 
+  @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()
   {