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 0fd4f1e386 Improve error messages from SQL REPLACE syntax (#12523)
0fd4f1e386 is described below
commit 0fd4f1e3863f8f33bec69fc77dcb1ea118a42ed4
Author: Adarsh Sanjeev <[email protected]>
AuthorDate: Tue May 17 09:55:58 2022 +0530
Improve error messages from SQL REPLACE syntax (#12523)
- Add user friendly error messages for missing or incorrect OVERWRITE
clause for REPLACE SQL query
- Move validation of missing OVERWRITE clause at code level instead of
parser for custom error message
---
sql/src/main/codegen/includes/replace.ftl | 7 ++++---
.../druid/sql/calcite/parser/DruidSqlReplace.java | 7 +++++--
.../apache/druid/sql/calcite/planner/DruidPlanner.java | 2 +-
.../druid/sql/calcite/CalciteReplaceDmlTest.java | 18 ++++++++++++++++++
4 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/sql/src/main/codegen/includes/replace.ftl
b/sql/src/main/codegen/includes/replace.ftl
index 741c59c0ad..b8b0199d70 100644
--- a/sql/src/main/codegen/includes/replace.ftl
+++ b/sql/src/main/codegen/includes/replace.ftl
@@ -28,7 +28,7 @@ SqlNode DruidSqlReplaceEof() :
// Using fully qualified name for Pair class, since Calcite also has a
same class name being used in the Parser.jj
org.apache.druid.java.util.common.Pair<Granularity, String> partitionedBy
= new org.apache.druid.java.util.common.Pair(null, null);
final Pair<SqlNodeList, SqlNodeList> p;
- final SqlNode replaceTimeQuery;
+ SqlNode replaceTimeQuery = null;
}
{
<REPLACE> { s = span(); }
@@ -41,8 +41,9 @@ SqlNode DruidSqlReplaceEof() :
}
}
]
- <OVERWRITE>
- replaceTimeQuery = ReplaceTimeQuery()
+ [
+ <OVERWRITE> replaceTimeQuery = ReplaceTimeQuery()
+ ]
source = OrderedQueryOrExpr(ExprContext.ACCEPT_QUERY)
// 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.
diff --git
a/sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlReplace.java
b/sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlReplace.java
index a56fd69251..d1860600e8 100644
--- a/sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlReplace.java
+++ b/sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlReplace.java
@@ -61,7 +61,7 @@ public class DruidSqlReplace extends SqlInsert
@Nonnull SqlInsert insertNode,
@Nullable Granularity partitionedBy,
@Nullable String partitionedByStringForUnparse,
- @Nonnull SqlNode replaceTimeQuery
+ @Nullable SqlNode replaceTimeQuery
) throws ParseException
{
super(
@@ -71,8 +71,11 @@ public class DruidSqlReplace extends SqlInsert
insertNode.getSource(),
insertNode.getTargetColumnList()
);
+ if (replaceTimeQuery == null) {
+ throw new ParseException("Missing time chunk information in OVERWRITE
clause for REPLACE, set it to OVERWRITE WHERE <__time based condition> or set
it to overwrite the entire table with OVERWRITE ALL.");
+ }
if (partitionedBy == null) {
- throw new ParseException("REPLACE statements must specify PARTITIONED BY
clause explictly");
+ throw new ParseException("REPLACE statements must specify PARTITIONED BY
clause explicitly");
}
this.partitionedBy = partitionedBy;
diff --git
a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
index c552356efd..7680c7a335 100644
--- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
+++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
@@ -899,7 +899,7 @@ public class DruidPlanner implements Closeable
SqlNode replaceTimeQuery = druidSqlReplace.getReplaceTimeQuery();
if (replaceTimeQuery == null) {
- throw new ValidationException("Missing time chunk information in
DELETE WHERE clause for replace.");
+ throw new ValidationException("Missing time chunk information in
OVERWRITE clause for REPLACE, set it to OVERWRITE WHERE <__time based
condition> or set it to overwrite the entire table with OVERWRITE ALL.");
}
Granularity ingestionGranularity = druidSqlReplace.getPartitionedBy();
diff --git
a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteReplaceDmlTest.java
b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteReplaceDmlTest.java
index 04354d126f..b3b0f97556 100644
--- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteReplaceDmlTest.java
+++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteReplaceDmlTest.java
@@ -368,6 +368,24 @@ public class CalciteReplaceDmlTest extends
CalciteIngestionDmlTest
.verify();
}
+ @Test
+ public void testReplaceWithoutOverwriteClause()
+ {
+ testIngestionQuery()
+ .sql("REPLACE INTO dst SELECT * FROM foo PARTITIONED BY ALL TIME")
+ .expectValidationError(SqlPlanningException.class, "Missing time chunk
information in OVERWRITE clause for REPLACE, set it to OVERWRITE WHERE <__time
based condition> or set it to overwrite the entire table with OVERWRITE ALL.")
+ .verify();
+ }
+
+ @Test
+ public void testReplaceWithoutCompleteOverwriteClause()
+ {
+ testIngestionQuery()
+ .sql("REPLACE INTO dst OVERWRITE SELECT * FROM foo PARTITIONED BY ALL
TIME")
+ .expectValidationError(SqlPlanningException.class, "Missing time chunk
information in OVERWRITE clause for REPLACE, set it to OVERWRITE WHERE <__time
based condition> or set it to overwrite the entire table with OVERWRITE ALL.")
+ .verify();
+ }
+
@Test
public void testReplaceIntoSystemTable()
{
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
