You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2020/03/27 07:40:55 UTC
[GitHub] [incubator-doris] HangyuanLiu opened a new pull request #3214: Fix
cast date type return wrong result
HangyuanLiu opened a new pull request #3214: Fix cast date type return wrong result
URL: https://github.com/apache/incubator-doris/pull/3214
For #3213
We have multiple date type, and we also need to cast between different date types.
If not cast, it will cause problems when binarypredicate
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] chaoyli commented on a change in pull request
#3214: Fix cast date type return wrong result
Posted by GitBox <gi...@apache.org>.
chaoyli commented on a change in pull request #3214: Fix cast date type return wrong result
URL: https://github.com/apache/incubator-doris/pull/3214#discussion_r401553351
##########
File path: fe/src/main/java/org/apache/doris/analysis/DateLiteral.java
##########
@@ -320,6 +320,13 @@ protected void toThrift(TExprNode msg) {
@Override
protected Expr uncheckedCastTo(Type targetType) throws AnalysisException {
if (targetType.isDateType()) {
+ if (targetType.equals(Type.DATE)) {
+ this.castToDate();
+ } else if (targetType.equals(Type.DATETIME)) {
+ this.type = Type.DATETIME;
Review comment:
If Date not to convert DateTime, is OK?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] kangkaisen commented on a change in pull request
#3214: Fix cast date type return wrong result
Posted by GitBox <gi...@apache.org>.
kangkaisen commented on a change in pull request #3214: Fix cast date type return wrong result
URL: https://github.com/apache/incubator-doris/pull/3214#discussion_r399163885
##########
File path: fe/src/test/java/org/apache/doris/analysis/DateLiteralTest.java
##########
@@ -55,4 +55,23 @@ public void TwoDigitYear() {
}
Assert.assertFalse(hasException);
}
+
+ @Test
+ public void uncheckedCastTo() {
Review comment:
Would better also add a test to `QueryPlanTest`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] kangkaisen commented on a change in pull request
#3214: Fix cast date type return wrong result
Posted by GitBox <gi...@apache.org>.
kangkaisen commented on a change in pull request #3214: Fix cast date type return wrong result
URL: https://github.com/apache/incubator-doris/pull/3214#discussion_r401338547
##########
File path: fe/src/main/java/org/apache/doris/analysis/DateLiteral.java
##########
@@ -319,7 +319,17 @@ protected void toThrift(TExprNode msg) {
@Override
protected Expr uncheckedCastTo(Type targetType) throws AnalysisException {
- if (targetType.equals(this.type)) {
+ if (targetType.isDateType()) {
Review comment:
If isDateType is true. The type could only be DATE and DATETIME.
So one `if else` if enough?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] kangkaisen commented on a change in pull request
#3214: Fix cast date type return wrong result
Posted by GitBox <gi...@apache.org>.
kangkaisen commented on a change in pull request #3214: Fix cast date type return wrong result
URL: https://github.com/apache/incubator-doris/pull/3214#discussion_r399163380
##########
File path: fe/src/main/java/org/apache/doris/analysis/DateLiteral.java
##########
@@ -319,7 +319,7 @@ protected void toThrift(TExprNode msg) {
@Override
protected Expr uncheckedCastTo(Type targetType) throws AnalysisException {
- if (targetType.isDateType()) {
+ if (targetType.equals(this.type)) {
Review comment:
Are you sure only change this point is enough？
Do we need to change this point:
FunctionCallExpr::analyzeImpl
```
Type[] args = fn.getArgs();
if (args.length > 0) {
// Implicitly cast all the children to match the function if necessary
for (int i = 0; i < argTypes.length; ++i) {
// For varargs, we must compare with the last type in callArgs.argTypes.
int ix = Math.min(args.length - 1, i);
if (!argTypes[i].matchesType(args[ix]) && !(
argTypes[i].isDateType() && args[ix].isDateType())) {
uncheckedCastChild(args[ix], i);
//if (argTypes[i] != args[ix]) castChild(args[ix], i);
}
}
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay merged pull request #3214: Fix cast date
type return wrong result
Posted by GitBox <gi...@apache.org>.
imay merged pull request #3214: Fix cast date type return wrong result
URL: https://github.com/apache/incubator-doris/pull/3214
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] HangyuanLiu commented on a change in pull request
#3214: Fix cast date type return wrong result
Posted by GitBox <gi...@apache.org>.
HangyuanLiu commented on a change in pull request #3214: Fix cast date type return wrong result
URL: https://github.com/apache/incubator-doris/pull/3214#discussion_r401677001
##########
File path: fe/src/main/java/org/apache/doris/analysis/DateLiteral.java
##########
@@ -320,6 +320,13 @@ protected void toThrift(TExprNode msg) {
@Override
protected Expr uncheckedCastTo(Type targetType) throws AnalysisException {
if (targetType.isDateType()) {
+ if (targetType.equals(Type.DATE)) {
+ this.castToDate();
+ } else if (targetType.equals(Type.DATETIME)) {
+ this.type = Type.DATETIME;
Review comment:
Yes, is ok。And explicit type conver(cast expr) also go to this line
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] imay commented on a change in pull request #3214:
Fix cast date type return wrong result
Posted by GitBox <gi...@apache.org>.
imay commented on a change in pull request #3214: Fix cast date type return wrong result
URL: https://github.com/apache/incubator-doris/pull/3214#discussion_r399890245
##########
File path: fe/src/main/java/org/apache/doris/analysis/DateLiteral.java
##########
@@ -319,7 +319,7 @@ protected void toThrift(TExprNode msg) {
@Override
protected Expr uncheckedCastTo(Type targetType) throws AnalysisException {
- if (targetType.isDateType()) {
+ if (targetType.equals(this.type)) {
Review comment:
Why not return a new DateLiteral when targetType and this.type are different but both dateType.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org