[CALCITE-6581] Incorrect INTERVAL math for WEEK and QUARTER #3968
Conversation
gianm
force-pushed
the
fix-week-quarter-math
branch
2 times, most recently
from
b0650a9
to
4997c2e
Compare
| @@ -746,7 +746,8 @@ private int[] evaluateIntervalLiteralAsQuarter( | |||
| checkLeadFieldInRange(typeSystem, sign, quarter, TimeUnit.QUARTER, pos); | |||
|
|
|||
| // package values up for return | |||
| return fillIntervalValueArray(sign, ZERO, quarter); | |||
| final BigDecimal months = quarter.multiply(BigDecimal.valueOf(3)); | |||
There was a problem hiding this comment.
I don't think quarters should be convertible to seconds, they do not have a fixed length
There was a problem hiding this comment.
They are being converted to 3 months here, since QUARTER is treated as a kind of YEAR-MONTH interval.
There was a problem hiding this comment.
sorry, I misread the JIRA issue
| /** | ||
| * Tests {@link SqlParserUtil}. | ||
| */ | ||
| public class SqlParserUtilTest { |
There was a problem hiding this comment.
SqlParserIntervalQualifierTest?
There was a problem hiding this comment.
I figured that at some point, this test file might be expanded to include more tests for SqlParserUtil, so I named it after the main class itself. However, please let me know if it's considered better to scope the test files more narrowly.
There was a problem hiding this comment.
It seems that the impact is not great.
I think we can add JavaDoc to let subsequent contributors know what this test file is for.
There was a problem hiding this comment.
I've added some additional javadoc.
|
Can you add a couple of end-to-end tests, either to SqlOperatorTest, or to one of the *.iq files? |
+1 |
Sure, I've added a test to |
WEEK and QUARTER intervals were incorrectly treated like HOUR and MONTH respectively. This patch fixes the handling, and adds unit tests. This patch also renames the two "fillIntervalValueArray" methods to "fillDayIntervalValueArray" (for INTERVAL_DAY_TIME type intervals) and "fillYearMonthIntervalValueArray" (for INTERVAL_YEAR_MONTH type intervals) to make it more clear which one is which.
gianm
force-pushed
the
fix-week-quarter-math
branch
from
795a01b
to
21f42cf
Compare
|
|
Committing; thank you for the reviews. |
|
Don't forget to mark the jira case as closed and to record the actual commit |
WEEK and QUARTER intervals were incorrectly treated like HOUR and MONTH respectively. This patch fixes the handling, and adds unit tests.
This patch also renames the two "fillIntervalValueArray" methods to "fillDayIntervalValueArray" (for INTERVAL_DAY_TIME type intervals) and "fillYearMonthIntervalValueArray" (for INTERVAL_YEAR_MONTH type intervals) to make it more clear which one is which.