Thank you Bruno, 1. Thank you for the notification. I have updated the ticket link accordingly. 2. Certainly, I'll update the KIP name. Should I initiate a new discussion for it, because if I change the name, the link will change. 3. Understood, I will add that to the KIP. 4. I propose we accept both `WindowRangeQuery.withAllKeys().fromTime(time1).toTime(time2)` and `WindowRangeQuery.withKeyRange(key1, key2).fromTime(time1).toTime(time2)`, while also reusing the existing `withKey` method. 5. Following a discussion with Matthias, we've decided to defer the implementation of order guarantees to a future KIP.
Sincerely, Hanyu On Wed, Nov 29, 2023 at 6:22 AM Bruno Cadonna <cado...@apache.org> wrote: > Hi, > > Thanks for the updates! > > > 1. > Could you please link the correct ticket in the KIP? > > 2. > Could you please adapt the motivation section and the title to the > updated goal of the KIP? There is no fetch() or fetchAll() method in the > query class. > > 3. > Could you please add the "// newly added" comment to all parts that were > newly added? That is methods lowerKeyBound() and upperKeyBound(). > > 4. > We should use a more fluent API as I proposed in my last e-mail: > > Here again > > WindowRangeQuery.withAllKeys().fromTime(time1).toTime(time2); > WindowRangeQuery.withKey(key1).fromTime(time1).toTime(time2); > WindowRangeQuery.withKeyRange(key1, key2).fromTime(time1).toTime(time2); > > 5. > We should also consider the order of the results similar as we did in > KIP-968. Alternatively, we do not guarantee any order and postpone the > order guarantees to a future KIP. > > > Best, > Bruno > > > > On 11/17/23 3:11 AM, Matthias J. Sax wrote: > > Thanks for the KIP. > > > > Given how `WindowRangeQuery` works right now, it's really time to > > improve it. > > > > > > 1) Agree. It's not clear what will be added right now. I think we should > > deprecate existing `getKey()` w/o an actually replacement? For > > `getFromKey` and `getToKey` we should actually be `lowerKeyBound()` and > > `upperKeyBound()` to align to KIP-969? > > > > Also wondering if we should deprecate existing `withKey()` and > > `withWindowStartRange`? `withKey` only works for SessionStores and > > implements a single-key full-time-range query. Similarly, > > `withWindowStartRange` only works for WindowedStore and implements an > > all-key time-range query. Thus, both are rather special and it seems the > > aim of this KIP is to generalize `WindowRangeQuery` to arbitrary > > key-range/time-range queries? > > > > What raises one question about time-range semantics, given that we query > > windows with different semantics. > > > > - The current `WindowStore` semantics used for > > `WindowRangeQuery#withWindowStartRange` is considering only the window > > start time, ie, the window-start time must fall into the query > > time-range to be returned. > > > > - In contrast, `SessionStore` time ranges base on `findSession` use > > earliest-session-end-time and latest-session-end-time and thus implement > > an "window-bounds / search-time-range overlap query". > > > > Is there any concern about semantic differences? I would also be > > possible to use the same semantics for both query types, and maybe even > > let the user pick with semantics they want (let users chose might > > actually be the best thing to do)? -- We can also do this incrementally, > > and limit the scope of this KIP (or keep the full KIP scope but > > implement it incrementally only) > > > > Btw: I think we should not add any ordering at this point, and > > explicitly state that no ordering is guarantee whatsoever at this point. > > > > > > > > 2) Agreed. We should deprecate `getFromTime` and `getToTime` and add new > > method `fromTime` and `toTime`. > > > > > > > > 3) About the API. If we move forward with general key-range/time-range I > > agree that a more modular approach might be nice. Not sure right now, > > what the best approach would be for this? Looking into KIP-969, we might > > want to have: > > > > - static withKeyRange > > - static withLowerKeyBound > > - static withUpperKeyBound > > - static withAllKeys (KIP-969 actually uses `allKeys` ?) > > - fromTime > > - toTime > > > > with default-time range would be "all / unbounded" ? > > > > > > > > 10: you mentioned that `WindowKeyQuery` functionality can be covered by > > `WindowRangeQuery`. I agree. For this case, it seems we want to > > deprecate `WindowKeyQuery` entirely? > > > > > > > > -Matthias > > > > On 11/16/23 1:19 AM, Bruno Cadonna wrote: > >> Hi Hanyu, > >> > >> Thanks for the KIP! > >> > >> 1) > >> Could you please mark the pieces that you want to add to the API in > >> the code listing in the KIP? You can add a comment like "// newly > >> added" or similar. That would make reading the KIP a bit easier > >> because one does not need to compare your code with the code in the > >> current codebase. > >> > >> 2) > >> Could you -- as a side cleanup -- also change the getters to not use > >> the get-prefix anymore, please? That was apparently an oversight when > >> those methods were added. Although the API is marked as Evolving, I > >> think we should still deprecate the getX() methods, since it does not > >> cost us anything. > >> > >> 3) > >> I propose to make the API a bit more fluent. For example, something like > >> > >> WindowRangeQuery.withKey(key).fromTime(t1).toTime(t2) > >> > >> and > >> > >> WindowRangeQuery.withAllKeys().fromTime(t1).toTime(t2) > >> > >> and > >> > >> WindowRangeQuery.withKeyRange(key1, key2).fromTime(t1).toTime(t2) > >> > >> and maybe even in addition to the above add also the option to start > >> with the time range > >> > >> WindowRangeQuery.withWindowStartRange(t1, t2).fromKey(key1).toKey(key2) > >> > >> > >> 4) > >> Could you also add some usage examples? Alieh did quite a nice job > >> regarding usage examples in KIP-986. > >> > >> > >> Best, > >> Bruno > >> > >> On 11/8/23 8:02 PM, Hanyu (Peter) Zheng wrote: > >>> Hello everyone, > >>> > >>> I would like to start the discussion for KIP-997: Support > fetch(fromKey, > >>> toKey, from, to) to WindowRangeQuery and unify WindowKeyQuery and > >>> WindowRangeQuery > >>> The KIP can be found here: > >>> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-997%3A++Support+fetch%28fromKey%2C+toKey%2C+from%2C+to%29+to+WindowRangeQuery+and+unify+WindowKeyQuery+and+WindowRangeQuery > >>> > >>> Any suggestions are more than welcome. > >>> > >>> Many thanks, > >>> Hanyu > >>> > >>> On Wed, Nov 8, 2023 at 10:38 AM Hanyu (Peter) Zheng > >>> <pzh...@confluent.io> > >>> wrote: > >>> > >>>> > >>>> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-997%3A++Support+fetch%28fromKey%2C+toKey%2C+from%2C+to%29+to+WindowRangeQuery+and+unify+WindowKeyQuery+and+WindowRangeQuery > >>>> > >>>> -- > >>>> > >>>> [image: Confluent] <https://www.confluent.io> > >>>> Hanyu (Peter) Zheng he/him/his > >>>> Software Engineer Intern > >>>> +1 (213) 431-7193 <+1+(213)+431-7193> > >>>> Follow us: [image: Blog] > >>>> < > https://www.confluent.io/blog?utm_source=footer&utm_medium=email&utm_campaign=ch.email-signature_type.community_content.blog > >[image: > >>>> Twitter] <https://twitter.com/ConfluentInc>[image: LinkedIn] > >>>> <https://www.linkedin.com/in/hanyu-peter-zheng/>[image: Slack] > >>>> <https://slackpass.io/confluentcommunity>[image: YouTube] > >>>> <https://youtube.com/confluent> > >>>> > >>>> [image: Try Confluent Cloud for Free] > >>>> < > https://www.confluent.io/get-started?utm_campaign=tm.fm-apac_cd.inbound&utm_source=gmail&utm_medium=organic > > > >>>> > >>> > >>> > -- [image: Confluent] <https://www.confluent.io> Hanyu (Peter) Zheng he/him/his Software Engineer Intern +1 (213) 431-7193 <+1+(213)+431-7193> Follow us: [image: Blog] <https://www.confluent.io/blog?utm_source=footer&utm_medium=email&utm_campaign=ch.email-signature_type.community_content.blog>[image: Twitter] <https://twitter.com/ConfluentInc>[image: LinkedIn] <https://www.linkedin.com/in/hanyu-peter-zheng/>[image: Slack] <https://slackpass.io/confluentcommunity>[image: YouTube] <https://youtube.com/confluent> [image: Try Confluent Cloud for Free] <https://www.confluent.io/get-started?utm_campaign=tm.fm-apac_cd.inbound&utm_source=gmail&utm_medium=organic>
