[GitHub] [calcite] tjbanghart commented on a diff in pull request #3009: CALCITE-5447/ DATE_TRUNC() function for BIG_QUERY

[GitBox <gi...@apache.org>]

tjbanghart commented on code in PR #3009:
URL: https://github.com/apache/calcite/pull/3009#discussion_r1065108800


##########
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() :

Review Comment:
   This is great! Can be used for `TIMESTAMP_TRUNC` as well



##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -4408,6 +4408,60 @@ protected void checkTimeUnitCodes(Map<String, TimeUnit> timeUnitCodes) {
     invalidCodes.forEach(invalidConsumer);
   }
 
+  /** Checks parsing of built-in functions that accept time unit
+   *  Checks WEEK(WEEKDAY)
+   * <p>Override if your parser supports more such functions. */
+  @Test void checkWeekdayCustomTimeFrames() {
+    final List<String> validWeekdays = asList(
+        "WEEK",
+        "WEEK(SUNDAY)",
+        "WEEK(MONDAY)",
+        "WEEK(TUESDAY)",
+        "WEEK(WEDNESDAY)",
+        "WEEK(THURSDAY)",
+        "WEEK(FRIDAY)",
+        "WEEK(SUNDAY)"
+    );
+
+    final List<String> invalidWeekdays = asList(
+        "A"
+    );
+    SqlValidatorFixture f = fixture()
+        .withOperatorTable(operatorTableFor(SqlLibrary.BIG_QUERY))
+        .withFactory(tf ->
+            tf.withTypeSystem(typeSystem ->

Review Comment:
   I'm not sure we need this `withTypeSystem` change unless we are adding time unit aliases right? Can we try these tests with just the operator table set?



##########
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:
   Does this support custom time frames? Might want `unitName = SimpleIdentifier()` for arbitrary ones a user has registered - unless we are purposefully not supporting custom ones.



-- 
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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org