-
Notifications
You must be signed in to change notification settings - Fork 998
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
[core] Support TimestampToNumericPrimitiveCastRule in casting #3854
Conversation
@Override | ||
public CastExecutor<Timestamp, Number> create(DataType inputType, DataType targetType) { | ||
if (inputType.is(DataTypeRoot.TIMESTAMP_WITHOUT_TIME_ZONE)) { | ||
return value -> DateTimeUtils.unixTimestamp(value.getMillisecond()); |
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.
Please add a case cast to and cast from.
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.
OK,had added cast to and from in test case @JingsongLi
6883e60
to
80e601a
Compare
return value -> DateTimeUtils.unixTimestamp(value.getMillisecond()); | ||
} else if (inputType.is(DataTypeRoot.TIMESTAMP_WITH_LOCAL_TIME_ZONE)) { | ||
return value -> | ||
DateTimeUtils.unixTimestamp( |
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.
For int, short, byte types, should not return long.
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.
Make sense,due to the number is hundreds million at least that Interge.Max also cannot hold the size,so I change to just handle Bigint condition in targetType and others return null. @JingsongLi
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.
integer is OK?
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.
And CastRulePredicate
should be only target to int and bigint
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.
Yes,should support int and bigint,I would change it latter. Thanks for the reminder. @JingsongLi
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.
+1
Purpose
Follow up #3832 had support NumericPrimitive to Timestamp casting,this pr support Timestamp to NumericPrimitive casting.
Linked issue: close #xxx
Tests
CastExecutorTest##testTimestampToNumeric
API and Format
Documentation