julianhyde commented on code in PR #3009:
URL: https://github.com/apache/calcite/pull/3009#discussion_r1065243893
##########
babel/src/test/java/org/apache/calcite/test/BabelTest.java:
##########
@@ -189,6 +189,23 @@ private void checkInfixCast(Statement statement, String
typeName, int sqlType)
.fails("'A' is not a valid time frame");
f2.withSql("SELECT DATE_PART(minute15, " + ts + ")")
.ok();
+
+ final SqlValidatorFixture f3 =
Review Comment:
I think this test should be moved. When DATE_TRUNC supports custom time
frames, you will no longer get the parse error "Encountered \"A\" at line 1".
Add something to `SqlParserTest.testTimeUnitCodes` and something to
`SqlValidatorTest.checkTimeUnitCodes` and I think you're good.
##########
core/src/main/codegen/templates/Parser.jj:
##########
@@ -5050,6 +5050,86 @@ SqlIntervalQualifier TimeUnitOrName() : {
}
}
+ SqlIntervalQualifier TimeUnitOrNameForDateTrunc() : {
+ final TimeUnit timeUnit;
+ final SqlIntervalQualifier weekdayIntervalQualifier;
+ final SqlIdentifier unitName;
+}
+{
+ LOOKAHEAD(1)
+ timeUnit = TimeUnitForDateTrunc() {
+ return new SqlIntervalQualifier(timeUnit, null, getPos());
+ }
+|
+ weekdayIntervalQualifier = WeekWeekdaySqlIntervalQualifier() {
+ return weekdayIntervalQualifier;
+ }
+|
+ unitName = DateTruncIdentifiers() {
Review Comment:
In CALCITE-5155 comment I wrote:
> Previously, NANOSECOND and MILLISECOND were allowed in
> EXTRACT but no other functions. Now all functions that
> accept time frames accept the same time frames (built-in
> time intervals, identifiers for user-defined time frames,
> and SQL_TSI_xxx which are defined in the JDBC standard but
> are treated as identifiers until validation).
I think you should accept all time frames (including custom frames) in the
parser. That should make the parser much simpler. We *could* reject certain
frames at validate time, but I don't think we will. It's easier if we implement
a superset of what BQ implements.
##########
site/_docs/reference.md:
##########
@@ -2616,6 +2625,7 @@ semantics.
| q | DATEPART(timeUnit, datetime) | Equivalent to
`EXTRACT(timeUnit FROM datetime)`
| b | DATE_FROM_UNIX_DATE(integer) | Returns the DATE that
is *integer* days after 1970-01-01
| p | DATE_PART(timeUnit, datetime) | Equivalent to
`EXTRACT(timeUnit FROM datetime)`
+| b | DATE_TRUNC(date_expressions, date_part) | Truncates a *date*
value to the granularity of *date_part*. The *date* value is always rounded to
the beginning of the *date_part*.
Review Comment:
no underscores, no final period
##########
core/src/main/codegen/templates/Parser.jj:
##########
@@ -5050,6 +5050,86 @@ SqlIntervalQualifier TimeUnitOrName() : {
}
}
+ SqlIntervalQualifier TimeUnitOrNameForDateTrunc() : {
+ final TimeUnit timeUnit;
+ final SqlIntervalQualifier weekdayIntervalQualifier;
+ final SqlIdentifier unitName;
+}
+{
+ LOOKAHEAD(1)
+ timeUnit = TimeUnitForDateTrunc() {
+ return new SqlIntervalQualifier(timeUnit, null, getPos());
+ }
+|
+ weekdayIntervalQualifier = WeekWeekdaySqlIntervalQualifier() {
+ return weekdayIntervalQualifier;
+ }
+|
+ unitName = DateTruncIdentifiers() {
+ return new SqlIntervalQualifier(unitName.getSimple(),
+ unitName.getParserPosition());
+ }
+}
+
+/** Parses time unit for the DATE_TRUNC function.
+* This is a subset of time units appropriate for DATE data type.
+*/
+TimeUnit TimeUnitForDateTrunc() : {}
+{
+ <DAY> { return TimeUnit.DAY; }
+| <MONTH> { return TimeUnit.MONTH; }
+| <QUARTER> { return TimeUnit.QUARTER; }
+| <YEAR> { return TimeUnit.YEAR; }
+}
+
+SqlIntervalQualifier WeekWeekdaySqlIntervalQualifier() : {
+ final String weekdayName;
+}
+{
+ weekdayName = WeekWeekday() {
+ return new SqlIntervalQualifier(weekdayName, getPos());
+ }
+}
+String WeekWeekday() :
+{
+}
+{
+ LOOKAHEAD(4)
+ <WEEK> <LPAREN> <SUNDAY> <RPAREN> { return "WEEK_SUNDAY"; }
+|
+ LOOKAHEAD(4)
+ <WEEK> <LPAREN> <MONDAY> <RPAREN> { return "WEEK_MONDAY"; }
+|
+ LOOKAHEAD(4)
+ <WEEK> <LPAREN> <TUESDAY> <RPAREN> { return "WEEK_TUESDAY"; }
+|
+ LOOKAHEAD(4)
+ <WEEK> <LPAREN> <WEDNESDAY> <RPAREN> { return "WEEK_WEDNESDAY"; }
+|
+ LOOKAHEAD(4)
+ <WEEK> <LPAREN> <THURSDAY> <RPAREN> { return "WEEK_THURSDAY"; }
+|
+ LOOKAHEAD(4)
+ <WEEK> <LPAREN> <FRIDAY> <RPAREN> { return "WEEK_FRIDAY"; }
+|
+ LOOKAHEAD(4)
+ <WEEK> <LPAREN> <SATURDAY> <RPAREN> { return "WEEK_SATURDAY"; }
+|
+ LOOKAHEAD(1)
+ <WEEK> { return "WEEK"; }
+}
Review Comment:
High lookahead values are expensive, and LOOKAHEAD(4) might be overkill. I
would write something like
```
<WEEK>
(
<LPAREN> w = weekDay() <RPAREN> { return weekDays.get(w); }
|
/* empty */ { return "WEEK"; }
)
```
and now the parser doesn't have to go so deep before it makes a decision.
##########
core/src/main/java/org/apache/calcite/rel/type/TimeFrames.java:
##########
@@ -100,6 +101,28 @@ private static BuilderImpl addCore(BuilderImpl b) {
b.addRollup(TimeUnit.DAY, TimeUnit.MONTH);
b.addRollup("ISOWEEK", TimeUnit.ISOYEAR.name());
+ b.addSub("WEEK_SUNDAY", false, 7,
+ "DAY", new TimestampString(1970, 1, 4, 0, 0, 0)
+ );
+ b.addSub("WEEK_MONDAY", false, 7,
Review Comment:
no space before ')'
--
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]
