[
https://issues.apache.org/jira/browse/DRILL-7926?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17376126#comment-17376126
]
ASF GitHub Bot commented on DRILL-7926:
---------------------------------------
paul-rogers commented on a change in pull request #2268:
URL: https://github.com/apache/drill/pull/2268#discussion_r664950317
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/DateTypeFunctions.java
##########
@@ -424,26 +424,35 @@ public void eval() {
public static class AgeTimeStampFunction implements DrillSimpleFunc {
@Param TimeStampHolder left;
@Param TimeStampHolder right;
+ @Workspace org.joda.time.PeriodType periodType;
@Output IntervalHolder out;
@Override
public void setup() {
+ periodType = org.joda.time.PeriodType.forFields(
+ new org.joda.time.DurationFieldType[] {
+ org.joda.time.DurationFieldType.months(),
+ org.joda.time.DurationFieldType.days(),
+ org.joda.time.DurationFieldType.millis()
+ }
+ );
}
@Override
public void eval() {
- long diff = left.value - right.value;
- long days = diff /
org.apache.drill.exec.vector.DateUtilities.daysToStandardMillis;
- out.months = (int) (days /
org.apache.drill.exec.vector.DateUtilities.monthToStandardDays);
- out.days = (int) (days %
org.apache.drill.exec.vector.DateUtilities.monthToStandardDays);
- out.milliseconds = (int) (diff %
org.apache.drill.exec.vector.DateUtilities.daysToStandardMillis);
+ org.joda.time.Period interval = new
org.joda.time.Period(right.value, left.value, periodType);
Review comment:
Using Joda is certainly a quick fix! What was wrong with the
`DateUtilities` code? Since that code is used in multiple places, could we fix
that code? Or, if the fix is complicated (everything with dates is
complicated), should we change all other uses of the same constants and delete
the constants in `DateUtilities`?
Also, Joda is long obsolete. Is there a "modern" way to do these
calculations?
##########
File path:
exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestDateFunctions.java
##########
@@ -104,10 +104,10 @@ public void testDateDifferenceArithmetic() throws
Exception {
@Test
public void testAge() throws Exception {
- String[] expectedResults = {"P109M16DT82800S",
- "P172M27D",
- "P-172M-27D",
- "P-39M-18DT-63573S"};
+ String[] expectedResults = {"P107M30DT82800S",
Review comment:
Thanks for the tests! The code changed four functions, yet we run only
one test. Should we have tests for each of the four functions? (The underlying
cause of this bug is that we did not have proper tests.)
Also, it is not clear what was wrong with the code: corner cases? Should we
test such corner cases here? Diff of two dates across Feb. 29 on a leap year?
What other things are wrong with the old approach?
Note that, if we have good coverage, then there is no reason to add a test
specific for the one case in DRILL-7926. That case should be just one of many
that we include in the overall test of the feature.
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/DateTypeFunctions.java
##########
@@ -424,26 +424,35 @@ public void eval() {
public static class AgeTimeStampFunction implements DrillSimpleFunc {
@Param TimeStampHolder left;
@Param TimeStampHolder right;
+ @Workspace org.joda.time.PeriodType periodType;
Review comment:
Looks like the period type is constant. Can it be made a `static final`
in, say, the `DateUtilities` class? Is the object thread-safe so that it can be
shared instead of generating the code into each query (and duplicating the
definition)? Looks like we need one for months/days/millis and another for
months/days?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
> The " age " function is not working properly.
> ---------------------------------------------
>
> Key: DRILL-7926
> URL: https://issues.apache.org/jira/browse/DRILL-7926
> Project: Apache Drill
> Issue Type: Bug
> Components: Functions - Drill
> Affects Versions: 1.17.0, 1.18.0
> Reporter: Александр Глухов
> Priority: Blocker
> Attachments: image-2021-05-13-16-17-21-154.png, screenshot-1.png,
> screenshot-2.png
>
>
> The " age " function is not working properly.
> *Playback steps*
> {code:sql}
> select extract(year from m. "years") `years` from (select age('2021-05-13',
> '2007-07-02') `years") m{code}
> *Expected result* 13.
> *Actual result* 14.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
