Hi Hanyu, Thanks a lot for the KIP! I agree with Bruno's comments about fluent API / keeping the query classes immutable. Apart from that, on the technical side this is looking good already!
Two comments on the KIP itself (not the technical part): - The motivation section could be extended by a short paragraph why we want to have `reverseRange` / `reverseAll` in the first place. - Consider using something like languagetool.org to avoid spelling / grammar mistakes, which helps readability. Cheers, Lucas On Wed, Oct 4, 2023 at 10:24 AM Bruno Cadonna <cado...@apache.org> wrote: > > Hi Hanyu, > > I agree with what others said about having a `withDescendingOrder()` > method and about to document how the results are ordered. > > I would not add a reverse flag and adding a parameter to each method in > RangeQuery. This makes the API less fluent and harder to maintain since > the flag would change all methods. There is no constraint to only add > static factory methods to RangeQuery. In fact, if you look into the > existing class KeyQuery, more precisely at skipCache() and into the > proposals for queries of versioned state stores, i.e., KIP-969, KIP-968, > and KIP-960, we already have examples where we set a flag with a > instance method, for example, asOf(). Such methods make the API more > fluent and limit the blast radius of the flag to only one public method > (plus the getter). > > So, making a query that reads the state store in reversed order would > then result in: > > final RangeQuery<Integer, Integer> query = RangeQuery.withRange(1, > 1000).withDescendingKeys(); > > I think this is more readable than: > > final RangeQuery<Integer, Integer> query = RangeQuery.withRange(1, 1000, > true); > > Additionally, I think the KIP would benefit from a usage example of the > newly introduced methods like in KIP-969 etc. > > In my opinion, the test plan should also mention that you plan to > write/adapt unit tests. > > Best, > Bruno > > On 10/4/23 5:16 AM, Hanyu (Peter) Zheng wrote: > > If we use WithDescendingKeys() to generate a RangeQuery to do the > > reveseQuery, how do we achieve the methods like withRange, withUpperBound, > > and withLowerBound only in this method? > > > > On Tue, Oct 3, 2023 at 8:01 PM Hanyu (Peter) Zheng <pzh...@confluent.io> > > wrote: > > > >> I believe there's no need to introduce a method like WithDescendingKeys(). > >> Instead, we can simply add a reverse flag to RangeQuery. Each method within > >> RangeQuery would then accept an additional parameter. If the reverse is set > >> to true, it would indicate the results should be reversed. > >> > >> Initially, I introduced a reverse variable. When set to false, the > >> RangeQuery class behaves normally. However, when reverse is set to true, > >> the RangeQuery essentially takes on the functionality of ReverseRangeQuery. > >> Further details can be found in the "Rejected Alternatives" section. > >> > >> In my perspective, RangeQuery is a class responsible for creating a series > >> of RangeQuery objects. It offers methods such as withRange, withUpperBound, > >> and withLowerBound, allowing us to generate objects representing different > >> queries. I'm unsure how adding a withDescendingOrder() method would be > >> compatible with the other methods, especially considering that, based on > >> KIP 969, WithDescendingKeys() doesn't appear to take any input variables. > >> And if withDescendingOrder() doesn't accept any input, how does it return a > >> RangeQuery? > >> > >> On Tue, Oct 3, 2023 at 4:37 PM Hanyu (Peter) Zheng <pzh...@confluent.io> > >> wrote: > >> > >>> Hi, Colt, > >>> The underlying structure of inMemoryKeyValueStore is treeMap. > >>> Sincerely, > >>> Hanyu > >>> > >>> On Tue, Oct 3, 2023 at 4:34 PM Hanyu (Peter) Zheng <pzh...@confluent.io> > >>> wrote: > >>> > >>>> Hi Bill, > >>>> 1. I will update the KIP in accordance with the PR and synchronize their > >>>> future updates. > >>>> 2. I will use that name. > >>>> 3. you mean add something about ordering at the motivation section? > >>>> > >>>> Sincerely, > >>>> Hanyu > >>>> > >>>> > >>>> On Tue, Oct 3, 2023 at 4:29 PM Hanyu (Peter) Zheng <pzh...@confluent.io> > >>>> wrote: > >>>> > >>>>> Hi, Walker, > >>>>> > >>>>> 1. I will update the KIP in accordance with the PR and synchronize > >>>>> their future updates. > >>>>> 2. I will use that name. > >>>>> 3. I'll provide additional details in that section. > >>>>> 4. I intend to utilize rangeQuery to achieve what we're referring to as > >>>>> reverseQuery. In essence, reverseQuery is merely a term. To clear up any > >>>>> ambiguity, I'll make necessary adjustments to the KIP. > >>>>> > >>>>> Sincerely, > >>>>> Hanyu > >>>>> > >>>>> > >>>>> > >>>>> On Tue, Oct 3, 2023 at 4:09 PM Hanyu (Peter) Zheng <pzh...@confluent.io> > >>>>> wrote: > >>>>> > >>>>>> Ok, I will change it back to following the code, and update them > >>>>>> together. > >>>>>> > >>>>>> On Tue, Oct 3, 2023 at 2:27 PM Walker Carlson > >>>>>> <wcarl...@confluent.io.invalid> wrote: > >>>>>> > >>>>>>> Hello Hanyu, > >>>>>>> > >>>>>>> Looking over your kip things mostly make sense but I have a couple of > >>>>>>> comments. > >>>>>>> > >>>>>>> > >>>>>>> 1. You have "withDescandingOrder()". I think you mean "descending" > >>>>>>> :) > >>>>>>> Also there are still a few places in the do where its called > >>>>>>> "setReverse" > >>>>>>> 2. Also I like "WithDescendingKeys()" better > >>>>>>> 3. I'm not sure of what ordering guarantees we are offering. > >>>>>>> Perhaps we > >>>>>>> can add a section to the motivation clearly spelling out the > >>>>>>> current > >>>>>>> ordering and the new offering? > >>>>>>> 4. When you say "use unbounded reverseQuery to achieve reverseAll" > >>>>>>> do > >>>>>>> you mean "use unbounded RangeQuery to achieve reverseAll"? as far > >>>>>>> as I can > >>>>>>> tell we don't have a reverseQuery as a named object? > >>>>>>> > >>>>>>> > >>>>>>> Looking good so far > >>>>>>> > >>>>>>> best, > >>>>>>> Walker > >>>>>>> > >>>>>>> On Tue, Oct 3, 2023 at 2:13 PM Colt McNealy <c...@littlehorse.io> > >>>>>>> wrote: > >>>>>>> > >>>>>>>> Hello Hanyu, > >>>>>>>> > >>>>>>>> Thank you for the KIP. I agree with Matthias' proposal to keep the > >>>>>>> naming > >>>>>>>> convention consistent with KIP-969. I favor the > >>>>>>> `.withDescendingKeys()` > >>>>>>>> name. > >>>>>>>> > >>>>>>>> I am curious about one thing. RocksDB guarantees that records > >>>>>>> returned > >>>>>>>> during a range scan are lexicographically ordered by the bytes of > >>>>>>> the keys > >>>>>>>> (either ascending or descending order, as specified in the query). > >>>>>>> This > >>>>>>>> means that results within a single partition are indeed ordered.** > >>>>>>> My > >>>>>>>> reading of KIP-805 suggests to me that you don't need to specify the > >>>>>>>> partition number you are querying in IQv2, which means that you can > >>>>>>> have a > >>>>>>>> valid reversed RangeQuery over a store with "multiple partitions" > >>>>>>> in it. > >>>>>>>> > >>>>>>>> Currently, IQv1 does not guarantee order of keys in this scenario. > >>>>>>> Does > >>>>>>>> IQv2 support ordering across partitions? Such an implementation > >>>>>>> would > >>>>>>>> require opening a rocksdb range scan** on multiple rocksdb > >>>>>>> instances (one > >>>>>>>> per partition), and polling the first key of each. Whether or not > >>>>>>> this is > >>>>>>>> ordered, could we please add that to the documentation? > >>>>>>>> > >>>>>>>> **(How is this implemented/guaranteed in an > >>>>>>> `inMemoryKeyValueStore`? I > >>>>>>>> don't know about that implementation). > >>>>>>>> > >>>>>>>> Colt McNealy > >>>>>>>> > >>>>>>>> *Founder, LittleHorse.dev* > >>>>>>>> > >>>>>>>> > >>>>>>>> On Tue, Oct 3, 2023 at 1:35 PM Hanyu (Peter) Zheng > >>>>>>>> <pzh...@confluent.io.invalid> wrote: > >>>>>>>> > >>>>>>>>> ok, I will update it. Thank you Matthias > >>>>>>>>> > >>>>>>>>> Sincerely, > >>>>>>>>> Hanyu > >>>>>>>>> > >>>>>>>>> On Tue, Oct 3, 2023 at 11:23 AM Matthias J. Sax <mj...@apache.org > >>>>>>>> > >>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>>> Thanks for the KIP Hanyu! > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> I took a quick look and it think the proposal makes sense > >>>>>>> overall. > >>>>>>>>>> > >>>>>>>>>> A few comments about how to structure the KIP. > >>>>>>>>>> > >>>>>>>>>> As you propose to not add `ReverseRangQuery` class, the code > >>>>>>> example > >>>>>>>>>> should go into "Rejected Alternatives" section, not in the > >>>>>>> "Proposed > >>>>>>>>>> Changes" section. > >>>>>>>>>> > >>>>>>>>>> For the `RangeQuery` code example, please omit all existing > >>>>>>> methods > >>>>>>>> etc, > >>>>>>>>>> and only include what will be added/changed. This make it > >>>>>>> simpler to > >>>>>>>>>> read the KIP. > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> nit: typo > >>>>>>>>>> > >>>>>>>>>>> the fault value is false > >>>>>>>>>> > >>>>>>>>>> Should be "the default value is false". > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Not sure if `setReverse()` is the best name. Maybe > >>>>>>>> `withDescandingOrder` > >>>>>>>>>> (or similar, I guess `withReverseOrder` would also work) might > >>>>>>> be > >>>>>>>>>> better? Would be good to align to KIP-969 proposal that suggest > >>>>>>> do use > >>>>>>>>>> `withDescendingKeys` methods for "reverse key-range"; if we go > >>>>>>> with > >>>>>>>>>> `withReverseOrder` we should change KIP-969 accordingly. > >>>>>>>>>> > >>>>>>>>>> Curious to hear what others think about naming this > >>>>>>> consistently across > >>>>>>>>>> both KIPs. > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> -Matthias > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> On 10/3/23 9:17 AM, Hanyu (Peter) Zheng wrote: > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-985%3A+Add+reverseRange+and+reverseAll+query+over+kv-store+in+IQv2 > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> -- > >>>>>>>>> > >>>>>>>>> [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> > >>>>>> > >>>>> > >>>>> > >>>>> -- > >>>>> > >>>>> [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> > >>>> > >>> > >>> > >>> -- > >>> > >>> [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> > >> > > > >
