-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CALCITE-5761] Handle DECADE, CENTURY, and MILLENNIUM for DATE_TRUNC #3253
Conversation
8a5cf83
to
cf7f71b
Compare
final Expression dayOperand0 = | ||
preFloor ? call(operand0, type, TimeUnit.DAY) : operand0; | ||
return Expressions.call(floorMethod, | ||
translator.getLiteral(operand1), dayOperand0); | ||
default: | ||
if (call.op.getName().equals("DATE_TRUNC")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this behavior for DATE_TRUNC
and not other functions? (e.g. TIMESTAMP_TRUNC
, DATETIME_TRUNC
)
Is it possible to add a test for this behavior?
Probably good to add SqlKind.DATE_TRUNC
. Switching on function name is not maintainable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested yet the behavior of TIMESTAMP functions.
This is strictly addressing CALCITE-5761.
It is possible that the timestamp functions have problems too.
Are you suggesting to add SqlKind.DATE_TRUNC in this PR, or should I file an issue for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested yet the behavior of TIMESTAMP functions.
The other functions seem so closely related, it's strange to not do them.
Are you suggesting to add SqlKind.DATE_TRUNC in this PR, or should I file an issue for that?
Yes, add SqlKind.DATE_TRUNC in this PR. Clean up tech debt as you create it. (Not that I'm criticizing you... the DATE_TRUNC function didn't need its own SqlKind previously, so adding one would be over-engineering, but now it does.)
@@ -9641,6 +9641,12 @@ void testTimestampDiff(boolean coercionEnabled) { | |||
"2015-01-01", "DATE NOT NULL"); | |||
f.checkScalar("date_trunc(date '2015-02-19', isoyear)", | |||
"2014-12-29", "DATE NOT NULL"); | |||
f.checkScalar("date_trunc(date '2015-02-19', decade)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment that DECADE rolls down to 0 mod 10, CENTURY and MILLENNIUM roll down to 1 mod 100 and 1 mod 1000. That behavior is surprising to some people.
Signed-off-by: Mihai Budiu <mbudiu@gmail.com>
Signed-off-by: Mihai Budiu <mbudiu@gmail.com>
Signed-off-by: Mihai Budiu <mbudiu@gmail.com>
Kudos, SonarCloud Quality Gate passed! |
…TE_TRUNC, TIMESTAMP_TRUNC, DATETIME_TRUNC functions Close apache#3253 Signed-off-by: Mihai Budiu <mbudiu@gmail.com>
…TE_TRUNC, TIMESTAMP_TRUNC, DATETIME_TRUNC functions Close apache#3253 Signed-off-by: Mihai Budiu <mbudiu@gmail.com>
…TE_TRUNC, TIMESTAMP_TRUNC, DATETIME_TRUNC functions Close apache#3253 Signed-off-by: Mihai Budiu <mbudiu@gmail.com>
…TE_TRUNC, TIMESTAMP_TRUNC, DATETIME_TRUNC functions Close apache#3253 Signed-off-by: Mihai Budiu <mbudiu@gmail.com>
No description provided.