[CALCITE-5757] BigQuery DATE_TRUNC return type should be ARG0 and TIMESTAMP_TRUNC/DATETIME_TRUNC should return TIMESTAMP for DATE/TIMESTAMPs and TIMESTAMP_LTZ for TIMESTAMP_LTZ #3248
Conversation
tanclary
force-pushed
the
stripped-trunc-cast
branch
4 times, most recently
from
34723b2
to
e6f0e3f
Compare
tanclary
changed the title
tanclary
changed the title
| DATE_TRUNC(DATETIME "2008-12-25 15:30:00", MONTH) AS datetime_result; | ||
| timestamp_result TIMESTAMP_WITH_LOCAL_TIME_ZONE NOT NULL | ||
| datetime_result TIMESTAMP NOT NULL | ||
| !type |
There was a problem hiding this comment.
neat, I didnt know about !type in quidem tests
There was a problem hiding this comment.
yeah super useful!!
julianhyde
force-pushed
the
main
branch
2 times, most recently
from
8a5cf83
to
cf7f71b
Compare
| * Type-inference strategy that returns the type of the first operand, | ||
| * unless it is a DATE, in which case the return type is TIMESTAMP. | ||
| */ | ||
| public static final SqlReturnTypeInference ARG0_EXCEPT_DATE = opBinding -> { |
There was a problem hiding this comment.
Might be nice to add a comment referencing [CALCITE-5757] to remind that this is for BQ typing behavior
There was a problem hiding this comment.
Updated, let me know if it's clear enough
| final RexBuilder rexBuilder = cx.getRexBuilder(); | ||
| RexNode op1 = cx.convertExpression(call.operand(0)); | ||
| RexNode op2 = cx.convertExpression(call.operand(1)); | ||
| if (op1.getType().getSqlTypeName() == SqlTypeName.DATE) { |
There was a problem hiding this comment.
Maybe also a comment here discussion why DATE is handled separately
tanclary
force-pushed
the
stripped-trunc-cast
branch
from
e6f0e3f
to
a6769a9
Compare
|
The commit message is currently as follows:
That contains 'correct', which is a synonym for 'fix', and is superfluous. Just state the desired behavior. Would the following be better?
|
tanclary
changed the title
tanclary
changed the title
|
After you've removed & inlined |
…ESTAMP_TRUNC/DATETIME_TRUNC should return TIMESTAMP for DATE/TIMESTAMPs and TIMESTAMP_LTZ for TIMESTAMP_LTZ
tanclary
force-pushed
the
stripped-trunc-cast
branch
from
a6769a9
to
7a2af29
Compare
|
@julianhyde I updated it, also, DATE_TRUNC /should/ return ARG0 and not DATE (in case you wonder why commit message differs from your suggestion) |
|
Kudos, SonarCloud Quality Gate passed! |
BigQuery TRUNC functions follow the return type behavior described in CALCITE-5757. This PR adjusts the return type for those functions such that it matches the described behavior.
Notable changes include converting a
DATEto aTIMESTAMPfor theTIMESTAMP_TRUNCandDATETIME_TRUNCfunctions. It also ensures thatDATE_TRUNCreturn type is equivalent to the operand type (akaReturnTypes.ARG0).Prior to this PR, discrepancies included
DATETIME_TRUNCincorrectly returning aTIMESTAMP_LTZif given aTIMESTAMPargument andDATE_TRUNChaving aDATEreturn type even if the argument was aTIMESTAMPorTIMESTAMP_LTZ.@julianhyde I opted to just include the TRUNC functions in this PR to keep the review and scope fairly easy (hopefully).