[CALCITE-5360] Implement BigQuery TIMESTAMP_ADD#2998
[CALCITE-5360] Implement BigQuery TIMESTAMP_ADD#2998tanclary wants to merge 5 commits intoapache:mainfrom
Conversation
julianhyde
left a comment
julianhyde
left a comment
There was a problem hiding this comment.
A good start. I suggested some improvements in comments.
I edited the code to check whether my suggestions made sense, so feel free to build on my changes in https://github.com/julianhyde/calcite/tree/5360-timestamp-add.
| ) | ||
| intervalQualifier = IntervalQualifierStart() { | ||
| intervalQualifier = TimeUnitOrName() { | ||
| if (sign == -1) { |
There was a problem hiding this comment.
Is this change still required?
| @LibraryOperator(libraries = {BIG_QUERY}) | ||
| public static final SqlFunction TIMESTAMP_ADD_BIG_QUERY = | ||
| new SqlTimestampAddBigQueryFunction("TIMESTAMP_ADD"); | ||
|
|
There was a problem hiding this 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"
| f.checkNull("timestamp_add(CAST(NULL AS TIMESTAMP), interval 5 minute)"); | ||
| } | ||
|
|
||
| @Test void testDenseRankFunc() { |
There was a problem hiding this comment.
fix the indent of testDenseRankFunc. then the diff will look better, and checkstyle will pass
| } | ||
|
|
||
| @Test void testDenseRankFunc() { | ||
| @Test void testBigQueryTimestampAdd() { |
There was a problem hiding this comment.
In addition to this test, you should remove the !if (false) { ... } around the TIMESTAMP_ADD test in big-query.iq.
| * independent of any time zone.*/ | ||
| @LibraryOperator(libraries = {BIG_QUERY}) | ||
| public static final SqlFunction TIMESTAMP_ADD_BIG_QUERY = | ||
| new SqlTimestampAddBigQueryFunction("TIMESTAMP_ADD"); |
There was a problem hiding this 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.
| 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', " |
There was a problem hiding this 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.
enable TIMESTAMP_ADD test in big-query.iq back out unnecessary parser change lint use SqlBasicFunction.create, which makes class SqlTimestampAddBigQueryFunction unnecessary the convertlet doesn't need to know the library, just whether the function call had 2 or 3 arguments disable tests that use INTERVAL MICROSECONDS or NANOSECONDS; TODO: log bug and change 'if (false)' to reference the bug number
…ported time units
The JDBC and builtin functions already contain a TIMESTAMPADD function with three arguments; this change adds a TIMESTAMP_ADD function with two arguments, consistent with BigQuery, and enabled if you have 'lib=bigquery' in your connection options. Close apache#2998
2 participants
Implement BigQuery's TIMESTAMP_ADD. This implementation is similar to that of the standard timestampadd but accounts for the difference in operand number and type. JIRA issue https://issues.apache.org/jira/browse/CALCITE-5360 includes TIMESTAMP_DIFF as well but this PR is being opened primarily for feedback to avoid potential duplicate mistakes as TIMESTAMP_DIFF should follow a similar implementation.