abhishekrb19 commented on code in PR #15836:
URL: https://github.com/apache/druid/pull/15836#discussion_r1482027153
##########
sql/src/main/codegen/includes/replace.ftl:
##########
@@ -91,7 +90,7 @@ SqlNode DruidSqlReplaceEof() :
<EOF>
{
sqlInsert = new SqlInsert(s.end(source), SqlNodeList.EMPTY, destination,
source, columnList);
- return new DruidSqlReplace(sqlInsert, partitionedBy.lhs,
partitionedBy.rhs, clusteredBy, replaceTimeQuery, exportFileFormat);
+ return DruidSqlReplace.create(sqlInsert, partitionedBy, clusteredBy,
replaceTimeQuery, exportFileFormat);
Review Comment:
There's an extra whitespace:
```suggestion
return DruidSqlReplace.create(sqlInsert, partitionedBy, clusteredBy,
replaceTimeQuery, exportFileFormat);
```
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlInsert.java:
##########
@@ -95,9 +116,9 @@ public void unparse(SqlWriter writer, int leftPrec, int
rightPrec)
getSource().unparse(writer, 0, 0);
writer.newlineAndIndent();
- if (partitionedByStringForUnparse != null) {
+ if (getPartitionedBy() != null) {
writer.keyword("PARTITIONED BY");
- writer.keyword(partitionedByStringForUnparse);
+ writer.keyword(partitionedBy.toString());
Review Comment:
nit: for consistency, use the getter. This would also make the IDE happy:
```suggestion
writer.keyword(getPartitionedBy().toString());
```
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParserUtils.java:
##########
@@ -200,17 +243,27 @@ public static Granularity
convertSqlNodeToGranularity(SqlNode sqlNode) throws Pa
throw makeInvalidPartitionByException(sqlNode);
}
+ private static Granularity convertSqlLiteralCharToGranularity(SqlLiteral
literal)
+ {
+ try {
+ String value = literal.getValueAs(String.class);
+ return Granularity.fromString(value);
+ }
+ catch (IllegalArgumentException e) {
+ throw makeInvalidPartitionByException(literal);
Review Comment:
The IAE context `e` is dropped here. Should we try to include the message in
the translated exception?
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParserUtils.java:
##########
@@ -77,6 +78,13 @@ public class DruidSqlParserUtils
private static final Logger log = new Logger(DruidSqlParserUtils.class);
public static final String ALL = "all";
+ @VisibleForTesting
+ public static final String PARTITION_ERROR_MESSAGE =
+ "Invalid granularity[%s] specified after PARTITIONED BY clause. "
+ + "Expected "
+ +
StringUtils.replace(StringUtils.replace(Arrays.toString(GranularityType.values()),
"[", ""), "]", ",").trim()
Review Comment:
I think this would also include the `WEEK` option in the error message even
though it's unsupported and documented so? If so, could we exclude `WEEK` by
streaming the enum set?
##########
sql/src/test/resources/calcite/expected/ingest/insertWithClusteredBy-logicalPlan.txt:
##########
@@ -1,4 +1,4 @@
-LogicalInsert(target=[druid.dst], partitionedBy=[{type=period, period=P1D,
timeZone=UTC, origin=null}], clusteredBy=[2, `dim1`, CEIL(`m2`)])
+LogicalInsert(target=[druid.dst], partitionedBy=[FLOOR(`__time` TO DAY)],
clusteredBy=[2, `dim1`, CEIL(`m2`)])
Review Comment:
Could we also a test with `PARTITIONED BY P1D` in the query so the plan
would still contain the granularity?
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParserUtils.java:
##########
@@ -96,28 +104,61 @@ public static Granularity
convertSqlNodeToGranularityThrowingParseExceptions(Sql
}
/**
- * This method is used to extract the granularity from a SqlNode
representing following function calls:
- * 1. FLOOR(__time TO TimeUnit)
- * 2. TIME_FLOOR(__time, 'PT1H')
+ * This method is used to extract the granularity from a SqlNode which
represents
+ * the argument to the {@code PARTITIONED BY} clause. The node can be any of
the following:
+ * <ul>
+ * <li>A literal with a string that matches the SQL keywords
+ * {@code HOUR, DAY, MONTH, YEAR, ALL [TIME]}</li>
+ * <li>A literal string with a period in ISO 8601 format.</li>
Review Comment:
I'm not clear how the ISO 8601 period case is handled here. Would the node
be a `SqlLiteral` or `SqlIdentifier`? The latter isn't handled, so it'd throw
an exception if I'm reading this code right. Do you mind clarifying and adding
unit tests for this case please?
##########
sql/src/main/codegen/includes/common.ftl:
##########
@@ -17,59 +17,53 @@
* under the License.
*/
-// 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>
PartitionGranularity() :
+SqlNode PartitionGranularity() :
{
SqlNode e;
- Granularity granularity;
- String unparseString;
+ SqlNode result;
}
{
(
<HOUR>
{
- granularity = Granularities.HOUR;
- unparseString = "HOUR";
+ result = SqlLiteral.createSymbol(GranularityType.HOUR, getPos());
}
|
<DAY>
{
- granularity = Granularities.DAY;
- unparseString = "DAY";
+ result = SqlLiteral.createSymbol(GranularityType.DAY, getPos());
}
|
<MONTH>
{
- granularity = Granularities.MONTH;
- unparseString = "MONTH";
+ result = SqlLiteral.createSymbol(GranularityType.MONTH, getPos());
}
|
<YEAR>
{
- granularity = Granularities.YEAR;
- unparseString = "YEAR";
+ result = SqlLiteral.createSymbol(GranularityType.YEAR, getPos());
}
|
<ALL>
{
- granularity = Granularities.ALL;
- unparseString = "ALL";
+ result = SqlLiteral.createSymbol(GranularityType.ALL, getPos());
}
[
<TIME>
{
- unparseString += " TIME";
+ result = SqlLiteral.createSymbol(GranularityType.ALL, getPos());
Review Comment:
I see, thanks for clarifying!
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlReplace.java:
##########
@@ -118,9 +152,9 @@ public void unparse(SqlWriter writer, int leftPrec, int
rightPrec)
getSource().unparse(writer, 0, 0);
writer.newlineAndIndent();
- if (partitionedByStringForUnparse != null) {
+ if (getPartitionedBy() != null) {
writer.keyword("PARTITIONED BY");
- writer.keyword(partitionedByStringForUnparse);
+ writer.keyword(partitionedBy.toString());
Review Comment:
```suggestion
writer.keyword(getPartitionedBy().toString());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
