Surface a user friendly error when PARTITIONED BY is omitted (#12246)

#12163 makes PARTITIONED BY a required clause in INSERT queries. While this is required, if a user accidentally omits the clause, it emits a JavaCC/Calcite error, since it's syntactically incorrect. The error message is cryptic. Since it's a custom clause, this PR aims to make the clause optional on the syntactic side, but move the validation to DruidSqlInsert where we can surface a friendlier error.

sql/src/main/codegen/includes/insert.ftl

@@ -21,13 +21,15 @@
 SqlNode DruidSqlInsert() :
 {
   SqlNode insertNode;
-  org.apache.druid.java.util.common.Pair<Granularity, String> partitionedBy = null;
+  org.apache.druid.java.util.common.Pair<Granularity, String> partitionedBy = new org.apache.druid.java.util.common.Pair(null, null);
   SqlNodeList clusteredBy = null;
 }
 {
   insertNode = SqlInsert()
-  <PARTITIONED> <BY>
-  partitionedBy = PartitionGranularity()
+  [
+    <PARTITIONED> <BY>
+    partitionedBy = PartitionGranularity()
+  ]
   [
     <CLUSTERED> <BY>
     clusteredBy = ClusterItems()
@@ -65,7 +67,7 @@ SqlNodeList ClusterItems() :
 org.apache.druid.java.util.common.Pair<Granularity, String> PartitionGranularity() :
 {
   SqlNode e = null;
-  org.apache.druid.java.util.common.granularity.Granularity granularity = null;
+  Granularity granularity = null;
   String unparseString = null;
 }
 {
@@ -94,11 +96,17 @@ org.apache.druid.java.util.common.Pair<Granularity, String> PartitionGranularity
       unparseString = "YEAR";
     }
   |
-    <ALL> <TIME>
+    <ALL>
     {
       granularity = Granularities.ALL;
-      unparseString = "ALL TIME";
+      unparseString = "ALL";
     }
+    [
+      <TIME>
+      {
+        unparseString += " TIME";
+      }
+    ]
   |
     e = Expression(ExprContext.ACCEPT_SUB_QUERY)
     {

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

@@ -48,12 +48,18 @@ public class DruidSqlInsert extends SqlInsert
   @Nullable
   private final SqlNodeList clusteredBy;
 
+  /**
+   * While partitionedBy and partitionedByStringForUnparse can be null as arguments to the constructor, this is
+   * disallowed (semantically) and the constructor performs checks to ensure that. This helps in producing friendly
+   * errors when the PARTITIONED BY custom clause is not present, and keeps its error separate from JavaCC/Calcite's
+   * custom errors which can be cryptic when someone accidentally forgets to explicitly specify the PARTITIONED BY clause
+   */
   public DruidSqlInsert(
       @Nonnull SqlInsert insertNode,
-      @Nonnull Granularity partitionedBy,
-      @Nonnull String partitionedByStringForUnparse,
+      @Nullable Granularity partitionedBy,
+      @Nullable String partitionedByStringForUnparse,
       @Nullable SqlNodeList clusteredBy
-  )
+  ) throws ParseException
   {
     super(
         insertNode.getParserPosition(),
@@ -62,10 +68,14 @@ public class DruidSqlInsert extends SqlInsert
         insertNode.getSource(),
         insertNode.getTargetColumnList()
     );
-    Preconditions.checkNotNull(partitionedBy); // Shouldn't hit due to how the parser is written
+    if (partitionedBy == null) {
+      throw new ParseException("INSERT statements must specify PARTITIONED BY clause explictly");
+    }
     this.partitionedBy = partitionedBy;
+
     Preconditions.checkNotNull(partitionedByStringForUnparse);
     this.partitionedByStringForUnparse = partitionedByStringForUnparse;
+
     this.clusteredBy = clusteredBy;
   }
 

sql/src/test/java/org/apache/druid/sql/calcite/CalciteInsertDmlTest.java

@@ -334,6 +334,7 @@ public class CalciteInsertDmlTest extends BaseCalciteQueryTest
                     .put("DAY", Granularities.DAY)
                     .put("MONTH", Granularities.MONTH)
                     .put("YEAR", Granularities.YEAR)
+                    .put("ALL", Granularities.ALL)
                     .put("ALL TIME", Granularities.ALL)
                     .put("FLOOR(__time TO QUARTER)", Granularities.QUARTER)
                     .put("TIME_FLOOR(__time, 'PT1H')", Granularities.HOUR)
@@ -542,6 +543,22 @@ public class CalciteInsertDmlTest extends BaseCalciteQueryTest
     }
   }
 
+  @Test
+  public void testInsertWithoutPartitionedBy()
+  {
+    SqlPlanningException e = Assert.assertThrows(
+        SqlPlanningException.class,
+        () ->
+            testQuery(
+                StringUtils.format("INSERT INTO dst SELECT * FROM %s", externSql(externalDataSource)),
+                ImmutableList.of(),
+                ImmutableList.of()
+            )
+    );
+    Assert.assertEquals("INSERT statements must specify PARTITIONED BY clause explictly", e.getMessage());
+    didTest = true;
+  }
+
   // Currently EXPLAIN PLAN FOR doesn't work with the modified syntax
   @Ignore
   @Test