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
This commit is contained in:
Adarsh Sanjeev 2022-05-17 09:55:58 +05:30 committed by GitHub
parent fdfecfd996
commit 0fd4f1e386
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 28 additions and 6 deletions

View File

@ -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 // 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); 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 Pair<SqlNodeList, SqlNodeList> p;
final SqlNode replaceTimeQuery; SqlNode replaceTimeQuery = null;
} }
{ {
<REPLACE> { s = span(); } <REPLACE> { s = span(); }
@ -41,8 +41,9 @@ SqlNode DruidSqlReplaceEof() :
} }
} }
] ]
<OVERWRITE> [
replaceTimeQuery = ReplaceTimeQuery() <OVERWRITE> replaceTimeQuery = ReplaceTimeQuery()
]
source = OrderedQueryOrExpr(ExprContext.ACCEPT_QUERY) 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 // 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. // DruidSqlInsert constructor so that we can return a custom error message.

View File

@ -61,7 +61,7 @@ public class DruidSqlReplace extends SqlInsert
@Nonnull SqlInsert insertNode, @Nonnull SqlInsert insertNode,
@Nullable Granularity partitionedBy, @Nullable Granularity partitionedBy,
@Nullable String partitionedByStringForUnparse, @Nullable String partitionedByStringForUnparse,
@Nonnull SqlNode replaceTimeQuery @Nullable SqlNode replaceTimeQuery
) throws ParseException ) throws ParseException
{ {
super( super(
@ -71,8 +71,11 @@ public class DruidSqlReplace extends SqlInsert
insertNode.getSource(), insertNode.getSource(),
insertNode.getTargetColumnList() 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) { 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; this.partitionedBy = partitionedBy;

View File

@ -899,7 +899,7 @@ public class DruidPlanner implements Closeable
SqlNode replaceTimeQuery = druidSqlReplace.getReplaceTimeQuery(); SqlNode replaceTimeQuery = druidSqlReplace.getReplaceTimeQuery();
if (replaceTimeQuery == null) { 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(); Granularity ingestionGranularity = druidSqlReplace.getPartitionedBy();

View File

@ -368,6 +368,24 @@ public class CalciteReplaceDmlTest extends CalciteIngestionDmlTest
.verify(); .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 @Test
public void testReplaceIntoSystemTable() public void testReplaceIntoSystemTable()
{ {