This is an automated email from the ASF dual-hosted git repository.
abhishek 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 5bd646e Surface a user friendly error when PARTITIONED BY is omitted
(#12246)
5bd646e is described below
commit 5bd646e10a120a675bf70911b832d1eb1f51ae75
Author: Laksh Singla <[email protected]>
AuthorDate: Fri Feb 11 11:49:00 2022 +0530
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 | 20 ++++++++++++++------
.../druid/sql/calcite/parser/DruidSqlInsert.java | 18 ++++++++++++++----
.../druid/sql/calcite/CalciteInsertDmlTest.java | 17 +++++++++++++++++
3 files changed, 45 insertions(+), 10 deletions(-)
diff --git a/sql/src/main/codegen/includes/insert.ftl
b/sql/src/main/codegen/includes/insert.ftl
index 1a0fc1d..506dca4 100644
--- a/sql/src/main/codegen/includes/insert.ftl
+++ b/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)
{
diff --git
a/sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlInsert.java
b/sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlInsert.java
index e98415e..3c08691 100644
--- a/sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlInsert.java
+++ b/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;
}
diff --git
a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteInsertDmlTest.java
b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteInsertDmlTest.java
index b211756..a156914 100644
--- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteInsertDmlTest.java
+++ b/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
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
