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

feat: Added query library functions to overload Period and Duration arithmetic #5509

Merged
merged 13 commits into from
Jun 18, 2024
188 changes: 186 additions & 2 deletions engine/time/src/main/java/io/deephaven/time/DateTimeUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import static io.deephaven.util.QueryConstants.NULL_INT;
import static io.deephaven.util.QueryConstants.NULL_LONG;
import static io.deephaven.util.QueryConstants.*;
import static java.time.format.DateTimeFormatter.*;

/**
Expand Down Expand Up @@ -1652,6 +1651,44 @@ public static ZonedDateTime plus(@Nullable final ZonedDateTime dateTime, @Nullab
}
}

/**
* Adds two durations.
*
* @param duration1 first duration
* @param duration2 second duration
* @return {@code null} if either input is {@code null}; otherwise the sum of the two durations
*/
public static Duration plus(@Nullable final Duration duration1, @Nullable final Duration duration2) {
if (duration1 == null || duration2 == null) {
return null;
}

try {
return duration1.plus(duration2);
} catch (Exception ex) {
throw new DateTimeOverflowException(ex);
}
}

/**
* Adds two periods.
*
* @param period1 first period
* @param period2 second period
* @return {@code null} if either input is {@code null}; otherwise the sum of the two periods
*/
public static Period plus(@Nullable final Period period1, @Nullable final Period period2) {
if (period1 == null || period2 == null) {
return null;
}

try {
return period1.plus(period2);
} catch (Exception ex) {
throw new DateTimeOverflowException(ex);
}
}

/**
* Subtracts days from a {@link LocalDate}.
*
Expand Down Expand Up @@ -1871,6 +1908,153 @@ public static long minus(@Nullable final ZonedDateTime dateTime1, @Nullable fina
return checkUnderflowMinus(epochNanos(dateTime1), epochNanos(dateTime2), true);
}

/**
* Subtracts two durations.
*
* @param duration1 first duration
* @param duration2 second duration
* @return {@code null} if either input is {@code null}; otherwise the difference of the two durations
*/
public static Duration minus(@Nullable final Duration duration1, @Nullable final Duration duration2) {
if (duration1 == null || duration2 == null) {
return null;
}

try {
return duration1.minus(duration2);
} catch (Exception ex) {
throw new DateTimeOverflowException(ex);
}
}

/**
* Subtracts two periods.
*
* @param period1 first period
* @param period2 second period
* @return {@code null} if either input is {@code null}; otherwise the difference of the two periods
*/
public static Period minus(@Nullable final Period period1, @Nullable final Period period2) {
if (period1 == null || period2 == null) {
return null;
}

try {
return period1.minus(period2);
} catch (Exception ex) {
throw new DateTimeOverflowException(ex);
}
}

/**
* Multiply a duration by a scalar.
*
* @param duration the duration to multiply
* @param scalar the scalar to multiply by
* @return {@code null} if either input is {@code null}; otherwise the duration multiplied by the scalar
*/
public static Duration multiply(final Duration duration, final long scalar) {
if (duration == null || scalar == NULL_LONG) {
return null;
}

try {
return duration.multipliedBy(scalar);
} catch (ArithmeticException ex) {
throw new DateTimeOverflowException(ex);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is inconsistent (although probably more correct) than the versions that catch (Exception ex). The date time operations typically document throwing:

     * @throws DateTimeException if the subtraction cannot be made
     * @throws ArithmeticException if numeric overflow occurs

So, when we catch Exception and re-wrap in DateTimeOverflowException, we might be "lying" b/c this could be caused by something other than overflow.

I see this pattern is used widely in DateTimeUtils, I think imprecisely.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have adjusted the handling to be:

        } catch (DateTimeException | ArithmeticException ex) {
            throw new DateTimeOverflowException(ex);
        }

Overflows can come through either exception. java.time does use DateTimeException for other problems, but those problems should not be in the methods here.

}

