You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2022/12/06 09:22:05 UTC
[GitHub] [calcite] julianhyde commented on a diff in pull request #2998: [CALCITE-5360] Implement BigQuery TIMESTAMP_ADD
julianhyde commented on code in PR #2998:
URL: https://github.com/apache/calcite/pull/2998#discussion_r1040631072
##########
core/src/main/codegen/templates/Parser.jj:
##########
@@ -4824,7 +4824,7 @@ SqlNode IntervalLiteralOrExpression() :
|
e = CompoundIdentifier()
)
- intervalQualifier = IntervalQualifierStart() {
+ intervalQualifier = TimeUnitOrName() {
if (sign == -1) {
Review Comment:
Is this change still required?
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -7913,7 +7913,50 @@ private static void checkArrayConcatAggFuncFails(SqlOperatorFixture t) {
"2015-01-01 00:00:00", "TIMESTAMP(0) NOT NULL");
}
- @Test void testDenseRankFunc() {
+ @Test void testBigQueryTimestampAdd() {
+ final SqlOperatorFixture nonBigQuery = fixture()
+ .setFor(SqlLibraryOperators.TIMESTAMP_ADD_BIG_QUERY);
+ nonBigQuery.checkFails("^timestamp_add(timestamp '2008-12-25 15:30:00', "
+ + "interval 5 minute)^",
+ "No match found for function signature "
+ + "TIMESTAMP_ADD\\(<TIMESTAMP>, <INTERVAL_DAY_TIME>\\)", false);
+
+ final SqlOperatorFixture f = fixture()
+ .withLibrary(SqlLibrary.BIG_QUERY)
+ .setFor(SqlLibraryOperators.TIMESTAMP_ADD_BIG_QUERY);
+ f.checkScalar("timestamp_add(timestamp '2008-12-25 15:30:00', "
+ + "interval 5000000000 nanosecond)",
+ "2008-12-25 15:30:05",
+ "TIMESTAMP(0) NOT NULL");
+ f.checkScalar(
+ "timestamp_add(timestamp '2008-12-25 15:30:00', interval 100000000000 microsecond)",
+ "2008-12-26 19:16:40",
+ "TIMESTAMP(3) NOT NULL");
+ f.checkScalar("timestamp_add(timestamp '2008-12-25 15:30:00', interval 100000000 millisecond)",
+ "2008-12-26 19:16:40",
+ "TIMESTAMP(3) NOT NULL");
+ f.checkScalar("timestamp_add(timestamp '2016-02-24 12:42:25', interval 2 second)",
+ "2016-02-24 12:42:27",
+ "TIMESTAMP(0) NOT NULL");
+ f.checkScalar("timestamp_add(timestamp '2016-02-24 12:42:25', interval 2 minute)",
+ "2016-02-24 12:44:25",
+ "TIMESTAMP(0) NOT NULL");
+ f.checkScalar("timestamp_add(timestamp '2016-02-24 12:42:25', interval -2000 hour)",
+ "2015-12-03 04:42:25",
+ "TIMESTAMP(0) NOT NULL");
+ f.checkScalar("timestamp_add(timestamp '2016-02-24 12:42:25', interval 1 day)",
+ "2016-02-25 12:42:25",
+ "TIMESTAMP(0) NOT NULL");
+ f.checkScalar("timestamp_add(timestamp '2016-02-24 12:42:25', interval 1 month)",
+ "2016-03-24 12:42:25",
+ "TIMESTAMP(0) NOT NULL");
+ f.checkScalar("timestamp_add(timestamp '2016-02-24 12:42:25', interval 1 year)",
+ "2017-02-24 12:42:25",
+ "TIMESTAMP(0) NOT NULL");
+ f.checkNull("timestamp_add(CAST(NULL AS TIMESTAMP), interval 5 minute)");
+ }
+
+ @Test void testDenseRankFunc() {
Review Comment:
fix the indent of `testDenseRankFunc`. then the diff will look better, and checkstyle will pass
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -634,6 +634,13 @@ private SqlLibraryOperators() {
ImmutableSet.<TimeUnitRange>builder()
.addAll(MONTH_UNITS).addAll(DATE_UNITS).addAll(TIME_UNITS).build();
+ /** The "TIMESTAMP_ADD(timestamp_expression, INTERVAL int64_expression date_part)"
+ * function (BigQuery); Adds int64_expression units of date_part to the timestamp,
+ * independent of any time zone.*/
+ @LibraryOperator(libraries = {BIG_QUERY})
+ public static final SqlFunction TIMESTAMP_ADD_BIG_QUERY =
+ new SqlTimestampAddBigQueryFunction("TIMESTAMP_ADD");
+
Review Comment:
Interval is built-in in Calcite, so the syntax description is just "TIMESTAMP_ADD(timestamp, interval)".
I would name the function TIMESTAMP_ADD2 and describe it as 'The 2-argument TIMESTAMP_ADD function, as in BigQuery, as opposed to the 3-argument TIMESTAMPADD JDBC and builtin function"
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -634,6 +634,13 @@ private SqlLibraryOperators() {
ImmutableSet.<TimeUnitRange>builder()
.addAll(MONTH_UNITS).addAll(DATE_UNITS).addAll(TIME_UNITS).build();
+ /** The "TIMESTAMP_ADD(timestamp_expression, INTERVAL int64_expression date_part)"
+ * function (BigQuery); Adds int64_expression units of date_part to the timestamp,
+ * independent of any time zone.*/
+ @LibraryOperator(libraries = {BIG_QUERY})
+ public static final SqlFunction TIMESTAMP_ADD_BIG_QUERY =
+ new SqlTimestampAddBigQueryFunction("TIMESTAMP_ADD");
Review Comment:
I don't think you need `class SqlTimestampAddBigQueryFunction` at all. Because this function doesn't support custom time frames, you can remove the logic for those, and the type resolution gets simpler. I think
```
SqlBasicFunction.create(SqlKind.TIMESTAMP_ADD, ReturnTypes.ARG0_NULLABLE,
OperandTypes.TIMESTAMP_INTERVAL)
.withFunctionType(SqlFunctionCategory.TIMEDATE)
```
is a sufficient definition.
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -7913,7 +7913,50 @@ private static void checkArrayConcatAggFuncFails(SqlOperatorFixture t) {
"2015-01-01 00:00:00", "TIMESTAMP(0) NOT NULL");
}
- @Test void testDenseRankFunc() {
+ @Test void testBigQueryTimestampAdd() {
+ final SqlOperatorFixture nonBigQuery = fixture()
+ .setFor(SqlLibraryOperators.TIMESTAMP_ADD_BIG_QUERY);
+ nonBigQuery.checkFails("^timestamp_add(timestamp '2008-12-25 15:30:00', "
+ + "interval 5 minute)^",
+ "No match found for function signature "
+ + "TIMESTAMP_ADD\\(<TIMESTAMP>, <INTERVAL_DAY_TIME>\\)", false);
+
+ final SqlOperatorFixture f = fixture()
+ .withLibrary(SqlLibrary.BIG_QUERY)
+ .setFor(SqlLibraryOperators.TIMESTAMP_ADD_BIG_QUERY);
+ f.checkScalar("timestamp_add(timestamp '2008-12-25 15:30:00', "
Review Comment:
I don't think Calcite supports NANOSECOND intervals right now (maybe not MICROSECOND either). You should disable this statement with an `if` and log a Calcite bug to track.
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -7913,7 +7913,50 @@ private static void checkArrayConcatAggFuncFails(SqlOperatorFixture t) {
"2015-01-01 00:00:00", "TIMESTAMP(0) NOT NULL");
}
- @Test void testDenseRankFunc() {
+ @Test void testBigQueryTimestampAdd() {
Review Comment:
In addition to this test, you should remove the `!if (false) {` ... `}` around the `TIMESTAMP_ADD` test in `big-query.iq`.
--
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