-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CALCITE-6581] Incorrect INTERVAL math for WEEK and QUARTER #3968
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think quarters should be convertible to seconds, they do not have a fixed length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I misread the JIRA issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok!
/** | ||
* Tests {@link SqlParserUtil}. | ||
*/ | ||
public class SqlParserUtilTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SqlParserIntervalQualifierTest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
795a01b
to
21f42cf
Compare
Quality Gate passedIssues Measures |
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.