[CALCITE-6198] Add TO_TIMESTAMP_LTZ function (enabled in Snowflake li… #3631
Conversation
tanclary
force-pushed
the
6198-snowflake
branch
from
9179785
to
00ac109
Compare
|
Does CI stability failed? |
|
Seems that the "Error Prone" check is unhappy. |
| @@ -1610,6 +1610,36 @@ private static RelDataType deriveTypeMapFromEntries(SqlOperatorBinding opBinding | |||
| OperandTypes.STRING_STRING, | |||
| SqlFunctionCategory.TIMEDATE); | |||
|
|
|||
| /** The "TO_TIMESTAMP_LTZ" function returns a Calcite | |||
There was a problem hiding this comment.
It's worth spelling out that a TIMESTAMP WITH LOCAL TIME ZONE is stored as millis since UTC epoch.
Describe which time zone is used for conversion from DATE and TIMESTAMP.
| * <ul> | ||
| * <li>{@code TO_TIMESTAMP_LTZ(numeric[, scale)]} | ||
| * <li>{@code TO_TIMESTAMP_LTZ(date)} | ||
| * <li>{@code TO_TIMESTAMP_LTZ(datetime)} |
There was a problem hiding this comment.
datetime is not a Calcite type
site/_docs/reference.md
Outdated
| @@ -2746,7 +2746,7 @@ In the following: | |||
| | h s | FORMAT_NUMBER(value, decimalVal) | Formats the number *value* like '#,###,###.##', rounded to decimal places *decimalVal*. If *decimalVal* is 0, the result has no decimal point or fractional part | |||
| | h s | FORMAT_NUMBER(value, format) | Formats the number *value* to MySQL's FORMAT *format*, like '#,###,###.##0.00' | |||
| | b | FORMAT_TIME(string, time) | Formats *time* according to the specified format *string* | |||
| | b | FORMAT_TIMESTAMP(string timestamp) | Formats *timestamp* according to the specified format *string* | |||
| | b | FORMAT_TIMESTAMP(string, timestamp) | Formats *timestamp* according to the specified format *string* | |||
There was a problem hiding this comment.
remove space before bar
| @@ -457,6 +457,9 @@ public enum SqlKind { | |||
| /** {@code TIME_SUB} function (BigQuery). */ | |||
| TIME_SUB, | |||
|
|
|||
| /** {@code TIMESTAMP} function (BigQuery). */ | |||
| TIMESTAMP, | |||
There was a problem hiding this comment.
not a good name, since TIMESTAMP is a type
There was a problem hiding this comment.
Done, didn't need to add that anyways I don't think.
| return timestamp(timestamp); | ||
| } | ||
|
|
||
| /** SQL {@code TO_TIMESTAMP_LTZ(timestamp)} function |
There was a problem hiding this comment.
be explicit what the argument means. millis since local epoch? millis since UTC epoch?
using 'timestamp' in the function name would by default mean a Calcite timestamp, i.e. ltz. But then the function would be a no-op.
There was a problem hiding this comment.
I was under the impression a Calcite TIMESTAMP did not have a LTZ but TIMESTAMP WITH LOCAL TIME ZONE did. So if you provide the function a Calcite TIMESTAMP it would return a TIMESTAMP WITH LOCAL TIME ZONE (not a no-op).
| /** SQL {@code TO_TIMESTAMP_LTZ(date)} function | ||
| * for a date values. */ | ||
| public static long toTimestampLtzDate(int days) { | ||
| return timestamp(days); |
There was a problem hiding this comment.
having overloaded timestamp(int) and timestamp(long) functions with different semantics is error-prone
There was a problem hiding this comment.
Yeah I just was not sure how else to handle the fact that you can provide the function a DATE which gets converted to an int or you can provide the function with a numeric value (which could be a long).
| @@ -2480,6 +2484,32 @@ private static class RegexpReplaceImplementor extends AbstractRexCallImplementor | |||
| } | |||
| } | |||
|
|
|||
| /** Implementor for the {@code TO_TIMESTAMP_LTZ} function. */ | |||
There was a problem hiding this comment.
is an implementor necessary? it would be better if calls to TO_TIMESTAMP_LTZ were translated to existing functions. Then we would not need new methods in SqlFunctions
There was a problem hiding this comment.
The main reason I added the implementor was to avoid the "error proneness" you mentioned in your above comment. It checks the type of the first argument (i.e. DATE, TIMESTAMP, etc.) to ensure it does not call the wrong method via reflection.
Before I added the implementor, when I was trying to use TO_TIMESTAMP_LTZ(numeric), it would end up using the method for DATE arguments since dates are represented as integers. This way, that confusion is avoided.
| } | ||
|
|
||
| /** SQL {@code TO_TIMESTAMP_LTZ(timestampSeconds, scale)} | ||
| * function for long values with a specified scale. */ |
There was a problem hiding this comment.
Can you add examples? Does toTimestampLtz(86_400L, 0) mean TIMESTAMP '1970-01-02 00:00:00'?
What would toTimestampLtz(86_400L, 3) mean?
There was a problem hiding this comment.
Added a longer comment that explains the scale parameter better. Added an example too. Let me know what you think.
|
| f.checkScalar("to_timestamp_ltz(12345689.123, 2)", | ||
| "1970-01-02 10:17:36", | ||
| "TIMESTAMP_WITH_LOCAL_TIME_ZONE(0) NOT NULL"); | ||
| f.checkNull("to_timestamp_ltz(null, 2)"); |
There was a problem hiding this comment.
Hello，to_timestamp_ltz(null, null) will report an error?
…brary)