[GitHub] [calcite] julianhyde commented on a diff in pull request #3008: [CALCITE-5436] Implement DATE_SUB, TIME_SUB, TIMESTAMP_SUB (compatible w/ BigQuery)

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

julianhyde commented on code in PR #3008:
URL: https://github.com/apache/calcite/pull/3008#discussion_r1063030973


##########
babel/src/test/resources/sql/big-query.iq:
##########
@@ -1348,17 +1347,17 @@ SELECT
 
 # Display of results may differ, depending upon the environment and
 # time zone where this query was executed.
-!if (false) {
+
 SELECT
-  TIMESTAMP("2008-12-25 15:30:00+00") AS original,

Review Comment:
   ditto, leave `AS original`



##########
core/src/main/codegen/default_config.fmpp:
##########
@@ -213,6 +213,7 @@ parser: {
     "PRIVILEGES"
     "PUBLIC"
     "QUARTER"
+    "QUARTERS"

Review Comment:
   I think you should break out Quarters/Weeks as a separate commit. Leave it as part of this PR, but it will need a test (maybe SqlParserTest?)



##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -660,6 +669,24 @@ private SqlLibraryOperators() {
           OperandTypes.TIMESTAMP_INTERVAL)
           .withFunctionType(SqlFunctionCategory.TIMEDATE);
 
+  /** The "TIME_SUB(time_expression, INTERVAL int64_expression date_part)"
+   * function (BigQuery) subtracts int64_expression units of date_part from

Review Comment:
   s/INTERVAL int64_expression date_part/interval_expression/
   
   In Calcite the INTERVAL keyword isn't specific to the TIME_SUB function, it's part of the interval expression.
   
   Also Calcite doesn't have an int64 type. (Yeah I know BQ does but we're using Calcite terminology here.)



##########
core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java:
##########
@@ -1893,6 +1899,39 @@ private static class TimestampAddConvertlet implements SqlRexConvertlet {
     }
   }
 
+  /** Convertlet that handles the BigQuery {@code TIMESTAMP_SUB} function. */
+  private static class TimestampSubConvertlet implements SqlRexConvertlet {
+    @Override public RexNode convertCall(SqlRexContext cx, SqlCall call) {
+      // TIMESTAMP_SUB(timestamp, interval)
+      //  => timestamp - count * INTERVAL '1' UNIT
+      final RexBuilder rexBuilder = cx.getRexBuilder();
+      final SqlBasicCall operandCall = call.operand(1);
+      SqlIntervalQualifier qualifier = operandCall.operand(1);
+      final RexNode op1 = cx.convertExpression(operandCall.operand(0));
+      final RexNode op2 = cx.convertExpression(call.operand(0));
+      final TimeFrame timeFrame = cx.getValidator().validateTimeFrame(qualifier);
+      final TimeUnit unit = timeFrame.unit();
+      RexNode interval2Sub;

Review Comment:
   `interval2Sub` should be `final`.
   
   Fix the checkerframework errors.
   
   Add comment why special handling for NANO, MICRO



##########
site/_docs/reference.md:
##########
@@ -2615,6 +2619,7 @@ semantics.
 | p q | DATEADD(timeUnit, integer, datetime)         | Equivalent to `TIMESTAMPADD(timeUnit, integer, datetime)`
 | p q | DATEDIFF(timeUnit, datetime, datetime2)      | Equivalent to `TIMESTAMPDIFF(timeUnit, datetime, datetime2)`
 | q | DATEPART(timeUnit, datetime)                   | Equivalent to `EXTRACT(timeUnit FROM  datetime)`
+| b | DATE_SUB(date, interval int64 date_part)       | Subtracts int64_expression units of date_part from the date, independent of any time zone.

Review Comment:
   not `int64_expression units of date_part`; it's just an interval expression
   
   remove '.' from end of line
   
   DATE_SUB, TIME_SUB, TIMESTAMP_SUB should have very similar descriptions to TIMESTAMP_ADD. I dropped the ball when reviewing TIMESTAMP_ADD. It should have been
   ```
   TIMESTAMP_ADD(timestamp, interval) | Returns the TIMESTAMP value that occurs *interval* after *timestamp*
   ```
   
   `TIMESTAMP_SUB` should be "the TIMESTAMP value that occurs *interval* before *timestamp*", and similarly DATE_SUB, TIME_SUB



##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -700,6 +727,14 @@ private SqlLibraryOperators() {
           ReturnTypes.TIMESTAMP_NULLABLE, OperandTypes.INTEGER,
           SqlFunctionCategory.TIMEDATE);
 
+  /** 2-argument form of the special minus-date operator

Review Comment:
   I'm not sure how MINUS_DATE2 gets found by the validator. The syntax is 'special' so it isn't found by name. Users can't reference it in their queries. If the operator is only used in intermediate expressions move it to `class SqlInternalOperators`.
   
   I also don't see why the return-type inference needs to be different.



##########
babel/src/test/resources/sql/big-query.iq:
##########
@@ -1318,17 +1318,16 @@ SELECT
 #
 # Returns TIME
 
-!if (false) {
 SELECT
-  TIME "15:30:00" as original_date,

Review Comment:
   don't remove the `TIME "15:30:00" as original_date` line. it was in the original example



##########
core/src/main/java/org/apache/calcite/sql/fun/SqlDatetimeSubtractionOperator.java:
##########
@@ -37,17 +37,23 @@
  * additional interval qualifier specification, when in {@link SqlCall} form.
  * In {@link org.apache.calcite.rex.RexNode} form, it has only two parameters,
  * and the return type describes the desired type of interval.
+ *
+ * When being used for BigQuery's TIMESTAMP_SUB, TIME_SUB, and DATE_SUB, the

Review Comment:
   add '<p>'
   
   The usual operator has infix syntax, '(t - t2) <unit> [ to <unit> ] '. It's a bit confusing to use the same operator class with function syntax. Did you consider extending `class SqlTimestampDiffFunction` (used for Redshsift `DATEDIFF`) instead?
   
   Also strange that you do not parameterize the name.



##########
core/src/main/java/org/apache/calcite/sql/fun/SqlDatetimeSubtractionOperator.java:
##########
@@ -37,17 +37,23 @@
  * additional interval qualifier specification, when in {@link SqlCall} form.
  * In {@link org.apache.calcite.rex.RexNode} form, it has only two parameters,
  * and the return type describes the desired type of interval.
+ *
+ * When being used for BigQuery's TIMESTAMP_SUB, TIME_SUB, and DATE_SUB, the

Review Comment:
   Ah, I see you're only using it for 'tsValue - intervalValue'. Not TIMESTAMP_SUB contrary to what you say in the comments.
   
   I thought Calcite already supported 'tsValue - intervalValue' out of the box. Surprised you needed a new thing.



##########
core/src/main/java/org/apache/calcite/sql/SqlKind.java:
##########
@@ -424,12 +424,20 @@ public enum SqlKind {
   /** {@code LEAST} function (Oracle). */
   LEAST,
 
+  /** {@code DATE_SUB} function (BigQuery Semantics). */

Review Comment:
   s/BigQuery Semantics/BigQuery/



-- 
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