Fix error messages

sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParserUtils.java

@@ -29,6 +29,7 @@
 import org.apache.druid.java.util.common.StringUtils;
 import org.apache.druid.java.util.common.granularity.Granularity;
 import org.apache.druid.java.util.common.granularity.PeriodGranularity;
+import org.apache.druid.java.util.common.logger.Logger;
 import org.apache.druid.segment.column.ColumnHolder;
 import org.apache.druid.sql.calcite.expression.TimeUnits;
 import org.apache.druid.sql.calcite.expression.builtin.TimeFloorOperatorConversion;
@@ -38,6 +39,9 @@
 
 public class DruidSqlParserUtils
 {
+
+  private static final Logger log = new Logger(DruidSqlParserUtils.class);
+
   /**
    * Delegates to {@code convertSqlNodeToGranularity} and converts the exceptions to {@link ParseException}
    * with the underlying message
@@ -48,6 +52,7 @@ public static Granularity convertSqlNodeToGranularityThrowingParseExceptions(Sql
       return convertSqlNodeToGranularity(sqlNode);
     }
     catch (Exception e) {
+      log.error(e, StringUtils.format("Unable to convert %s to a valid granularity.", sqlNode.toString()));
       throw new ParseException(e.getMessage());
     }
   }
@@ -74,7 +79,8 @@ public static Granularity convertSqlNodeToGranularityThrowingParseExceptions(Sql
   public static Granularity convertSqlNodeToGranularity(SqlNode sqlNode) throws ParseException
   {
 
-    final String genericParseFailedMessageFormatString = "Unable to parse the granularity from %s. Expected HOUR, DAY, MONTH, YEAR, ALL TIME, FLOOR function or %s function";
+    final String genericParseFailedMessageFormatString = "Encountered %s after PARTITIONED BY. "
+                                                         + "Expected HOUR, DAY, MONTH, YEAR, ALL TIME, FLOOR function or %s function";
 
     if (!(sqlNode instanceof SqlCall)) {
       throw new ParseException(StringUtils.format(
@@ -91,15 +97,15 @@ public static Granularity convertSqlNodeToGranularity(SqlNode sqlNode) throws Pa
         "FLOOR".equalsIgnoreCase(operatorName)
         || TimeFloorOperatorConversion.SQL_FUNCTION_NAME.equalsIgnoreCase(operatorName),
         StringUtils.format(
-            "PARTITIONED BY clause can only parse FLOOR and %s functions.",
+            "PARTITIONED BY clause only supports FLOOR(__time TO <unit> and %s(__time, period) functions",
             TimeFloorOperatorConversion.SQL_FUNCTION_NAME
         )
     );
 
     List<SqlNode> operandList = sqlCall.getOperandList();
     Preconditions.checkArgument(
         operandList.size() == 2,
-        StringUtils.format("Invalid number of arguments passed to %s in PARTIITONED BY clause", operatorName)
+        StringUtils.format("%s in PARTITIONED BY clause must have two arguments", operatorName)
     );
 
 
@@ -120,15 +126,15 @@ public static Granularity convertSqlNodeToGranularity(SqlNode sqlNode) throws Pa
       SqlNode granularitySqlNode = operandList.get(1);
       Preconditions.checkArgument(
           granularitySqlNode.getKind().equals(SqlKind.LITERAL),
-          "Second argument to TIME_FLOOR in PARTITIONED BY clause should be string representing a period"
+          "Second argument to TIME_FLOOR in PARTITIONED BY clause must be a period like 'PT1H'"
       );
       String granularityString = SqlLiteral.unchain(granularitySqlNode).toValue();
       Period period;
       try {
         period = new Period(granularityString);
       }
       catch (IllegalArgumentException e) {
-        throw new ParseException(StringUtils.format("Unable to create period from %s", granularitySqlNode.toString()));
+        throw new ParseException(StringUtils.format("%s is an invalid period string", granularitySqlNode.toString()));
       }
       return new PeriodGranularity(period, null, null);
 
@@ -138,15 +144,16 @@ public static Granularity convertSqlNodeToGranularity(SqlNode sqlNode) throws Pa
       // granularitySqlNode.getKind().equals(SqlKind.INTERVAL_QUALIFIER)
       Preconditions.checkArgument(
           granularitySqlNode instanceof SqlIntervalQualifier,
-          "Second argument to the FLOOR function in PARTITIONED BY clause cannot be converted to a valid granularity"
+          "Second argument to the FLOOR function in PARTITIONED BY clause is not a valid granularity. "
+          + "Please refer to the documentation of FLOOR function"
       );
       SqlIntervalQualifier granularityIntervalQualifier = (SqlIntervalQualifier) granularitySqlNode;
 
       Period period = TimeUnits.toPeriod(granularityIntervalQualifier.timeUnitRange);
       Preconditions.checkNotNull(
           period,
           StringUtils.format(
-              "Unable to convert %s to valid granularity for ingestion",
+              "%s is not a valid granularity for ingestion",
               granularityIntervalQualifier.timeUnitRange.toString()
           )
       );

sql/src/test/java/org/apache/druid/sql/calcite/parser/DruidSqlParserUtilsTest.java

@@ -122,14 +122,6 @@ public void testGetGranularityFromFloor() throws ParseException
 
   public static class FloorToGranularityConversionTestErrors
   {
-    /**
-     * incorrect node
-     * incorrect function name
-     * Incorrect number of arguments
-     * incorrect first argument
-     * incorrect second argument
-     */
-
     /**
      * Tests clause like "PARTITIONED BY 'day'"
      */
@@ -139,12 +131,10 @@ public void testConvertSqlNodeToGranularityWithIncorrectNode()
       SqlNode sqlNode = SqlLiteral.createCharString("day", SqlParserPos.ZERO);
       ParseException e = Assert.assertThrows(
           ParseException.class,
-          () -> {
-            DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(sqlNode);
-          }
+          () -> DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(sqlNode)
       );
       Assert.assertEquals(
-          "Unable to parse the granularity from 'day'. Expected HOUR, DAY, MONTH, YEAR, ALL TIME, FLOOR function or TIME_FLOOR function",
+          "Encountered 'day' after PARTITIONED BY. Expected HOUR, DAY, MONTH, YEAR, ALL TIME, FLOOR function or TIME_FLOOR function",
           e.getMessage()
       );
     }
