[CALCITE-7304] Floor/Ceil can not simplify with WEEK TimeUnit #4651
Conversation
| return TimeUnitRange.MONTH; | ||
| default: | ||
| return TimeUnitRange.DAY; | ||
| return unit; |
There was a problem hiding this comment.
The modification here is to enable the test class RexInterpreter to support time units such as HOUR/MINUTE/SECOND， otherwise here would not support such as HOUR/MINUTE/SECOND time unit. Since this class is only for testing, I think the modification is acceptable.
There was a problem hiding this comment.
public static int unixTimestampExtract(TimeUnitRange range,
long timestamp) {
return unixTimeExtract(range,
(int) Math.floorMod(timestamp, MILLIS_PER_DAY));
}
I noticed a piece of code that requires the input to be in days. It seems that modifying it in this way may not be appropriate. I understand that this is not intended for testing purposes, but rather serves constant folding.
| if (inner == TimeUnit.QUARTER) { | ||
| return outer == TimeUnit.YEAR; | ||
| } | ||
| if (outer == TimeUnit.WEEK && inner != TimeUnit.YEAR && inner != TimeUnit.MONTH) { |
There was a problem hiding this comment.
This change is because the order of WEEK is after YEAR/MONTH/DAY/HOUR/MINUTE/SECOND in TimeUnit, so the inner and outer cases are handled separately.
There was a problem hiding this comment.
Although this is very old code, I still don't understand why the inner layer processing requires an additional check on the outer layer's type—including the case where inner == TimeUnit.QUARTER.
There was a problem hiding this comment.
Yes，I also noticed this historical code. Since there were no issues involving it, I didn't modify it, and this shouldn't cause any problems.
| } | ||
|
|
||
| /** Test case for simplifier of ceil/floor. */ | ||
| /** Test case for |
There was a problem hiding this comment.
With the PR SELECT floor(floor(\"hire_date\" TO DAY) TO WEEK) FROM \"employee\" would simplify to SELECT FLOOR(\"hire_date\" TO WEEK)\nFROM \"foodmart\".\"employee\"
|
| return TimeUnitRange.MONTH; | ||
| default: | ||
| return TimeUnitRange.DAY; | ||
| return unit; |
There was a problem hiding this comment.
public static int unixTimestampExtract(TimeUnitRange range,
long timestamp) {
return unixTimeExtract(range,
(int) Math.floorMod(timestamp, MILLIS_PER_DAY));
}
I noticed a piece of code that requires the input to be in days. It seems that modifying it in this way may not be appropriate. I understand that this is not intended for testing purposes, but rather serves constant folding.
| if (inner == TimeUnit.QUARTER) { | ||
| return outer == TimeUnit.YEAR; | ||
| } | ||
| if (outer == TimeUnit.WEEK && inner != TimeUnit.YEAR && inner != TimeUnit.MONTH) { |
There was a problem hiding this comment.
Although this is very old code, I still don't understand why the inner layer processing requires an additional check on the outer layer's type—including the case where inner == TimeUnit.QUARTER.
| * <a href="https://issues.apache.org/jira/browse/CALCITE-2178">[CALCITE-2178] | ||
| * Extend expression simplifier to work on datetime CEIL/FLOOR functions</a>, | ||
| * <a href="https://issues.apache.org/jira/browse/CALCITE-7304">[CALCITE-7304] | ||
| * Floor/Ceil can not simplify with WEEK TimeUnit</a>. */ |
There was a problem hiding this comment.
As I mentioned earlier regarding a potential issue, I think we may need an actual execution result to verify its correctness.
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions. |
2 participants
jira：https://issues.apache.org/jira/browse/CALCITE-7304