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 b74cb7624d Make error messages for insert statements consistent with
select statements (#12414)
b74cb7624d is described below
commit b74cb7624df15aff61c0c979dac9236f3a53bb45
Author: Adarsh Sanjeev <[email protected]>
AuthorDate: Sat Apr 9 12:21:40 2022 +0530
Make error messages for insert statements consistent with select statements
(#12414)
For a query like
INSERT INTO tablename SELECT channel, added as count FROM wikipedia the
error message is Encountered "as count". However, for the insert statement
INSERT INTO t SELECT channel, added as count FROM wikipedia PARTITIONED BY
ALL
returns INSERT statements must specify PARTITIONED BY clause explictly
(incorrectly). This PR corrects this.
Add EOF to end of Druid SQL Insert statements
Rename SQL Insert statements in the parser to reflect the behaviour change
---
sql/src/main/codegen/config.fmpp | 2 +-
sql/src/main/codegen/includes/explain.ftl | 2 +-
sql/src/main/codegen/includes/insert.ftl | 9 ++++++++-
.../apache/druid/sql/calcite/parser/DruidSqlInsert.java | 2 +-
.../apache/druid/sql/calcite/CalciteInsertDmlTest.java | 16 +++++++++++++++-
5 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/sql/src/main/codegen/config.fmpp b/sql/src/main/codegen/config.fmpp
index 502bb2725d..817bd4e79e 100644
--- a/sql/src/main/codegen/config.fmpp
+++ b/sql/src/main/codegen/config.fmpp
@@ -382,7 +382,7 @@ data: {
# Return type of method implementation should be 'SqlNode'.
# Example: SqlShowDatabases(), SqlShowTables().
statementParserMethods: [
- "DruidSqlInsert()"
+ "DruidSqlInsertEof()"
"DruidSqlExplain()"
]
diff --git a/sql/src/main/codegen/includes/explain.ftl
b/sql/src/main/codegen/includes/explain.ftl
index 5431d50d09..b24927e500 100644
--- a/sql/src/main/codegen/includes/explain.ftl
+++ b/sql/src/main/codegen/includes/explain.ftl
@@ -61,7 +61,7 @@ SqlNode DruidQueryOrSqlQueryOrDml() :
}
{
(
- stmt = DruidSqlInsert()
+ stmt = DruidSqlInsertEof()
|
stmt = SqlQueryOrDml()
)
diff --git a/sql/src/main/codegen/includes/insert.ftl
b/sql/src/main/codegen/includes/insert.ftl
index 506dca47d3..ced692759f 100644
--- a/sql/src/main/codegen/includes/insert.ftl
+++ b/sql/src/main/codegen/includes/insert.ftl
@@ -18,7 +18,7 @@
*/
// Using fully qualified name for Pair class, since Calcite also has a same
class name being used in the Parser.jj
-SqlNode DruidSqlInsert() :
+SqlNode DruidSqlInsertEof() :
{
SqlNode insertNode;
org.apache.druid.java.util.common.Pair<Granularity, String> partitionedBy =
new org.apache.druid.java.util.common.Pair(null, null);
@@ -26,6 +26,8 @@ SqlNode DruidSqlInsert() :
}
{
insertNode = SqlInsert()
+ // PARTITIONED BY is necessary, but is kept optional in the grammar. It is
asserted that it is not missing in the
+ // DruidSqlInsert constructor so that we can return a custom error message.
[
<PARTITIONED> <BY>
partitionedBy = PartitionGranularity()
@@ -34,6 +36,11 @@ SqlNode DruidSqlInsert() :
<CLUSTERED> <BY>
clusteredBy = ClusterItems()
]
+ // EOF is also present in SqlStmtEof but EOF is a special case and a single
EOF can be consumed multiple times.
+ // The reason for adding EOF here is to ensure that we create a
DruidSqlInsert node after the syntax has been
+ // validated and throw SQL syntax errors before performing validations in
the DruidSqlInsert which can overshadow the
+ // actual error message.
+ <EOF>
{
if (!(insertNode instanceof SqlInsert)) {
// This shouldn't be encountered, but done as a defensive practice.
SqlInsert() always returns a node of type
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 3c08691220..c941da3e59 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
@@ -69,7 +69,7 @@ public class DruidSqlInsert extends SqlInsert
insertNode.getTargetColumnList()
);
if (partitionedBy == null) {
- throw new ParseException("INSERT statements must specify PARTITIONED BY
clause explictly");
+ throw new ParseException("INSERT statements must specify PARTITIONED BY
clause explicitly");
}
this.partitionedBy = partitionedBy;
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 d0aa72604b..1757d4efc2 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
@@ -554,7 +554,7 @@ public class CalciteInsertDmlTest extends
BaseCalciteQueryTest
ImmutableList.of()
)
);
- Assert.assertEquals("INSERT statements must specify PARTITIONED BY clause
explictly", e.getMessage());
+ Assert.assertEquals("INSERT statements must specify PARTITIONED BY clause
explicitly", e.getMessage());
didTest = true;
}
@@ -729,6 +729,20 @@ public class CalciteInsertDmlTest extends
BaseCalciteQueryTest
.verify();
}
+ @Test
+ public void testInsertWithInvalidSelectStatement()
+ {
+ testInsertQuery()
+ .sql("INSERT INTO t SELECT channel, added as count FROM foo
PARTITIONED BY ALL") // count is a keyword
+ .expectValidationError(
+ CoreMatchers.allOf(
+ CoreMatchers.instanceOf(SqlPlanningException.class),
+
ThrowableMessageMatcher.hasMessage(CoreMatchers.startsWith("Encountered \"as
count\""))
+ )
+ )
+ .verify();
+ }
+
private String externSql(final ExternalDataSource externalDataSource)
{
try {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
