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 2021/11/23 02:59:12 UTC

[GitHub] [incubator-doris] littleeleventhwolf opened a new pull request #7183: [BUG] Fix `curdate()+0` behave like mysql

littleeleventhwolf opened a new pull request #7183:
URL: https://github.com/apache/incubator-doris/pull/7183


   ## Proposed changes
   
   I proposed this point in [PR-6005](https://github.com/apache/incubator-doris/pull/6005).
   Now I try to make `curdate()+0` behave like mysql.
   
   ## Types of changes
   
   What types of changes does your code introduce to Doris?
   _Put an `x` in the boxes that apply_
   
   - [ ] Bugfix (non-breaking change which fixes an issue)
   - [ ] New feature (non-breaking change which adds functionality)
   - [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
   - [ ] Documentation Update (if none of the other choices apply)
   - [ ] Code refactor (Modify the code structure, format the code, etc...)
   - [ ] Optimization. Including functional usability improvements and performance improvements.
   - [ ] Dependency. Such as changes related to third-party components.
   - [ ] Other.
   
   ## Checklist
   
   _Put an `x` in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code._
   
   - [ ] I have created an issue on (Fix #ISSUE) and described the bug/feature there in detail
   - [x] Compiling and unit tests pass locally with my changes
   - [ ] I have added tests that prove my fix is effective or that my feature works
   - [ ] If these changes need document changes, I have updated the document
   - [ ] Any dependent changes have been merged
   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] littleeleventhwolf commented on a change in pull request #7183: [BUG] Fix `curdate()+0` to behave like mysql

Posted by GitBox <gi...@apache.org>.
littleeleventhwolf commented on a change in pull request #7183:
URL: https://github.com/apache/incubator-doris/pull/7183#discussion_r757040757



##########
File path: fe/fe-core/src/main/java/org/apache/doris/analysis/DateLiteral.java
##########
@@ -361,8 +368,18 @@ public int compareLiteral(LiteralExpr expr) {
         if (expr == MaxLiteral.MAX_VALUE) {
             return -1;
         }
+
+        long thisExprLongValue = getLongValue();
+        long otherExprLongValue = expr.getLongValue();
+        if (!getType().equals(expr.getType())) {
+            if (getType().equals(Type.DATE)) {

Review comment:
       Thank you for reminding.
   `Expr` could be other type besides `DATE` and `DATETIME`. I find the constructor or `init` method separately carry out `DATE` type. Other types are treated as `DATETIME` indeed, even though the assigned type is not `DATETIME`.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] morningman commented on a change in pull request #7183: [BUG] Fix `curdate()+0` to behave like mysql

Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #7183:
URL: https://github.com/apache/incubator-doris/pull/7183#discussion_r757006435



##########
File path: fe/fe-core/src/main/java/org/apache/doris/analysis/DateLiteral.java
##########
@@ -361,8 +368,18 @@ public int compareLiteral(LiteralExpr expr) {
         if (expr == MaxLiteral.MAX_VALUE) {
             return -1;
         }
+
+        long thisExprLongValue = getLongValue();
+        long otherExprLongValue = expr.getLongValue();
+        if (!getType().equals(expr.getType())) {
+            if (getType().equals(Type.DATE)) {

Review comment:
       Are you sure the `expr` is either `DATE` or `DATETIME` type?
   May it be a Long value? for example.

##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/cache/PartitionRange.java
##########
@@ -48,6 +48,8 @@
 import java.util.List;
 import java.util.Map;
 
+import static org.apache.doris.analysis.DateLiteral.BASIC_ZERO_TIME;

Review comment:
       not using static import:
   http://doris.incubator.apache.org/master/zh-CN/developer-guide/java-format-code.html#import-order

##########
File path: fe/fe-core/src/test/java/org/apache/doris/planner/QueryPlanTest.java
##########
@@ -17,6 +17,7 @@
 
 package org.apache.doris.planner;
 
+import com.google.common.base.Splitter;

Review comment:
       import order:
   http://doris.incubator.apache.org/master/zh-CN/developer-guide/java-format-code.html#import-order




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] littleeleventhwolf commented on a change in pull request #7183: [BUG] Fix `curdate()+0` to behave like mysql

Posted by GitBox <gi...@apache.org>.
littleeleventhwolf commented on a change in pull request #7183:
URL: https://github.com/apache/incubator-doris/pull/7183#discussion_r759830022



##########
File path: fe/fe-core/src/main/java/org/apache/doris/analysis/DateLiteral.java
##########
@@ -361,8 +368,18 @@ public int compareLiteral(LiteralExpr expr) {
         if (expr == MaxLiteral.MAX_VALUE) {
             return -1;
         }
+
+        long thisExprLongValue = getLongValue();
+        long otherExprLongValue = expr.getLongValue();
+        if (!getType().equals(expr.getType())) {
+            if (getType().equals(Type.DATE)) {

Review comment:
       Could you please give me some advice on how to process other types reasonably?
   What I can think of now is to process `DATE` type separately, because the constructor or init-method or getLongValue-method only process `DATE` type separately too. And other types (e.g. LONG), the assigned type is not `DATEIME`, but in fact they are still processed as `DATETIME`.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] littleeleventhwolf commented on a change in pull request #7183: [BUG] Fix `curdate()+0` to behave like mysql

Posted by GitBox <gi...@apache.org>.
littleeleventhwolf commented on a change in pull request #7183:
URL: https://github.com/apache/incubator-doris/pull/7183#discussion_r756215416



##########
File path: fe/fe-core/src/main/java/org/apache/doris/analysis/DateLiteral.java
##########
@@ -361,8 +361,18 @@ public int compareLiteral(LiteralExpr expr) {
         if (expr == MaxLiteral.MAX_VALUE) {
             return -1;
         }
+
+        long thisExprLongValue = getLongValue();
+        long otherExprLongValue = expr.getLongValue();
+        if (!getType().equals(expr.getType())) {
+            if (getType().equals(Type.DATE)) {
+                thisExprLongValue *= 1000000;

Review comment:
       Thanks for your reply.
   I have modified it according to your suggestion.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] morningman commented on a change in pull request #7183: [BUG] Fix `curdate()+0` to behave like mysql

Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #7183:
URL: https://github.com/apache/incubator-doris/pull/7183#discussion_r759843860



##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/cache/PartitionRange.java
##########
@@ -238,7 +238,11 @@ public long realValue() {
         }
 
         private Date getDateValue(LiteralExpr expr) {
-            value = expr.getLongValue() / 1000000;
+            if (expr.getType().equals(Type.DATE)) {
+                value = expr.getLongValue();
+            } else {
+                value = expr.getLongValue() / DateLiteral.BASIC_ZERO_TIME;

Review comment:
       You already handle the type in `DateLiteral.getLongValue()`, why divide `BASIC_ZERO_TIME` here again?




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] morningman commented on a change in pull request #7183: [BUG] Fix `curdate()+0` to behave like mysql

Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #7183:
URL: https://github.com/apache/incubator-doris/pull/7183#discussion_r755799838



##########
File path: fe/fe-core/src/main/java/org/apache/doris/analysis/DateLiteral.java
##########
@@ -361,8 +361,18 @@ public int compareLiteral(LiteralExpr expr) {
         if (expr == MaxLiteral.MAX_VALUE) {
             return -1;
         }
+
+        long thisExprLongValue = getLongValue();
+        long otherExprLongValue = expr.getLongValue();
+        if (!getType().equals(expr.getType())) {
+            if (getType().equals(Type.DATE)) {
+                thisExprLongValue *= 1000000;

Review comment:
       Better define this `1000000` as a final static variable, and explain what this number's mean. 




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] littleeleventhwolf commented on a change in pull request #7183: [BUG] Fix `curdate()+0` to behave like mysql

Posted by GitBox <gi...@apache.org>.
littleeleventhwolf commented on a change in pull request #7183:
URL: https://github.com/apache/incubator-doris/pull/7183#discussion_r757043811



##########
File path: fe/fe-core/src/main/java/org/apache/doris/analysis/DateLiteral.java
##########
@@ -361,8 +368,18 @@ public int compareLiteral(LiteralExpr expr) {
         if (expr == MaxLiteral.MAX_VALUE) {
             return -1;
         }
+
+        long thisExprLongValue = getLongValue();
+        long otherExprLongValue = expr.getLongValue();
+        if (!getType().equals(expr.getType())) {
+            if (getType().equals(Type.DATE)) {

Review comment:
       Thank you for reminding.
   `Expr` could be other type besides `DATE` and `DATETIME`. I find the constructor or `init` method separately carry out `DATE` type. Other types are treated as `DATETIME` indeed, even though the assigned type is not `DATETIME`.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] littleeleventhwolf commented on a change in pull request #7183: [BUG] Fix `curdate()+0` to behave like mysql

Posted by GitBox <gi...@apache.org>.
littleeleventhwolf commented on a change in pull request #7183:
URL: https://github.com/apache/incubator-doris/pull/7183#discussion_r760247210



##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/cache/PartitionRange.java
##########
@@ -238,7 +238,11 @@ public long realValue() {
         }
 
         private Date getDateValue(LiteralExpr expr) {
-            value = expr.getLongValue() / 1000000;
+            if (expr.getType().equals(Type.DATE)) {
+                value = expr.getLongValue();
+            } else {
+                value = expr.getLongValue() / DateLiteral.BASIC_ZERO_TIME;

Review comment:
       In `PartitionRange` class, the first parameter `type` of init-method cannot represent the second parameter `expr`'s real type, as illustrated in following figure:
   ![image](https://user-images.githubusercontent.com/20592822/144253131-dc2acc21-55f7-4966-b613-9b26a87b42e8.png)
    
   So I reset to last commit, still handle the `expr`'s type with if-else statement, non-`DATE` type should divide `BASIC_ZERO_TIME`.
   
   What about your opinion?




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org

[GitHub] [incubator-doris] littleeleventhwolf commented on a change in pull request #7183: [BUG] Fix `curdate()+0` to behave like mysql

Posted by GitBox <gi...@apache.org>.
littleeleventhwolf commented on a change in pull request #7183:
URL: https://github.com/apache/incubator-doris/pull/7183#discussion_r759852289



##########
File path: fe/fe-core/src/main/java/org/apache/doris/qe/cache/PartitionRange.java
##########
@@ -238,7 +238,11 @@ public long realValue() {
         }
 
         private Date getDateValue(LiteralExpr expr) {
-            value = expr.getLongValue() / 1000000;
+            if (expr.getType().equals(Type.DATE)) {
+                value = expr.getLongValue();
+            } else {
+                value = expr.getLongValue() / DateLiteral.BASIC_ZERO_TIME;

Review comment:
       Yeah, you're right. Method `getDateValue` is only invoked in the case of `DATE`, the division is unnecessary.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org