@@ -161,13 +151,11 @@ public void testConvertSqlNodeToGranularityWithIncorrectFunctionCall()
       final SqlNode sqlNode = SqlStdOperatorTable.CEIL.createCall(args);
       ParseException e = Assert.assertThrows(
           ParseException.class,
-          () -> {
-            DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(sqlNode);
-          }
+          () -> DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(sqlNode)
       );
       Assert.assertEquals(
           StringUtils.format(
-              "PARTITIONED BY clause can only parse FLOOR and %s functions.",
+              "PARTITIONED BY clause only supports FLOOR(__time TO <unit> and TIME_FLOOR(__time, period) functions",
               TimeFloorOperatorConversion.SQL_FUNCTION_NAME
           ),
           e.getMessage()
@@ -185,30 +173,77 @@ public void testConvertSqlNodeToGranularityWithIncorrectNumberOfArguments()
       final SqlNode sqlNode = SqlStdOperatorTable.FLOOR.createCall(args);
       ParseException e = Assert.assertThrows(
           ParseException.class,
-          () -> {
-            DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(sqlNode);
-          }
+          () -> DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(sqlNode)
       );
-      Assert.assertEquals("Invalid number of arguments passed to FLOOR in PARTIITONED BY clause", e.getMessage());
+      Assert.assertEquals("FLOOR in PARTITIONED BY clause must have two arguments", e.getMessage());
     }
 
     /**
-     * Tests clause like "PARTITIONED BY (timestamps TO DAY)"
+     * Tests clause like "PARTITIONED BY FLOOR(timestamps TO DAY)"
      */
     @Test
-    public void testConvertSqlNodeToGranularityWithWrongIdentifier()
+    public void testConvertSqlNodeToGranularityWithWrongIdentifierInFloorFunction()
     {
       final SqlNodeList args = new SqlNodeList(SqlParserPos.ZERO);
       args.add(new SqlIdentifier("timestamps", SqlParserPos.ZERO));
       args.add(new SqlIntervalQualifier(TimeUnit.DAY, null, SqlParserPos.ZERO));
       final SqlNode sqlNode = SqlStdOperatorTable.FLOOR.createCall(args);
       ParseException e = Assert.assertThrows(
           ParseException.class,
-          () -> {
-            DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(sqlNode);
-          }
+          () -> DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(sqlNode)
       );
       Assert.assertEquals("First argument to FLOOR in PARTITIONED BY clause can only be __time", e.getMessage());
     }
+
+    /**
+     * Tests clause like "PARTITIONED BY TIME_FLOOR(timestamps, 'PT1H')"
+     */
+    @Test
+    public void testConvertSqlNodeToGranularityWithWrongIdentifierInTimeFloorFunction()
+    {
+      final SqlNodeList args = new SqlNodeList(SqlParserPos.ZERO);
+      args.add(new SqlIdentifier("timestamps", SqlParserPos.ZERO));
+      args.add(SqlLiteral.createCharString("PT1H", SqlParserPos.ZERO));
+      final SqlNode sqlNode = TimeFloorOperatorConversion.SQL_FUNCTION.createCall(args);
+      ParseException e = Assert.assertThrows(
+          ParseException.class,
+          () -> DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(sqlNode)
+      );
+      Assert.assertEquals("First argument to TIME_FLOOR in PARTITIONED BY clause can only be __time", e.getMessage());
+    }
+
+    /**
+     * Tests clause like "PARTITIONED BY FLOOR(__time to ISOYEAR)"
+     */
+    @Test
+    public void testConvertSqlNodeToGranularityWithIncorrectIngestionGranularityInFloorFunction()
+    {
+      final SqlNodeList args = new SqlNodeList(SqlParserPos.ZERO);
+      args.add(new SqlIdentifier("__time", SqlParserPos.ZERO));
+      args.add(new SqlIntervalQualifier(TimeUnit.ISOYEAR, null, SqlParserPos.ZERO));
+      final SqlNode sqlNode = SqlStdOperatorTable.FLOOR.createCall(args);
+      ParseException e = Assert.assertThrows(
+          ParseException.class,
+          () -> DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(sqlNode)
+      );
+      Assert.assertEquals("ISOYEAR is not a valid granularity for ingestion", e.getMessage());
+    }
+
+    /**
+     * Tests clause like "PARTITIONED BY TIME_FLOOR(__time, 'abc')"
+     */
+    @Test
+    public void testConvertSqlNodeToGranularityWithIncorrectIngestionGranularityInTimeFloorFunction()
+    {
+      final SqlNodeList args = new SqlNodeList(SqlParserPos.ZERO);
+      args.add(new SqlIdentifier("__time", SqlParserPos.ZERO));
+      args.add(SqlLiteral.createCharString("abc", SqlParserPos.ZERO));
+      final SqlNode sqlNode = TimeFloorOperatorConversion.SQL_FUNCTION.createCall(args);
+      ParseException e = Assert.assertThrows(
+          ParseException.class,
+          () -> DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(sqlNode)
+      );
+      Assert.assertEquals("'abc' is an invalid period string", e.getMessage());
+    }
   }
 }