abhishekrb19 commented on code in PR #15836:
URL: https://github.com/apache/druid/pull/15836#discussion_r1480756006
##########
docs/multi-stage-query/reference.md:
##########
@@ -199,6 +199,33 @@ The following ISO 8601 periods are supported for
`TIME_FLOOR` and the string con
- P3M
- P1Y
+The string constant can also include any of the keywords mentioned above:
+
+- `HOUR` - Same as `'PT1H'`
+- `DAY` - Same as `'P1D'`
+- `MONTH` - Same as `'P1M'`
+- `YEAR` - Same as `'P1Y'`
+- `ALL TIME`
+- `ALL` - Alias for `ALL TIME`
+
+The `WEEK` granularity is deprecated and not supported in MSQ.
+
+Examples:
+
+```SQL
+-- Keyword
+PARTITIONED BY HOUR
+
+-- String constant
+PARTITIONED BY 'HOUR'
+
+-- Or
Review Comment:
```suggestion
-- ISO 8601 period
```
##########
docs/multi-stage-query/reference.md:
##########
@@ -199,6 +199,33 @@ The following ISO 8601 periods are supported for
`TIME_FLOOR` and the string con
- P3M
- P1Y
+The string constant can also include any of the keywords mentioned above:
+
+- `HOUR` - Same as `'PT1H'`
+- `DAY` - Same as `'P1D'`
+- `MONTH` - Same as `'P1M'`
+- `YEAR` - Same as `'P1Y'`
+- `ALL TIME`
+- `ALL` - Alias for `ALL TIME`
+
+The `WEEK` granularity is deprecated and not supported in MSQ.
+
+Examples:
+
+```SQL
+-- Keyword
+PARTITIONED BY HOUR
+
+-- String constant
+PARTITIONED BY 'HOUR'
+
+-- Or
+PARTITIOND BY 'PT1H'
Review Comment:
```suggestion
PARTITIONED BY 'PT1H'
```
##########
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:
hmm, why is it `ALL` for `<TIME>`?
##########
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());
}
]
|
e = Expression(ExprContext.ACCEPT_SUB_QUERY)
{
- granularity =
DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(e);
- unparseString = e.toString();
+ // validate
Review Comment:
Or maybe break up the `convertSqlNodeToGranularityThrowingParseExceptions`
to `validateGranularity()` or something?
```suggestion
// validate, the return Granularity value is not needed
```
##########
docs/multi-stage-query/reference.md:
##########
@@ -199,6 +199,33 @@ The following ISO 8601 periods are supported for
`TIME_FLOOR` and the string con
- P3M
- P1Y
+The string constant can also include any of the keywords mentioned above:
+
+- `HOUR` - Same as `'PT1H'`
+- `DAY` - Same as `'P1D'`
+- `MONTH` - Same as `'P1M'`
+- `YEAR` - Same as `'P1Y'`
+- `ALL TIME`
+- `ALL` - Alias for `ALL TIME`
+
+The `WEEK` granularity is deprecated and not supported in MSQ.
+
+Examples:
+
+```SQL
+-- Keyword
+PARTITIONED BY HOUR
+
+-- String constant
Review Comment:
```suggestion
-- String literal
```
##########
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>
Review Comment:
👍
##########
sql/src/test/java/org/apache/druid/sql/calcite/parser/DruidSqlParserUtilsTest.java:
##########
@@ -424,11 +425,11 @@ public void
testConvertSqlNodeToGranularityWithIncorrectIngestionGranularityInTi
args.add(new SqlIdentifier("__time", SqlParserPos.ZERO));
args.add(SqlLiteral.createCharString("abc", SqlParserPos.ZERO));
final SqlNode sqlNode =
TimeFloorOperatorConversion.SQL_FUNCTION.createCall(args);
- ParseException e = Assert.assertThrows(
- ParseException.class,
+ DruidException e = Assert.assertThrows(
+ DruidException.class,
Review Comment:
It seems strange that `convertSqlNodeToGranularityThrowingParseExceptions`
now also throws `DruidException` - maybe the function is not kept up-to-date
and could use a rename to drop the exception suffix
`convertSqlNodeToGranularity`.
##########
sql/src/test/java/org/apache/druid/sql/calcite/parser/DruidSqlUnparseTest.java:
##########
@@ -41,7 +41,7 @@ public void testUnparseInsert() throws ParseException
String sqlQuery = "INSERT INTO dst SELECT * FROM foo PARTITIONED BY ALL
TIME";
String prettySqlQuery = "INSERT INTO \"dst\"\n"
+ "SELECT *\n"
- + " FROM \"foo\" PARTITIONED BY ALL TIME";
+ + " FROM \"foo\" PARTITIONED BY ALL";
Review Comment:
I suppose this change is fine as `ALL` and `ALL TIME` are effectively the
same? Or should we also explicitly handle `ALL TIME` to retain the query as-is?
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlParserUtils.java:
##########
@@ -96,28 +104,61 @@
}
/**
- * 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>
+ * <li>Function call: {@code FLOOR(__time TO TimeUnit)}</li>
+ * <li>Function call: TIME_FLOOR(__time, 'PT1H')}</li>
+ * </ul>
* <p>
- * Validation on the sqlNode is contingent to following conditions:
- * 1. sqlNode is an instance of SqlCall
- * 2. Operator is either one of TIME_FLOOR or FLOOR
- * 3. Number of operands in the call are 2
- * 4. First operand is a SimpleIdentifier representing __time
- * 5. If operator is TIME_FLOOR, the second argument is a literal, and can
be converted to the Granularity class
- * 6. If operator is FLOOR, the second argument is a TimeUnit, and can be
mapped using {@link TimeUnits}
+ * Validation of the function sqlNode is contingent to following conditions:
+ * <ol>
+ * <li>sqlNode is an instance of SqlCall</li>
+ * <li>Operator is either one of TIME_FLOOR or FLOOR</li>
+ * <li>Number of operands in the call are 2</li>
+ * <li>First operand is a SimpleIdentifier representing __time</li>
+ * <li>If operator is TIME_FLOOR, the second argument is a literal, and can
be converted to the Granularity class</li>
+ * <li>If operator is FLOOR, the second argument is a TimeUnit, and can be
mapped using {@link TimeUnits}</li>
+ * </ol>
* <p>
- * Since it is to be used primarily while parsing the SqlNode, it is wrapped
in {@code convertSqlNodeToGranularityThrowingParseExceptions}
+ * This method is called during validation, which will catch any errors. It
is then called again
+ * during conversion, at which time we assume the node is valid.
*
* @param sqlNode SqlNode representing a call to a function
*
* @return Granularity as intended by the function call
*
- * @throws ParseException SqlNode cannot be converted a granularity
+ * @throws InvalidSqlInput if SqlNode cannot be converted to a granularity
Review Comment:
```suggestion
* @throws DruidException if SqlNode cannot be converted to a granularity
```
##########
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlReplace.java:
##########
@@ -51,22 +53,43 @@ public class DruidSqlReplace extends DruidSqlIngest
* 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 DruidSqlReplace(
Review Comment:
please update javadocs to remove references to
`partitionedByStringForUnparse`
##########
sql/src/test/java/org/apache/druid/sql/calcite/parser/DruidSqlParserUtilsTest.java:
##########
@@ -133,6 +134,28 @@ public void testGetGranularityFromFloor() throws
ParseException
Granularity actualGranularity =
DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(floorCall);
Assert.assertEquals(expectedGranularity, actualGranularity);
}
+
+ /**
+ * Tests clause like "PARTITIONED BY 'day'"
+ */
+ @Test
+ public void testConvertSqlNodeToGranularityAsLiteral() throws
ParseException
+ {
+ SqlNode sqlNode = SqlLiteral.createCharString(timeUnit.name(),
SqlParserPos.ZERO);
+ Granularity actualGranularity =
DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(sqlNode);
+ Assert.assertEquals(expectedGranularity, actualGranularity);
+ }
+
+ /**
+ * Tests thet Granularity literal SqlNode can be converted properly to
Granularity.
Review Comment:
```suggestion
* Tests that Granularity literal SqlNode can be converted properly to
Granularity.
```
--
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]
