-
Notifications
You must be signed in to change notification settings - Fork 78
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
Conversation
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.
Test results:
from deephaven import empty_table
t = empty_table(100).update("BaseTime = '2020-01-01T00:00:00Z'")
# pass
t1 = t.update("Timestamp = BaseTime + i*'PT1s'")
# pass
t2 = t.update("Timestamp = BaseTime + ii*'PT1s'")
# pass
t3 = t.update("Timestamp = BaseTime + i*'P1D'")
# fail
t4 = t.update("Timestamp = BaseTime + ii*'P1D'")
# pass
t5 = t.update("Timestamp = BaseTime + 1*'PT1s'")
# pass
t6 = t.update("Timestamp = BaseTime + 1L*'PT1s'")
# pass
t7 = t.update("Timestamp = BaseTime + 1*'P1D'")
# fail
t8 = t.update("Timestamp = BaseTime + 1L*'P1D'")
# pass
t9 = t.update("Timestamp = BaseTime + ((int)1)*'PT1s'")
# pass
t10 = t.update("Timestamp = BaseTime + ((long)1)*'PT1s'")
# pass
t11 = t.update("Timestamp = BaseTime + ((int)1)*'P1D'")
# fail
t12 = t.update("Timestamp = BaseTime + ((long)1)*'P1D'")
Since multiply(long, Duration)
is supported, I also expect multiply(long, Period)
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.
Code looks good, is there a good reason to implement a workaround for multiply(long, Period)
?
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.
This seems okay in isolation, but it does seem a little bit weird to offer multiplication but not addition. Just making sure we are aware of this and are ok with it before merging.
Add periods. Subtract durations. Subtract periods. Multiply durations by doubles. Multiply periods by longs. Divide durations.
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.
A couple of questions / observations:
- Should
Duration1 / Duration2 -> Duration
whenDuration1 >= Duration2
be supported? - Should
Duration1 / Duration2 -> Period
whenDuration1 < Duration2
be supported? - Should
Period1 / Period2 -> Period
be supported? - Should anything involving
%
be supported?
Next, I ran more tests and found unexpected or inconsistent behavior with Java's primitive null
:
from deephaven import empty_table
t = empty_table(10).update(["P='P3D'", "D = 'PT3s'"])
# Pass
t1 = t.update("P1 = P + null")
# Pass
t2 = t.update("P2 = P - null")
# Fail
t3 = t.update("P3 = P * null")
# Fail
t4 = t.update("P3 = P / null")
# Pass
t5 = t.update("D1 = D + null")
# Pass
t6 = t.update("D2 = D - null")
# Fail
t7 = t.update("D3 = D * null")
# Fail
t8 = t.update("D3 = D / null")
I also ran all of these tests, and everything worked as expected:
from deephaven import empty_table
t = empty_table(10).update(["P='P3D'", "D = 'PT3s'"])
# Pass
t1 = t.update("P1 = P * NULL_INT")
# Pass
t2 = t.update("P2 = D * NULL_INT")
# Pass
t3 = t.update("P3 = D / NULL_INT")
# Pass
t4 = t.update("P4 = P * NULL_LONG")
# Pass
t5 = t.update("P5 = D * NULL_LONG")
# Pass
t6 = t.update("P6 = D / NULL_LONG")
# Pass
t7 = t.update("P7 = D * NULL_FLOAT")
# Pass
t8 = t.update("P8 = D / NULL_FLOAT")
# Pass
t9 = t.update("P9 = D * NULL_DOUBLE")
# Pass
t10 = t.update("P10 = D / NULL_DOUBLE")
if (scalar > Integer.MAX_VALUE || scalar < Integer.MIN_VALUE) { | ||
throw new DateTimeOverflowException("Scalar value is too large to be cast to an int"); | ||
} |
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.
Why provide a long
version if this is the case?
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.
The lack of a long version was the first complaint when @alexpeters1208 did user testing.
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.
(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)
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 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.
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'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
.
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 agree with @rbasralian on this. Stuff like this happens frequently in real queries.
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.
If this is happening frequently in real queries, we should address the root cause of it: "Why are logical int columns typed as longs?".
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.
This isn't something we're doing elsewhere in our libraries. It isn't something users should expect from libraries generally.
I'm not sure what you are asking for. You want Is there a real use case for that?
Again, I'm not sure what you are asking for. You want
Period divisions were intentionally not supported. It is too poorly defined too quickly. There is no way to properly represent 1 day divided by 3.
No operations are obvious to me.
Note that in all of these tests the
I'm guessing this is getting converted to
I'm guessing this is getting converted to
There are no object methods for
There are no object methods for
I'm guessing this is getting converted to
I'm guessing this is getting converted to
There are no object methods for
There are no object methods for What exactly did you expect to happen in these cases? How would it happen with a real query? |
Removed floating point methods.
} catch (ArithmeticException ex) { | ||
throw new DateTimeOverflowException(ex); | ||
} |
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.
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.
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 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.
More explicit overflow exception handling.
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 would approve this if Period multiply(final Period period, final long scalar)
is changed to an int
. I think if you still feel strongly about the long
version we can continue debating after this PR is merged?
Labels indicate documentation is required. Issues for documentation have been opened: Community: deephaven/deephaven-docs-community#237 |
Added query library functions to overload Period and Duration arithmetic.
Resolves #5379