Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Thank you, Matthias and Alieh, I've initiated a vote. Sincerely, Hanyu On Fri, Oct 13, 2023 at 9:14 AM Matthias J. Sax wrote: > Thanks for pointing this out Alieh! I totally missed this. > > So I guess everything is settled and Hanyu can start a VOTE? > > For the KIP PR, we should ensure to

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Thanks for pointing this out Alieh! I totally missed this. So I guess everything is settled and Hanyu can start a VOTE? For the KIP PR, we should ensure to update the JavaDocs to avoid confusion in the future. -Matthias On 10/12/23 12:21 PM, Alieh Saeedi wrote: Hi, just pointing to

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Hi, just pointing to javadocs for range() and reverseRange(): range(): *@return The iterator for this range, from smallest to largest bytes.* reverseRange(): * @return The reverse iterator for this range, from largest to smallest key bytes. Cheers, Alieh On Thu, Oct 12, 2023 at 7:32 AM

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Quick addendum. Some DSL operator use `range` and seems to rely on ascending order, too... Of course, if we say `range()` has no ordering guarantee, we would add `forwardRange()` and let the DSL use `forwardRange`, too. The discussion of course also applies to `all()` and `reveserAll()`, and

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Thanks for raising this question Hanyu. Great find! My interpretation is as follows (it's actually a warning signal that the API contract is not better defined, and we should fix this by extending JavaDocs and docs on the web page about it). We have existing `range()` and `reverseRange()`

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Thank you Colt, At first, we misinterpreted the JavaDoc. Upon further discussion, we realized that after the key is converted to bytes, queries are based on the key's byte order, not its intrinsic order. Sincerely, Hanyu On Mon, Oct 9, 2023 at 6:55 PM Colt McNealy wrote: > Hanyu, > > I like

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Hanyu, I like the attention to detail! It is correct that the JavaDoc does not "guarantee" order. However, the public API contract specified in the javadoc does mention lexicographical ordering of the bytes, which is a useful API contract. In our Streams app we make use of that contract during

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

After our discussion, we discovered something intriguing. The definitions for the range and reverseRange methods in the ReadOnlyKeyValueStore are as follows: /** * Get an iterator over a given range of keys. This iterator must be closed after use. * The returned iterator must be safe

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Thank you, Matthias, for the detailed implementation and explanation. As of now, our capability is limited to executing interactive queries on individual partitions. To illustrate: Consider the IQv2StoreIntegrationTest: We have two partitions: Partition0 contains key-value pairs: <0,0> and

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Hi, Hao, In this case, it will return an empty set or list in the end. Sincerely, Hanyu On Wed, Oct 4, 2023 at 10:29 PM Matthias J. Sax wrote: > Great discussion! > > It seems the only open question might be about ordering guarantees? > IIRC, we had a discussion about this in the past. > > >

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Great discussion! It seems the only open question might be about ordering guarantees? IIRC, we had a discussion about this in the past. Technically (at least from my POV), existing `RangeQuery` does not have a guarantee that data is return in any specific order (not even on a per

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Hi Hanyu, Thanks for the KIP! Seems there are already a lot of good discussions. I only have two comments: 1. Please make it clear in ``` /** * Interactive range query using a lower and upper bound to filter the keys returned. * @param lower The key that specifies the lower bound

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

For testing purposes, we previously used a Set to record the results in IQv2StoreIntegrationTest. Let's take an example where we now have two partitions and four key-value pairs: <0,0> in p0, <1,1> in p1, <2,2> in p0, and <3,3> in p1. If we execute withRange(1,3), it will return a Set of <1, 2,

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Hi, Bruno Thank you for your suggestions, I will update them soon. Sincerely, Hanyu On Wed, Oct 4, 2023 at 9:25 AM Hanyu (Peter) Zheng wrote: > Hi, Lucas, > > Thank you for your suggestions. > I will update the KIP and code together. > > Sincerely, > Hanyu > > On Tue, Oct 3, 2023 at 8:16 PM

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Hi, Lucas, Thank you for your suggestions. I will update the KIP and code together. Sincerely, Hanyu On Tue, Oct 3, 2023 at 8:16 PM Hanyu (Peter) Zheng wrote: > If we use WithDescendingKeys() to generate a RangeQuery to do the > reveseQuery, how do we achieve the methods like withRange,

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

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

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

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

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

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 wrote: > I believe there's no need to introduce a method like

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

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,

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Hi, Colt, The underlying structure of inMemoryKeyValueStore is treeMap. Sincerely, Hanyu On Tue, Oct 3, 2023 at 4:34 PM Hanyu (Peter) Zheng 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

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

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 wrote: > Hi, Walker, > > 1. I will

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

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

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

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

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Ok, I just talked about it with Matthias, I will change the text back to following the code, and update them together. Sincerely, Hanyu On Tue, Oct 3, 2023 at 3:49 PM Bill Bejeck wrote: > Hi Hanyu, > > Thanks for the KIP, overall it's looking good, but I have a couple of > comments > >- In

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

Hi Hanyu, Thanks for the KIP, overall it's looking good, but I have a couple of comments - In the “Proposed Changes” section there's a reference to `setReverse()` but the code example has “withDescendingOrder()” so I think the text needs an update to reflect the code example. - I

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

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

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

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

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

ok, I will update it. Thank you Matthias Sincerely, Hanyu On Tue, Oct 3, 2023 at 11:23 AM Matthias J. Sax 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

Re: [DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

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"

[DISCUSS] KIP-985 Add reverseRange and reverseAll query over kv-store in IQv2

https://cwiki.apache.org/confluence/display/KAFKA/KIP-985%3A+Add+reverseRange+and+reverseAll+query+over+kv-store+in+IQv2 -- [image: Confluent] Hanyu (Peter) Zheng he/him/his Software Engineer Intern +1 (213) 431-7193 <+1+(213)+431-7193> Follow us: [image: Blog]