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 > > > > > >
