julianhyde commented on code in PR #3079:
URL: https://github.com/apache/calcite/pull/3079#discussion_r1114986375
##########
core/src/main/java/org/apache/calcite/sql/SqlKind.java:
##########
@@ -1215,6 +1218,7 @@ public enum SqlKind {
FILTER, WITHIN_GROUP, IGNORE_NULLS, RESPECT_NULLS, SEPARATOR,
DESCENDING, CUBE, ROLLUP, GROUPING_SETS, EXTEND, LATERAL,
SELECT, JOIN, OTHER_FUNCTION, POSITION, CAST, TRIM, FLOOR,
CEIL,
+ DATE_ADD, TIMESTAMP_ADD, TIMESTAMP_DIFF, EXTRACT, INTERVAL,
Review Comment:
EXTRACT, INTERVAL and maybe others are now duplicated.
##########
core/src/main/codegen/templates/Parser.jj:
##########
@@ -7313,6 +7361,12 @@ SqlNode JdbcFunctionCall() :
s = span();
}
(
+ LOOKAHEAD(1)
+ call = DateDiffFunctionCall() {
+ name = call.getOperator().getName();
+ args = new SqlNodeList(call.getOperandList(), getPos());
+ }
Review Comment:
why have you added a call to `DateDiffFunctionCall` but not
`DatetimeDiffFunctionCall`?
with this omission, I don't understand how the tests could pass.
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -933,6 +948,22 @@ private SqlLibraryOperators() {
ReturnTypes.BIGINT_NULLABLE, OperandTypes.TIMESTAMP,
SqlFunctionCategory.TIMEDATE);
+ /** BigQuery's "DATETIME_ADD(timestamp, interval) function; Behaves similarly
+ * to BigQuery's TIMESTAMP_ADD because in Calcite, datetime is a type alias
+ * for timestamp. */
+ @LibraryOperator(libraries = {BIG_QUERY})
+ public static final SqlFunction DATETIME_ADD =
+ TIMESTAMP_ADD2.withName("DATETIME_ADD");
+
+ /** BigQuery's "DATETIME_DIFF(timestamp, timestamp, timeUnit) function;
Behaves
+ * similarly to BigQuery's TIMESTAMP_DIFF because in Calcite, datetime is a
type
+ * alias for timestamp. Returns the whole number of timeUnit between datetime
+ * and datetime2, with the result being negative if datetime occurs before
datetime2. */
+ @LibraryOperator(libraries = {BIG_QUERY})
+ public static final SqlFunction DATETIME_DIFF =
Review Comment:
As TIMESTAMP_ADD. Also change signature to DATETIME_DIFF(timestamp,
timestamp2, timeUnit)" and fix references to "datetime" and "datetime2" in the
description.
##########
core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java:
##########
@@ -1974,27 +1982,42 @@ private static class TimestampDiffConvertlet implements
SqlRexConvertlet {
SqlIntervalQualifier qualifier;
final RexNode op1;
final RexNode op2;
- if (call.operand(0).getKind() == SqlKind.INTERVAL_QUALIFIER) {
- qualifier = call.operand(0);
- op1 = cx.convertExpression(call.operand(1));
- op2 = cx.convertExpression(call.operand(2));
- } else {
+ final boolean isBigQuery = !(call.operand(0).getKind() ==
SqlKind.INTERVAL_QUALIFIER);
Review Comment:
I don't think the `isBigQuery` variable helps much. You should extend the
comment, a few lines previously, about the two variants, and how one only
occurs in BigQuery mode.
You should also explain why TIMESTAMP_TRUNC calls are necessary.
##########
core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java:
##########
@@ -2027,13 +2050,38 @@ private static class TimestampDiffConvertlet implements
SqlRexConvertlet {
qualifier.getParserPosition());
break;
}
- final RelDataType intervalType =
- cx.getTypeFactory().createTypeWithNullability(
- cx.getTypeFactory().createSqlIntervalType(qualifier),
- op1.getType().isNullable() || op2.getType().isNullable());
- final RexCall rexCall = (RexCall) rexBuilder.makeCall(
- intervalType, SqlStdOperatorTable.MINUS_DATE,
- ImmutableList.of(op2, op1));
+
+ RelDataType intervalType;
+ RexCall rexCall;
+ /* This additional logic handles the differing definitions of 'WEEK'
between BigQuery
Review Comment:
Use Java-style comments (`//`) rather than C-style (`/*` ... `*/`)
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -933,6 +948,22 @@ private SqlLibraryOperators() {
ReturnTypes.BIGINT_NULLABLE, OperandTypes.TIMESTAMP,
SqlFunctionCategory.TIMEDATE);
+ /** BigQuery's "DATETIME_ADD(timestamp, interval) function; Behaves similarly
Review Comment:
Change `BigQuery's "DATETIME_ADD(timestamp, interval) function` to `The
"DATETIME_ADD(timestamp, interval) function (BigQuery)`. I don't want to assume
that no other library will ever use it.
Center the javadoc in Calcite, not BigQuery. Change "; Behaves similarly to
BigQuery's TIMESTAMP_ADD because in Calcite, datetime is a type alias for
timestamp." to ". Like {@code TIMESTAMP_ADD}, returns a Calcite {@code
TIMESTAMP} (which BigQuery calls a {@code DATETIME})."
##########
babel/src/test/resources/sql/big-query.iq:
##########
@@ -1798,21 +1811,20 @@ SELECT
#
# Returns INT64
-!if (false) {
SELECT
DATETIME "2010-07-07 10:20:00" as first_datetime,
DATETIME "2008-12-25 15:30:00" as second_datetime,
DATETIME_DIFF(DATETIME "2010-07-07 10:20:00",
DATETIME "2008-12-25 15:30:00", DAY) as difference;
-+----------------------------+------------------------+------------------------+
-| first_datetime | second_datetime | difference
|
-+----------------------------+------------------------+------------------------+
-| 2010-07-07T10:20:00 | 2008-12-25T15:30:00 | 559
|
-+----------------------------+------------------------+------------------------+
++---------------------+---------------------+------------+
+| first_datetime | second_datetime | difference |
++---------------------+---------------------+------------+
+| 2010-07-07 10:20:00 | 2008-12-25 15:30:00 | 558 |
++---------------------+---------------------+------------+
+(1 row)
Review Comment:
difference has changed from 559 to 558. which is the correct value?
Add a comment to the test justifying the value.
##########
site/_docs/reference.md:
##########
@@ -2649,8 +2653,12 @@ BigQuery's type system uses confusingly different names
for types and functions:
| b | DATETIME(date) | Converts *date* to a
TIMESTAMP value (at midnight)
| b | DATETIME(date, timeZone) | Converts *date* to a
TIMESTAMP value (at midnight), in *timeZone*
| b | DATETIME(year, month, day, hour, minute, second) | Creates a TIMESTAMP
for *year*, *month*, *day*, *hour*, *minute*, *second* (all of type INTEGER)
+| b | DATETIME_ADD(datetime, interval) | Returns the DATETIME
value that occurs *interval* after *datetime*
Review Comment:
s/DATETIME/TIMESTAMP
##########
core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java:
##########
@@ -2027,13 +2050,38 @@ private static class TimestampDiffConvertlet implements
SqlRexConvertlet {
qualifier.getParserPosition());
break;
}
- final RelDataType intervalType =
- cx.getTypeFactory().createTypeWithNullability(
- cx.getTypeFactory().createSqlIntervalType(qualifier),
- op1.getType().isNullable() || op2.getType().isNullable());
- final RexCall rexCall = (RexCall) rexBuilder.makeCall(
- intervalType, SqlStdOperatorTable.MINUS_DATE,
- ImmutableList.of(op2, op1));
+
+ RelDataType intervalType;
+ RexCall rexCall;
+ /* This additional logic handles the differing definitions of 'WEEK'
between BigQuery
+ * and Calcite. BigQuery considers Sunday as the start of the week, so
two dates whose
+ * day difference is <7 may have a week difference of 1 if they occur on
two different
Review Comment:
Your comments seem to imply that the difference is due to weeks starting on
a different day. I don't think that's true. Calcite considers Sunday to be the
start of the week too. (Monday the start of an ISOWEEK.)
I think it's just that this particular function truncates before it
subtracts.
--
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]
