Adds BatchID interval to TS interval #443
Conversation
| b match { | ||
| case Empty() => Empty[Timestamp]() | ||
| case Universe() => Universe[Timestamp]() | ||
| case ExclusiveUpper(upper) => ExclusiveUpper(latestTimeOf(upper) + Timestamp(1)) |
There was a problem hiding this comment.
that is weird. you should not be able to add timestamps. Timestamp + Int or or + Long makes sense, but what does adding two times mean?
There was a problem hiding this comment.
Thats a good point.... will remove it from here. But will add a todo to remove the methods and find/change other call sites of the operator
There was a problem hiding this comment.
The larger question aside, shouldn't this be:
ExclusiveUpper(earliestTimeOf(upper)) ?
There was a problem hiding this comment.
Agree with Joe.
The exclusive UB on Batch covers time that should also be exclusive. Therefore, the earliest time in that batch is the least upper bound. Right?
I may have made this error originally.
Update the interval mappings with batchID -> Timestamp, update test to cover it
| def -(other: Long) = Timestamp(milliSinceEpoch - other) | ||
| def +(other: Long) = Timestamp(milliSinceEpoch + other) | ||
| // Delta between two timestamps | ||
| def -(other: Timestamp) = milliSinceEpoch - other.milliSinceEpoch |
There was a problem hiding this comment.
can we put the type on a public API, Long?
There was a problem hiding this comment.
Fuck….
Actually, let's be pro-style, we know we are going there: how about a case class Milliseconds(toLong: Long)? Clearly that is where we are heading here. In 2.10 this can extend AnyVal and it is free.
|
Sorry forgot about this, fixed the build... |
…erval Adds BatchID interval to TS interval
add QTreeAggregator and add approximatePercentileBounds to Aggregator
@johnynek splitting this out to get it discussed/in first. Modified the bounds on the exclusives a little.