RFR:8146218: Producing streams in java.time?

[Roger Riggs]

Hi Stephen,

On 1/26/2016 8:57 AM, Stephen Colebourne wrote:
> While there is no blocking reason why the concept could not be pulled up to
> ChronoLocalDate, the method signatures would differ.
>
> LocalDate::datesUntil(LocalDate)
> LocalDate::datesUntil(LocalDate, Period)
>
> ChronoLocalDate::datesUntil(ChronoLocalDate)
> ChronoLocalDate::datesUntil(ChronoLocalDate, ChronoPeriod)
>
> Thus, adding the ChronoLocalDate methods later will make two additional
> methods available on LocalDate (as they will not override).
Since all the calendars are built on the same 24hour days and epochDays, 
the computations
result would be the same regardless of the Chronology.

The existing LocalDate.until, compareTo, isBefore, isAfter, isEqual 
methods already use the
ChronoLocalDate argument type to avoid having double the signatures.

Modifying the type of the argument to be ChronoLocalDate would not 
modify the semantics
and would make it possible to avoid extra methods in the future.

I recommend changing the argument to ChronoLocalDate be consistent with 
the existing
until method to keep the option open for a possible addition to 
ChronoLocalDate
>
> As such, I'm not convinced that the streaming methods are needed on
> ChronoLocalDate. The "correct" mechanism for looping with a ChronoPeriod
> may differ by calendar system (not sure how, but it might). And while
> datesUntil(ChronoLocalDate) would be safe on ChronoLocalDate, it would
> weaken the method signature for the common case on LocalDate.
>
> Basically, since the stream can be created manually in the rare cases of
> ChronoLocalDate, it seems unnecessary to worry about abstracting up.
Yes, there is a work around.

Roger

>
> Stephen
>
>
>
> On 23 January 2016 at 21:40, Roger Riggs <roger.riggs at oracle.com> wrote:
>
>> Hi,
>>
>> Looks good.
>>
>> I would want to verify that it is defined such that if it later needs to be
>> abstracted up to ChronoLocalDate and apply to compatible Chronologies
>> and respective local dates such as JapaneseDate there is no conflict.
>> It should not be an issue since Period implements ChronoPeriod.
>> In the respective implementations, the estimation/computation of the
>> number of steps
>> would need to depend on the Chronology's definition of months.
>>
>> I can sponsor this enhancement.
>>
>> Thanks, Roger
>>
>>
>>
>>
>> On 1/20/16 10:15 AM, Stephen Colebourne wrote:
>>
>>> I'm happy with the logic and specification of this proposal. I think it
>>> will be a useful addition.
>>>
>>> I'll let the Oracle team chime in to do a further review.
>>>
>>> thanks
>>> Stephen
>>>
>>>
>>>
>>> On 16 January 2016 at 13:31, Tagir F. Valeev <amaembo at gmail.com> wrote:
>>>
>>> Hello!
>>>> Thanks for review! Here's the updated patch:
>>>> http://cr.openjdk.java.net/~tvaleev/webrev/8146218/r2/
>>>>
>>>> SC> The docs say that if the end date is before the start date, the
>>>> SC> stream is empty. While I can see just about why LongStream.range()
>>>> SC> works that way, I don't think this API should. The minimum is an
>>>> SC> exception, but it would be easy to support negative in the
>>>> SC> days-only case or the months-only case. The problem is where there
>>>> SC> are both months/years and days and those should be exceptions.
>>>>
>>>> Now datesUntil(endExclusive) throws an IllegalArgumentException if end
>>>> date is before start date.
>>>>
>>>> datesUntil(endExclusive, step) supports negative periods. It throws
>>>> IllegalArgumentException if:
>>>> - step is zero
>>>> - step.toTotalMonths() and step.getDays() both non-zero and have
>>>> opposite sign
>>>> - step is negative and end date is after start date
>>>> - step is positive and end date is before start date
>>>>
>>>> Otherwise it works correctly: you can use
>>>> LocalDate.of(2016, 1, 1)
>>>>     .datesUntil(LocalDate.of(2015, 1, 1), Period.ofMonths(-1));
>>>>
>>>> Steps like Period.of(-1, -1, -1) are also supported.
>>>>
>>>> SC> The single-arg implementation uses plusDays() with an
>>>> SC> incrementing number. When the performance patch goes in, the
>>>> SC> proposed streaming implementation will be optimal for small
>>>> SC> streams but less optimal for large ones. The fastest way to loop
>>>> SC> over a list of dates would be to manually generate them by
>>>> SC> incrementing the day until it exceeds the length of month, and so
>>>> SC> on. However, this would be serial.
>>>>
>>>> As I understand, plusDays performance patch is already pushed.
>>>>
>>>> It's possible to write custom Spliterator which would use
>>>> previous.plusDays(1) in tryAdvance() and jump (via
>>>> LocalDate.of(startEpochDay+n)) in trySplit() if parallel processing is
>>>> requested. However this adds at least one additional class and more
>>>> complexity. I guess, such optimization can be considered later as
>>>> separate issue when API is stabilized.
>>>>
>>>> SC> As such, I think the best way to write this, taking account of
>>>> SC> how plusDays() is implemented, is as follows:
>>>>
>>>> SC> LongStream.range(start.toEpochDay(),
>>>> SC> end.toEpochDay()).mapToObj(LocalDate::ofEpochDay);
>>>>
>>>> Ok, now it's done this way.
>>>>
>>>> SC> In the period-based method, it would be best to capture the case
>>>> SC> where totalMonths == 0 and days > 0 and forward to another method
>>>> SC> that ignores months. That private method can share implementation
>>>> SC> with the public single-arg method (passing in 1).
>>>>
>>>> This case still more complex than one-day case as it requires division
>>>> and multiplication. Thus I'd keep these case separately. However I
>>>> simplified "months = 0" path using ofEpochDay, now it should be
>>>> faster.
>>>>
>>>> SC> The docs for the period-based method should specify the strategy
>>>> SC> that produces the results (in abstract terms). This needs to cover
>>>> SC> that the result is equivalent to always adding to the start period
>>>> SC> a multiple of the period.
>>>>
>>>> I added some clarifications, please check.
>>>>
>>>> SC> Some nits:
>>>> SC> I prefer to avoid @link in the first sentence. Just using 'stream'
>>>> would be sufficient.
>>>>
>>>> Done.
>>>>
>>>> SC> The first sentence should be a summary. In this case it probably has
>>>> a
>>>> bit too much detail.
>>>>
>>>> Done.
>>>>
>>>> SC> The @return has 'values' on a new line when it could be on the same
>>>> line.
>>>>
>>>> I set now line length = 100 characters in my IDE. Is it ok?
>>>>
>>>> SC> If statements need braces.
>>>>
>>>> Done.
>>>>
>>>> With best regards,
>>>> Tagir Valeev.
>>>>
>>>>
>>>>