Hi Patrick, thanks for the KIP! I hope you, Guozhang, and Luke don't mind if I share some thoughts:
1. I think you just meant to remove that private constructor from the KIP, right? 2. I think WindowRangeQuery#withWindowRage(windowLower, windowUpper) is the version that yields an iterator over both keys and windows, so it's not totally redundant. However, it does seem like WindowRangeQuery#withKey is equivalent to WindowKeyQuery#withKey . I overlooked it before, but we probably don't need both. Stepping back, it seems like we could either just de- duplicate that specific withKey query or we could even just merge both use cases into the WindowRangeQuery. I've personally never been convinced that the extra complexity of having the WindowStoreIterator is worth the savings of not duplicating the key on each record. Maybe it would be if we separately deserialized the same key over and over again, but that seems optimizable. On the other hand, that's more work, and the focus for IQv2 right now is to just get some MVP of queries implemented to cover basic use cases, so it might be worthwhile to keep it simple (by just dropping `WindowRangeQuery#withKey`). I'm happy with whatever call Patrick wants to make here, since it's his work to do. 3. I got the impression that WindowRangeQuery was intended for use with both Window and Session stores, but that might be my assumption. 4. That's good feedback. I think those names were inspired by the WindowStore methods that just say stuff like `timeFrom` and `timeTo`. The SessionStore does a better job of being unambiguous, since it says stuff like `earliestSessionEndTime` and `latestSessionStartTime`. And, actually, that brings up a point that I think we overlooked before. While the method signatures of the WindowStore and SessionStore methods look the same, the meanings of their arguments are not the same. In particular, the WindowStore ranges are always in reference to the window start time, while the SessionStore ranges may be in reference to the window start time or end time (or different combinations of both). I suspect that your instinct to cover both stores with the same Query is correct, and we just need to be specific about the kinds of ranges we're talking about. For example, instead of withWindowRange, maybe we would have: withWindowStartRange( Instant earliestWindowStartTime, Instant latestWindowStartTime ); Then in the future, we could add stuff like: withWindowStartAndEndRange( Instant earliestWindowStartTime, Instant latestWindowEndTime ); Etc. 5. I think Luke's question reveals how weirdly similar but not the same these two queries are. It's not your fault, this is inherited from the existing set of weirdly-similar- but-not-the-same store methods. The thing that distinguishes the WindowKeyQuery from the WindowRangeQuery is: * WindowKeyQuery: all results share the same K key (cf point 2: I don't think we need `WindowRangeQuery#withKey(k)`). Therefore, the iterator is just (windowStartTime, value) pairs. * WindowRangeQuery: the results may have different keys, so the iterator is (Windowed<K>, value) pairs, where Windowed<K> is basically a (windowStartTime, K key) pair. Therefore, we could support a WindowRangeQuery with a fixed key, but it would just be another way to express the same thing as the WindowKeyQuery with the same parameters. As in point 2, I do think it might be worth converging all use cases into the WindowRangeQuery, but we can also just shoot for parity now and defer elegance for the future. 6. I'll just add one question of my own here: If we do keep both query classes, then I think we would drop `withKey` and `getKey` from the WindowRangeQuery. But if we do wind up keeping the WindowRangeQuery class, I think we should reconsider the name of the getter, since there are other range queries that support a range of keys. Is there a getter name we can use that would still work if we come back later and add lower and upper key bounds? Thanks again for the KIP! John On Mon, 2021-12-13 at 16:35 +0800, Luke Chen wrote: > 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 > >
