[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.
2 participants
jira：https://issues.apache.org/jira/browse/CALCITE-7304