/**
* Multiply a duration by a scalar.
*
* @param duration the duration to multiply
* @param scalar the scalar to multiply by
* @return {@code null} if either input is {@code null}; otherwise the duration multiplied by the scalar
*/
public static Duration multiply(final long scalar, final Duration duration) {
return multiply(duration, scalar);
}

/**
* Multiply a period by a scalar.
*
* @param period the period to multiply
* @param scalar the scalar to multiply by
* @return {@code null} if either input is {@code null}; otherwise the period multiplied by the scalar
*/
public static Period multiply(final Period period, final int scalar) {
if (period == null || scalar == NULL_INT) {
return null;
}

try {
return period.multipliedBy(scalar);
} catch (ArithmeticException ex) {
throw new DateTimeOverflowException(ex);
}
}

/**
* Multiply a period by a scalar.
*
* @param period the period to multiply
* @param scalar the scalar to multiply by
* @return {@code null} if either input is {@code null}; otherwise the period multiplied by the scalar
*/
public static Period multiply(final int scalar, final Period period) {
return multiply(period, scalar);
}

/**
* Multiply a period by a scalar.
*
* @param period the period to multiply
* @param scalar the scalar to multiply by
* @return {@code null} if either input is {@code null}; otherwise the period multiplied by the scalar
*/
public static Period multiply(final Period period, final long scalar) {
if (period == null || scalar == NULL_LONG) {
return null;
}

if (scalar > Integer.MAX_VALUE || scalar < Integer.MIN_VALUE) {
throw new DateTimeOverflowException("Scalar value is too large to be cast to an int");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why provide a long version if this is the case?

Copy link
Member Author

@chipkent chipkent May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lack of a long version was the first complaint when @alexpeters1208 did user testing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(makes sense to me to have this — it's easy to wind up with long-typed columns in the query language even when you only need/expect int-or-smaller values)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this argument. The same could be said for any int-based method "why doesn't it accept a long?". Should we create a version of this that works with BigInteger? If the answer is "no", I would say we shouldn't provide a long version. I would much prefer to address the root of the issue "I've got a logical int-column, but it's technically typed as long; how can I make it an int column?". In which case, you could just cast or do a check + cast.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with Devin, here. If we don't accept longs that are out of int range, it makes no sense to have a method that takes a long.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @rbasralian on this. Stuff like this happens frequently in real queries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is happening frequently in real queries, we should address the root cause of it: "Why are logical int columns typed as longs?".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't something we're doing elsewhere in our libraries. It isn't something users should expect from libraries generally.


return period.multipliedBy((int) scalar);
}

/**
* Multiply a period by a scalar.
*
* @param period the period to multiply
* @param scalar the scalar to multiply by
* @return {@code null} if either input is {@code null}; otherwise the period multiplied by the scalar
*/
public static Period multiply(final long scalar, final Period period) {
return multiply(period, scalar);
}

/**
* Divide a duration by a scalar.
*
* @param duration the duration to divide
* @param scalar the scalar to divide by
* @return {@code null} if either input is {@code null}; otherwise the duration divide by the scalar
*/
public static Duration divide(final Duration duration, final long scalar) {
if (duration == null || scalar == NULL_LONG) {
return null;
}

try {
return duration.dividedBy(scalar);
} catch (ArithmeticException ex) {
throw new DateTimeOverflowException(ex);
}
}

/**
* Returns the difference in nanoseconds between two instant values.
*
Expand Down
129 changes: 129 additions & 0 deletions engine/time/src/test/java/io/deephaven/time/TestDateTimeUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -2013,6 +2013,40 @@ public void testPlusLocalDate() {
}
}

public void testPlusDuration() {
final Duration d1 = Duration.ofSeconds(1);
final Duration d2 = Duration.ofSeconds(2);
final Duration d3 = Duration.ofSeconds(3);
TestCase.assertEquals(d3, DateTimeUtils.plus(d1, d2));
TestCase.assertEquals(d3, DateTimeUtils.plus(d2, d1));
TestCase.assertNull(DateTimeUtils.plus(d1, null));
TestCase.assertNull(DateTimeUtils.plus((Duration) null, d2));

try {
DateTimeUtils.plus(d1, Duration.ofSeconds(Long.MAX_VALUE));
TestCase.fail("This should have overflowed");
} catch (DateTimeUtils.DateTimeOverflowException e) {
// ok
}
}

public void testPlusPeriod() {
final Period d1 = Period.ofDays(1);
final Period d2 = Period.ofDays(2);
final Period d3 = Period.ofDays(3);
TestCase.assertEquals(d3, DateTimeUtils.plus(d1, d2));
TestCase.assertEquals(d3, DateTimeUtils.plus(d2, d1));
TestCase.assertNull(DateTimeUtils.plus(d1, null));
TestCase.assertNull(DateTimeUtils.plus((Period) null, d2));

try {
DateTimeUtils.plus(d1, Period.ofDays(Integer.MAX_VALUE));
TestCase.fail("This should have overflowed");
} catch (DateTimeUtils.DateTimeOverflowException e) {
// ok
}
}

public void testPlus() {
final Instant instant = DateTimeUtils.parseInstant("2010-01-01T12:13:14.999123456 JP");
final ZonedDateTime zdt = DateTimeUtils.toZonedDateTime(instant, TZ_AL);
Expand Down Expand Up @@ -2188,6 +2222,38 @@ public void testMinusLocalDate() {
}
}

public void testMinusDuration() {
final Duration d1 = Duration.ofSeconds(3);
final Duration d2 = Duration.ofSeconds(1);
TestCase.assertEquals(Duration.ofSeconds(2), DateTimeUtils.minus(d1, d2));
TestCase.assertEquals(Duration.ofSeconds(-2), DateTimeUtils.minus(d2, d1));
TestCase.assertNull(DateTimeUtils.minus(d1, null));
TestCase.assertNull(DateTimeUtils.minus((Duration) null, d2));

try {
DateTimeUtils.minus(d1, Duration.ofSeconds(Long.MIN_VALUE));
TestCase.fail("This should have overflowed");
} catch (DateTimeUtils.DateTimeOverflowException e) {
// ok
}
}

public void testMinusPeriod() {
final Period d1 = Period.ofDays(3);
final Period d2 = Period.ofDays(1);
TestCase.assertEquals(Period.ofDays(2), DateTimeUtils.minus(d1, d2));
TestCase.assertEquals(Period.ofDays(-2), DateTimeUtils.minus(d2, d1));
TestCase.assertNull(DateTimeUtils.minus(d1, null));
TestCase.assertNull(DateTimeUtils.minus((Period) null, d2));

try {
DateTimeUtils.minus(d1, Period.ofDays(Integer.MIN_VALUE));
TestCase.fail("This should have overflowed");
} catch (DateTimeUtils.DateTimeOverflowException e) {
// ok
}
}

public void testMinus() {
final Instant instant1 = DateTimeUtils.parseInstant("2010-01-01T12:13:14.999123456 JP");
final Instant instant2 = DateTimeUtils.parseInstant("2010-01-01T13:13:14.999123456 JP");
Expand Down Expand Up @@ -2347,6 +2413,69 @@ public void testMinus() {

}

public void testMultiply() {
final Period p = Period.ofDays(3);
final Duration d = Duration.ofNanos(123456789L);

TestCase.assertEquals(Period.ofDays(6), DateTimeUtils.multiply(p, 2));
TestCase.assertEquals(Period.ofDays(6), DateTimeUtils.multiply(2, p));
TestCase.assertEquals(Period.ofDays(9), DateTimeUtils.multiply(p, 3));
TestCase.assertEquals(Period.ofDays(3), DateTimeUtils.multiply(p, 1));
TestCase.assertEquals(Period.ofDays(0), DateTimeUtils.multiply(p, 0));
TestCase.assertEquals(Period.ofDays(-3), DateTimeUtils.multiply(p, -1));
TestCase.assertEquals(Period.ofDays(-6), DateTimeUtils.multiply(p, -2));
TestCase.assertEquals(Period.ofDays(-9), DateTimeUtils.multiply(p, -3));
TestCase.assertNull(DateTimeUtils.multiply((Period) null, 3));
TestCase.assertNull(DateTimeUtils.multiply(p, NULL_INT));

TestCase.assertEquals(Period.ofDays(6), DateTimeUtils.multiply(p, 2L));
TestCase.assertEquals(Period.ofDays(6), DateTimeUtils.multiply(2L, p));
TestCase.assertEquals(Period.ofDays(9), DateTimeUtils.multiply(p, 3L));
TestCase.assertEquals(Period.ofDays(3), DateTimeUtils.multiply(p, 1L));
TestCase.assertEquals(Period.ofDays(0), DateTimeUtils.multiply(p, 0L));
TestCase.assertEquals(Period.ofDays(-3), DateTimeUtils.multiply(p, -1L));
TestCase.assertEquals(Period.ofDays(-6), DateTimeUtils.multiply(p, -2L));
TestCase.assertEquals(Period.ofDays(-9), DateTimeUtils.multiply(p, -3L));
TestCase.assertNull(DateTimeUtils.multiply((Period) null, 3L));
TestCase.assertNull(DateTimeUtils.multiply(p, NULL_LONG));

try {
DateTimeUtils.multiply(p, Long.MAX_VALUE);
TestCase.fail("This should have overflowed");
} catch (DateTimeUtils.DateTimeOverflowException e) {
// ok
}

TestCase.assertEquals(Duration.ofNanos(246913578L), DateTimeUtils.multiply(d, 2));
TestCase.assertEquals(Duration.ofNanos(246913578L), DateTimeUtils.multiply(2, d));
TestCase.assertEquals(Duration.ofNanos(370370367L), DateTimeUtils.multiply(d, 3));
TestCase.assertEquals(Duration.ofNanos(123456789L), DateTimeUtils.multiply(d, 1));
TestCase.assertEquals(Duration.ofNanos(0), DateTimeUtils.multiply(d, 0));
TestCase.assertEquals(Duration.ofNanos(-123456789L), DateTimeUtils.multiply(d, -1));
TestCase.assertEquals(Duration.ofNanos(-246913578L), DateTimeUtils.multiply(d, -2));
TestCase.assertEquals(Duration.ofNanos(-370370367L), DateTimeUtils.multiply(d, -3));
TestCase.assertNull(DateTimeUtils.multiply((Duration) null, 3));
TestCase.assertNull(DateTimeUtils.multiply(d, NULL_LONG));
}

public void testDivideDuration() {
final Duration d = Duration.ofNanos(123456789L);

TestCase.assertEquals(Duration.ofNanos(61728394L), DateTimeUtils.divide(d, 2));
TestCase.assertEquals(Duration.ofNanos(123456789L), DateTimeUtils.divide(d, 1));
TestCase.assertEquals(Duration.ofNanos(-123456789L), DateTimeUtils.divide(d, -1));
TestCase.assertEquals(Duration.ofNanos(-61728394L), DateTimeUtils.divide(d, -2));
TestCase.assertNull(DateTimeUtils.divide((Duration) null, 3));
TestCase.assertNull(DateTimeUtils.divide(d, NULL_LONG));

try {
DateTimeUtils.divide(d, 0);
TestCase.fail("This should have excepted");
} catch (DateTimeUtils.DateTimeOverflowException e) {
// ok
}
}

public void testDiffNanos() {
final Instant i1 = DateTimeUtils.epochNanosToInstant(12345678987654321L);
final Instant i2 = DateTimeUtils.epochNanosToInstant(98765432123456789L);
Expand Down
Loading