JAVA-1347: Add support for duration type #776
Conversation
adutra
force-pushed
the
java1347
branch
2 times, most recently
from
ed34457 to
ecbe5a7
Compare
|
|
||
| /** | ||
| */ | ||
| public static class DurationType extends CustomType { |
There was a problem hiding this comment.
Just a reminder to either fill in or remove the javadoc comment
| private Duration(int months, int days, long nanoseconds) { | ||
| // Makes sure that all the values are negatives if one of them is | ||
| assert (months >= 0 && days >= 0 && nanoseconds >= 0) | ||
| || ((months <= 0 && days <= 0 && nanoseconds <= 0)); |
There was a problem hiding this comment.
We should throw an IllegalArgumentException rather than using an assertion, this is called directly from newInstance which is public.
| this.nanoseconds = nanoseconds; | ||
| } | ||
|
|
||
| public static Duration newInstance(int months, int days, long nanoseconds) { |
There was a problem hiding this comment.
Needs a javadoc with an explanation on positive/negative arguments.
| * </ul> | ||
| * | ||
| * @param input the <code>String</code> to convert | ||
| * @return a number of nanoseconds |
| } else if (s.equals("ns")) { | ||
| return builder.addNanos(number); | ||
| } | ||
| throw new InvalidTypeException(String.format("Unknown duration symbol '%s'", symbol)); |
There was a problem hiding this comment.
I wonder if InvalidTypeException is the best error to throw here, it's closely related to the codec system but this class is not. Maybe IllegalArgumentException would be more appropriate.
|
|
||
| private DurationType() { | ||
| super(Name.CUSTOM, "org.apache.cassandra.db.marshal.DurationType"); | ||
| } |
There was a problem hiding this comment.
Nit: we could override toString() here, as this produces a slightly better output, for example in TableMetadata.toString() (although the full custom type name is valid and entirely compatible).
| * | ||
| * @return the Duration type. | ||
| */ | ||
| public static DurationType duration() { |
There was a problem hiding this comment.
We might want to add a special case in DataType.decode() to return this singleton as well.
Currently it returns a CustomType instance that is not a DurationType. It appears for example in the column definitions of a prepared statement.
tolbertam
left a comment
tolbertam
left a comment
There was a problem hiding this comment.
Looks good to me! I updated DurationIntegrationTest to test with some sample values defined in the unit tests just to get a wider range of values tried at the integration level. I also tried testing using Durations as a bind parameter for comparison operations w/ CASSANDRA-11936 (i.e. WHERE reading_time < now() - ?) but that doesn't seem to work yet on the C* side.
| .put("tinyint", tinyint()) | ||
| .put("smallint", smallint()) | ||
| // duration is not really a native CQL type, but appears as so in system tables | ||
| .put("duration", duration()) |
There was a problem hiding this comment.
It turns out it will be in the system tables in C* 4.x (v5 protocol), but is a custom type in v4 protocol.
There was a problem hiding this comment.
Nice, I remember that it was a similar story with UDTs being a custom type between a later version of 2.0 and 2.1 (with protocol v3). I'm glad that is the case.
4 participants
No description provided.