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/12 01:55:33 UTC
[GitHub] [calcite] julianhyde commented on a diff in pull request #3000: [CALCITE-5423] Implement TIMESTAMP_DIFF function (compatible with BigQuery)
julianhyde commented on code in PR #3000:
URL: https://github.com/apache/calcite/pull/3000#discussion_r1045344772
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlTimestampDiffFunction.java:
##########
@@ -94,7 +105,10 @@ private static RelDataType inferReturnType2(SqlOperatorBinding opBinding) {
// with startUnit = EPOCH and timeFrameName = 'MINUTE15'.
//
// If the latter, check that timeFrameName is valid.
- validator.validateTimeFrame(
- (SqlIntervalQualifier) call.getOperandList().get(0));
+ if (call.operand(2) instanceof SqlIntervalQualifier) {
+ validator.validateTimeFrame((SqlIntervalQualifier) call.operand(2));
+ } else {
+ validator.validateTimeFrame((SqlIntervalQualifier) call.operand(0));
+ }
Review Comment:
is the comment above valid? If not, fix it.
##########
core/src/main/codegen/templates/Parser.jj:
##########
@@ -6588,6 +6590,31 @@ SqlCall TimestampDiffFunctionCall() :
}
}
+/**
+ * Parses a call to TIMESTAMP_DIFF
+ */
+/**
+ * Parses a call to TIME_TRUNC.
+ */
+SqlCall TimestampDiffBQFunctionCall() :
Review Comment:
Remove TIME_TRUNC javadoc. Clarify the the difference between TIMESTAMP_DIFF and TIMESTAMPDIFF.
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlTimestampDiffFunction.java:
##########
@@ -61,23 +61,34 @@
class SqlTimestampDiffFunction extends SqlFunction {
private static RelDataType inferReturnType2(SqlOperatorBinding opBinding) {
final RelDataTypeFactory typeFactory = opBinding.getTypeFactory();
- TimeUnit timeUnit = opBinding.getOperandLiteralValue(0, TimeUnit.class);
+ TimeUnit timeUnit;
+ RelDataType type1;
+ RelDataType type2;
+ if (opBinding.isOperandLiteral(0, true)) {
+ type1 = opBinding.getOperandType(0);
+ type2 = opBinding.getOperandType(1);
+ timeUnit = opBinding.getOperandLiteralValue(2, TimeUnit.class);
+ } else {
+ timeUnit = opBinding.getOperandLiteralValue(0, TimeUnit.class);
+ type1 = opBinding.getOperandType(1);
+ type2 = opBinding.getOperandType(2);
+ }
SqlTypeName sqlTypeName =
timeUnit == TimeUnit.NANOSECOND
? SqlTypeName.BIGINT
: SqlTypeName.INTEGER;
return typeFactory.createTypeWithNullability(
typeFactory.createSqlType(sqlTypeName),
- opBinding.getOperandType(1).isNullable()
- || opBinding.getOperandType(2).isNullable());
+ type1.isNullable()
+ || type2.isNullable());
}
/** Creates a SqlTimestampDiffFunction. */
SqlTimestampDiffFunction(String name) {
super(name, SqlKind.TIMESTAMP_DIFF,
SqlTimestampDiffFunction::inferReturnType2, null,
- OperandTypes.family(SqlTypeFamily.ANY, SqlTypeFamily.DATETIME,
- SqlTypeFamily.DATETIME),
+ OperandTypes.family(SqlTypeFamily.ANY, SqlTypeFamily.TIMESTAMP, SqlTypeFamily.TIMESTAMP)
+ .or(OperandTypes.family(SqlTypeFamily.TIMESTAMP, SqlTypeFamily.TIMESTAMP, SqlTypeFamily.ANY)),
Review Comment:
Can you also make sure checkstyle passes.
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -7768,6 +7772,35 @@ private static void checkArrayConcatAggFuncFails(SqlOperatorFixture t) {
f.checkNull("timestamp_add(CAST(NULL AS TIMESTAMP), interval 5 minute)");
}
+ @Test void testBigQueryTimestampDiff() {
Review Comment:
For the benefit of future maintainers, it's worth spelling out the difference (and similarity) between BQ TIMESTAMP_DIFF and regular TIMESTAMPDIFF.
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -7534,6 +7534,10 @@ private static void checkArrayConcatAggFuncFails(SqlOperatorFixture t) {
f.checkScalar("timestampdiff(\"month4\", date '2016-02-24', "
+ "date '2016-02-23')",
"0", "INTEGER NOT NULL");
+ f.withLibrary(SqlLibrary.BIG_QUERY).setFor(SqlLibraryOperators.TIMESTAMP_DIFF)
+ .checkScalar("timestamp_diff(timestamp '2008-12-25 15:30:00', "
+ + "timestamp '2008-12-25 16:30:00', \"minute15\")",
+ "4", "INTEGER NOT NULL");
Review Comment:
do you still want this here?
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -7768,6 +7772,35 @@ private static void checkArrayConcatAggFuncFails(SqlOperatorFixture t) {
f.checkNull("timestamp_add(CAST(NULL AS TIMESTAMP), interval 5 minute)");
}
+ @Test void testBigQueryTimestampDiff() {
+ final SqlOperatorFixture f = fixture()
+ .withLibrary(SqlLibrary.BIG_QUERY)
+ .setFor(SqlLibraryOperators.TIMESTAMP_DIFF);
+ f.checkScalar("timestamp_diff(timestamp '2008-12-25 15:30:00', timestamp '2008-12-25 15:35:00', second)",
+ "300",
+ "INTEGER NOT NULL");
+ f.checkScalar("timestamp_diff(timestamp '2008-12-25 15:30:00', timestamp '2008-12-25 15:35:00', minute)",
+ "5",
+ "INTEGER NOT NULL");
+ f.checkScalar("timestamp_diff(timestamp '2008-12-25 15:30:00', timestamp '2008-12-26 15:30:00', hour)",
+ "24",
+ "INTEGER NOT NULL");
+ f.checkScalar("timestamp_diff(timestamp '2008-12-25 15:30:00', timestamp '2008-12-26 15:30:00', day)",
+ "1",
+ "INTEGER NOT NULL");
+ f.checkScalar("timestamp_diff(timestamp '2008-12-25 15:30:00', timestamp '2008-5-25 15:30:00', month)",
+ "-7",
+ "INTEGER NOT NULL");
+ f.checkScalar("timestamp_diff(timestamp '2008-12-25 15:30:00', timestamp '2009-01-01 15:30:00', week)",
+ "1",
+ "INTEGER NOT NULL");
+ f.checkScalar("timestamp_diff(timestamp '2008-12-25 15:30:00', timestamp '2011-12-25 15:30:00', year)",
+ "3",
+ "INTEGER NOT NULL");
+ f.checkScalar("timestamp_diff(timestamp '2008-12-25 15:30:00', cast(null as timestamp), day)",
+ isNullValue(),
+ "INTEGER");
+ }
Review Comment:
blank line between methods
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlTimestampDiffFunction.java:
##########
@@ -61,23 +61,34 @@
class SqlTimestampDiffFunction extends SqlFunction {
private static RelDataType inferReturnType2(SqlOperatorBinding opBinding) {
final RelDataTypeFactory typeFactory = opBinding.getTypeFactory();
- TimeUnit timeUnit = opBinding.getOperandLiteralValue(0, TimeUnit.class);
+ TimeUnit timeUnit;
+ RelDataType type1;
+ RelDataType type2;
+ if (opBinding.isOperandLiteral(0, true)) {
+ type1 = opBinding.getOperandType(0);
+ type2 = opBinding.getOperandType(1);
+ timeUnit = opBinding.getOperandLiteralValue(2, TimeUnit.class);
+ } else {
+ timeUnit = opBinding.getOperandLiteralValue(0, TimeUnit.class);
+ type1 = opBinding.getOperandType(1);
+ type2 = opBinding.getOperandType(2);
+ }
SqlTypeName sqlTypeName =
timeUnit == TimeUnit.NANOSECOND
? SqlTypeName.BIGINT
: SqlTypeName.INTEGER;
return typeFactory.createTypeWithNullability(
typeFactory.createSqlType(sqlTypeName),
- opBinding.getOperandType(1).isNullable()
- || opBinding.getOperandType(2).isNullable());
+ type1.isNullable()
+ || type2.isNullable());
}
/** Creates a SqlTimestampDiffFunction. */
SqlTimestampDiffFunction(String name) {
super(name, SqlKind.TIMESTAMP_DIFF,
SqlTimestampDiffFunction::inferReturnType2, null,
- OperandTypes.family(SqlTypeFamily.ANY, SqlTypeFamily.DATETIME,
- SqlTypeFamily.DATETIME),
+ OperandTypes.family(SqlTypeFamily.ANY, SqlTypeFamily.TIMESTAMP, SqlTypeFamily.TIMESTAMP)
+ .or(OperandTypes.family(SqlTypeFamily.TIMESTAMP, SqlTypeFamily.TIMESTAMP, SqlTypeFamily.ANY)),
Review Comment:
Using `.or` was a nice idea. But I think it would be safer if you passed in the `OperandTypeChecker` as a parameter, along with the name. Then it's clearer that TIMESTAMPDIFF accepts (unit, TS, TS) and TIMESTAMP_DIFF accepts (TS, TS, unit).
##########
site/_docs/reference.md:
##########
@@ -2654,6 +2654,7 @@ semantics.
| m | STRCMP(string, string) | Returns 0 if both of the strings are same and returns -1 when the first argument is smaller than the second and 1 when the second one is smaller than the first one
| b o | TANH(numeric) | Returns the hyperbolic tangent of *numeric*
| b | TIMESTAMP_ADD(timestamp, interval int64 date_part) | Adds int64_expression units of date_part to the timestamp, independent of any time zone.
+| b | TIMESTAMP_DIFF(timestamp, timestamp2, timeUnit) | Returns the whole number of *timeUnit* between timestamp and timestamp2
| b | TIMESTAMP_MICROS(integer) | Returns the TIMESTAMP that is *integer* microseconds after 1970-01-01 00:00:00
Review Comment:
timestamp and timestamp2 need asterisks
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -7768,6 +7772,35 @@ private static void checkArrayConcatAggFuncFails(SqlOperatorFixture t) {
f.checkNull("timestamp_add(CAST(NULL AS TIMESTAMP), interval 5 minute)");
}
+ @Test void testBigQueryTimestampDiff() {
Review Comment:
You could just copy the `testTimestampDiff` and re-order the arguments, right? I think you should.
--
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