-
Notifications
You must be signed in to change notification settings - Fork 409
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
Support push function concat
, concat_ws
, datediff
, year
, day
, unix_timestamp
down to TiFlash.
#1588
Conversation
Maybe add some unit tests or integration tests(port from TiDB)? |
@@ -1303,13 +1303,11 @@ class FunctionTiDBTimestampDiff : public IFunction | |||
throw Exception("First argument for function " + getName() + " (unit) must be String", | |||
ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT); | |||
|
|||
if(!checkDataType<DataTypeMyDateTime>(removeNullable(arguments[1]).get()) && | |||
!checkDataType<DataTypeMyDate>(removeNullable(arguments[1]).get())) | |||
if(!removeNullable(arguments[1])->isDateOrDateTime()) |
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 change this? DataTypeDate
, DataTypeDatetime
is not supported yet.
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.
can we assume that this TiDB
Function will never accept the non-TiDB
datatype?
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 thought we won't build DataTypeDate
or DataTypeDateTime
arguments for this function anyway?
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.
But we do not delete DataTypeDate
and DataTypeDateTime
, I think it's better to throw error explicitly if the function does not support DataTypeDate
and DataTypeDateTime
throw Exception("Second argument for function " + getName() + " must be MyDate or MyDateTime", | ||
ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT); | ||
|
||
if(!checkDataType<DataTypeMyDateTime>(removeNullable(arguments[2]).get()) && | ||
!checkDataType<DataTypeMyDate>(removeNullable(arguments[2]).get())) | ||
if(!removeNullable(arguments[2])->isDateOrDateTime()) |
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.
ditto.
/run-all-tests |
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.
Should add some tests for this.
@@ -1303,13 +1303,11 @@ class FunctionTiDBTimestampDiff : public IFunction | |||
throw Exception("First argument for function " + getName() + " (unit) must be String", | |||
ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT); | |||
|
|||
if(!checkDataType<DataTypeMyDateTime>(removeNullable(arguments[1]).get()) && | |||
!checkDataType<DataTypeMyDate>(removeNullable(arguments[1]).get())) | |||
if(!removeNullable(arguments[1])->isDateOrDateTime()) |
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.
But we do not delete DataTypeDate
and DataTypeDateTime
, I think it's better to throw error explicitly if the function does not support DataTypeDate
and DataTypeDateTime
concat
, concat_ws
, datediff
, year
, day
, unix_timestamp
down to TiFlash.
What problem does this PR solve?
Issue Number: partially #1480
What is changed and how it works?
already supported:
opened
supported
Related changes
pingcap/docs
/pingcap/docs-cn
: need update push down docsCheck List
Tests
Release note