kgyrtkirk commented on code in PR #15836:
URL: https://github.com/apache/druid/pull/15836#discussion_r1479477073
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlIngest.java:
##########
@@ -34,10 +35,8 @@
*/
public abstract class DruidSqlIngest extends SqlInsert
{
- protected final Granularity partitionedBy;
-
- // Used in the unparse function to generate the original query since we
convert the string to an enum
- protected final String partitionedByStringForUnparse;
+ @Nullable
+ protected final SqlNode partitionedBy;
Review Comment:
note: wouldn't it be usefull to turn the `SqlNode` into `Granularity` either
in the constructor or at the point its passed to the constructor?
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlReplace.java:
##########
@@ -53,20 +55,41 @@ public class DruidSqlReplace extends DruidSqlIngest
*/
public DruidSqlReplace(
@Nonnull SqlInsert insertNode,
- @Nullable Granularity partitionedBy,
- @Nullable String partitionedByStringForUnparse,
+ @Nullable SqlNode partitionedBy,
@Nullable SqlNodeList clusteredBy,
@Nullable SqlNode replaceTimeQuery
)
{
- super(
+ this(
insertNode.getParserPosition(),
(SqlNodeList) insertNode.getOperandList().get(0), // No better getter
to extract this
Review Comment:
might be out-of-scope ; but I wonder what's the reason for `get(0)` ?
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlInsert.java:
##########
@@ -41,27 +41,40 @@ public class DruidSqlInsert extends DruidSqlIngest
// This allows reusing super.unparse
public static final SqlOperator OPERATOR = SqlInsert.OPERATOR;
- /**
- * 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(
Review Comment:
note: this constructor could be a static method
##########
sql/src/main/codegen/includes/common.ftl:
##########
@@ -18,58 +18,52 @@
*/
// 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(DruidSqlParserUtils.GranularityGrain.HOUR_GRAIN,
getPos());
}
|
<DAY>
{
- granularity = Granularities.DAY;
- unparseString = "DAY";
+ result =
SqlLiteral.createSymbol(DruidSqlParserUtils.GranularityGrain.DAY_GRAIN,
getPos());
}
|
<MONTH>
{
- granularity = Granularities.MONTH;
- unparseString = "MONTH";
+ result =
SqlLiteral.createSymbol(DruidSqlParserUtils.GranularityGrain.MONTH_GRAIN,
getPos());
}
|
<YEAR>
{
- granularity = Granularities.YEAR;
- unparseString = "YEAR";
+ result =
SqlLiteral.createSymbol(DruidSqlParserUtils.GranularityGrain.YEAR_GRAIN,
getPos());
}
|
<ALL>
{
- granularity = Granularities.ALL;
- unparseString = "ALL";
+ result =
SqlLiteral.createSymbol(DruidSqlParserUtils.GranularityGrain.ALL_GRAIN,
getPos());
}
[
<TIME>
{
- unparseString += " TIME";
+ result =
SqlLiteral.createSymbol(DruidSqlParserUtils.GranularityGrain.ALL_TIME_GRAIN,
getPos());
}
]
|
e = Expression(ExprContext.ACCEPT_SUB_QUERY)
{
- granularity =
DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(e);
Review Comment:
I think some validation should happen here; I don't see a nice way to do it
but I've found this:
```
result = new
SqlLiteral(DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(e),
SYMBOL, getPos());
```
which might work (but the typeName must be supplied - other candidate could
be: `UNKNOWN` )
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParserUtils.java:
##########
@@ -70,13 +75,76 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
+import java.util.Map;
import java.util.stream.Collectors;
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] after PARTITIONED BY. "
+ + "Expected HOUR, DAY, MONTH, YEAR, ALL TIME, FLOOR() or TIME_FLOOR()";
+
+ public enum GranularityGrain
+ {
+ SECOND_GRAIN("SECOND", Granularities.SECOND),
+ MINUTE_GRAIN("MINUTE", Granularities.MINUTE),
+ HOUR_GRAIN("HOUR", Granularities.HOUR),
+ DAY_GRAIN("DAY", Granularities.DAY),
+ WEEK_GRAIN("WEEK", Granularities.WEEK),
+ MONTH_GRAIN("MONTH", Granularities.MONTH),
+ QUARTER_GRAIN("QUARTER", Granularities.QUARTER),
+ YEAR_GRAIN("YEAR", Granularities.YEAR),
+ ALL_GRAIN("ALL", Granularities.ALL),
+ ALL_TIME_GRAIN("ALL TIME", Granularities.ALL);
+
+ static final Map<String, GranularityGrain> VALUE_TO_GRANULARITY_GRAIN =
ImmutableMap.of(
Review Comment:
it seems to me that there are a few aliases here - but it would be great to
rely on the existing classes - there is a `GranularityType` enum which seems to
be a close relative to what `GranularityGrain` describes
--
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]
