Skip to content
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-5864] Convert interval weeks and quarters into the correct millisecond total #3329

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jsternberg
Copy link

The week and quarter intervals were added but did not correctly convert
into the correct number of milliseconds when evaluated.

These now correctly translate a week to 7 days and a quarter to 3
months.

…illisecond total

The week and quarter intervals were added but did not correctly convert
into the correct number of milliseconds when evaluated.

These now correctly translate a week to 7 days and a quarter to 3
months.
@jsternberg
Copy link
Author

Figured out how to add some unit tests for this. I also have a comment on the original issue about why the numbers are intended to be these numbers: https://issues.apache.org/jira/browse/CALCITE-5864?focusedCommentId=17746626&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17746626

@sonarcloud
Copy link

sonarcloud bot commented Jul 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@njriasan
Copy link
Contributor

njriasan commented Aug 3, 2023

@jsternberg Wanted to comment an issue with this approach I found in case you still care about this. Inside RexLiteral the intervalString method uses the typeName information directly instead of the actual qualifier in getTimeUnits(). As a result, if you try convert from Rex to SQL, you will accidentally multiply the underlying value (by 7 for week and 3 for quarter).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants