[
https://issues.apache.org/jira/browse/CALCITE-5864?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17750075#comment-17750075
]
Jonathan A Sternberg commented on CALCITE-5864:
-----------------------------------------------
I can perform that refactor, but I'd like to make sure I understand what you're
intending so I don't do it incorrectly.
My first question is should I modify this method or create a new
For the months pathway, it seems to return one format and the milliseconds
pathway it returns another. If I refactor things to use {{{}TimeUnit{}}},
what's the desired return value from this function? It seems like returning an
array might not be ideal. It seems like the only area where this method is used
is within {{intervalToMonths}} or {{{}intervalToMillis{}}}. It seems to me like
creating a new type and returning the months and milliseconds as part of that
type might be a better return value.
On the other hand, the {{evaluateIntervalLiteral}} method is public. It might
be good to keep it around and mark it as deprecated if I were to add a new
method of retrieving the values.
On an unrelated note, I'd also noticed that the way things are done doesn't
allow {{TimeUnit.MICROSECOND}} or {{{}TimeUnit.NANOSECOND{}}}. If I'm going to
spend the time refactoring this area of code, would you be ok with modifying
the result to support the full-breadth of possible time unit values?
> getValueAs reports the wrong number of milliseconds for WEEK and the wrong
> number of months for QUARTER
> -------------------------------------------------------------------------------------------------------
>
> Key: CALCITE-5864
> URL: https://issues.apache.org/jira/browse/CALCITE-5864
> Project: Calcite
> Issue Type: Bug
> Components: core
> Reporter: Jonathan A Sternberg
> Priority: Major
> Labels: pull-request-available
>
> The change that introduced WEEK and QUARTER processing here did not correctly
> implement the `evaluateIntervalLiteralAsX` functions for these two time units.
> For both of them, it used the `fillIntervalValueArray` with the week
> parameter being used for months and the quarter parameter being used for
> months with no modifications to the underlying values.
> For weeks, this results in the field being misused as the hour parameter so 2
> weeks becomes 2 hours. For quarters, this results in the field being misused
> as months so 2 quarters becomes 2 months.
> I believe the proper way to implement these is to perform the modification so
> weeks gets translated to 7 days and quarters gets translated to 3 months.
> This seems to only affect the string versions such as `INTERVAL '2' WEEK`.
> The integer variants don't seem to use the literal pathway so they aren't
> affected.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
