Hi Patrick, Thanks for the KIP!
I have some comments, in addition to Guozhang's comments: 4. The parameter names `windowLower` and `windowUpper` are kind of ambiguous to me. Could we come up a better name for it, like `windowStartTime`, `windowEndTime`, or even we don't need the "window" name, just `startTime` and `endTime`? 5. Why can't we support window range query with a key within a time range? You might need to explain in the KIP. Thank you. Luke On Sat, Dec 11, 2021 at 7:54 AM Guozhang Wang <wangg...@gmail.com> wrote: > Hi Patrick, > > I made a pass on the KIP and have a few comments below: > > 1. The `WindowRangeQuery` has a private constructor while the > `WindowKeyQuery` has not, is that intentional? > > 2. The `WindowRangeQuery` seems not allowing to range over both window and > key, but only window with a fixed key, in that case it seems pretty much > the same as the other (ignoring the constructor), since we know we would > only have a single `key` value in the returned iterator, and hence it seems > returning in the form of `WindowStoreIterator<V>` is also fine as the key > is fixed and hence no need to maintain it in the returned iterator. I'm > wondering should we actually support ranging over keys as well in > `WindowRangeQuery`? > > 3. The KIP title mentioned both session and window, but the APIs only > involves window stores; However the return type `WindowStoreIterator` is > only for window stores not session stores, so I feel we would still have > some differences for session window query interface? > > > Guozhang > > On Fri, Dec 10, 2021 at 1:32 PM Patrick Stuedi > <pstu...@confluent.io.invalid> > wrote: > > > Hi everyone, > > > > I would like to start the vote for KIP-806 that adds window and session > > query support to query KV-stores using IQv2. > > > > The KIP can be found here: > > https://cwiki.apache.org/confluence/x/LJaqCw > > > > Skipping the discussion phase as this KIP is following the same pattern > as > > the previously submitted KIP-805 (KIP: > > https://cwiki.apache.org/confluence/x/85OqCw, Discussion: > > https://tinyurl.com/msp5mcb2). Of course concerns/comments can still be > > brought up in this thread. > > > > -Patrick > > > > > -- > -- Guozhang >
