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

[Hanyu (Peter) Zheng]

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 update the JavaDocs to avoid
> confusion in the future.
>
>
> -Matthias
>
> On 10/12/23 12:21 PM, Alieh Saeedi wrote:
> > 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 Matthias J. Sax 
> wrote:
> >
> >> 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
> >> and I assume also `prefixScan()` (even if there is no "reverse" version
> >> for it).
> >>
> >>
> >> On 10/11/23 10:22 PM, Matthias J. Sax wrote:
> >>> 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()` methods on
> >>> `ReadOnlyKeyValueStore` -- the interface itself is not typed (ie, just
> >>> generics), and we state that we don't guarantee "logical order" because
> >>> underlying stores are based on `byte[]` type. So far so... well.
> >>>
> >>> However, to make matters worse, we are also not explicit if the
> >>> underlying store implementation *must* return keys is
> >>> byte[]-lexicographical order or not...
> >>>
> >>> For `range()`, I would be kinda willing to accept that there is no
> >>> ordering guarantee at all -- for example, if the underlying
> byte[]-store
> >>> is hash-based and implements a full scan to answer a `range()` it might
> >>> not be efficient, but also not incorrect if keys are be returned in
> some
> >>> "random" (byte[]-)order. In isolation, I don't see an API contract
> >>> violation.
> >>>
> >>> However, `reverseRange` implicitly states with its name, that some
> >>> "descending order" (base on keys) is expected. Given the JavaDoc
> comment
> >>> about "logical" vs "byte[]" order, the contract (at least to me) is
> >>> clear: returns records in descending byte[]-lexicographical order. --
> >>> Any other interpretation seems to be off? Curious to hear if you agree
> >>> or disagree to this interpretation?
> >>>
> >>>
> >>>
> >>> If this is correct, it means we are actually lacking a API contract for
> >>> ascending byte[]-lexicographical range scan. Furthermore, a hash-based
> >>> byte[]-store would need to actually explicitly sort it's result for
> >>> `reverseRange` to not violate the contract.
> >>>
> >>> To me, this raises the question if `range()` actually has a
> >>> (non-explicit) contract about returning data in byte[]-lexicographical
> >>> order? It seems a lot of people rely on this, and our default stores
> >>> actually implement it this way. So if we don't look at `range()` in
> >>> isolation, but look at the `ReadOnlyKeyValueStore` interface
> >>> holistically, I would also buy the argument that `range()` implies
> >>> "ascending "byte[]-lexicographical order". Thoughts?
> >>>
> >>> To be frank: to me, it's pretty clear that the original idea to add
> >>> `range()` was to return data in ascending order.
> >>>
> >>>
> >>> Question 1:
> >>>- Do we believe that the range() contract is ascending
> >>> byte[]-lexicographical order right now?
> >>>
> >>>  If yes, I would propose to make it explicit in the JavaDocs.
> >>>
> >>>  If no, I would also propose to make it explicit in the JavaDocs.
> In
> >>> addition, it raises the question if a method `forwardRange()` (for the
> >>> lack of a better idea about a name right now) is actually missing to
> >>> provide such a contract?
> >>>
> >>>
> >>> Of course, we always depend on the serialization format for order, and
> >>> if users need "logical order" they need to ensure to use a
> serialization
> >>> format that align byte[]-lexicographical order to logical order. But
> for
> >>> the scope of this work, I would not even try to open this can of
> worms...
> >>>
> >>>
> >>>
> >>>
> >>> Looking into `RangeQuery` the JavaDocs don't say anything about order.
> >>> Thus, `RangeQuery#range()` could actually also be implemented by
> calling
> >>> `reverseRange()` without violating the contract as it seems. A
> hash-base
> >>> store could also implement it, without the need to explicitly sort...
> >>>
> >>> What brings be back to my original though 

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

[Matthias J. Sax]

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 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 Matthias J. Sax  wrote:


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
and I assume also `prefixScan()` (even if there is no "reverse" version
for it).


On 10/11/23 10:22 PM, Matthias J. Sax wrote:

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()` methods on
`ReadOnlyKeyValueStore` -- the interface itself is not typed (ie, just
generics), and we state that we don't guarantee "logical order" because
underlying stores are based on `byte[]` type. So far so... well.

However, to make matters worse, we are also not explicit if the
underlying store implementation *must* return keys is
byte[]-lexicographical order or not...

For `range()`, I would be kinda willing to accept that there is no
ordering guarantee at all -- for example, if the underlying byte[]-store
is hash-based and implements a full scan to answer a `range()` it might
not be efficient, but also not incorrect if keys are be returned in some
"random" (byte[]-)order. In isolation, I don't see an API contract
violation.

However, `reverseRange` implicitly states with its name, that some
"descending order" (base on keys) is expected. Given the JavaDoc comment
about "logical" vs "byte[]" order, the contract (at least to me) is
clear: returns records in descending byte[]-lexicographical order. --
Any other interpretation seems to be off? Curious to hear if you agree
or disagree to this interpretation?



If this is correct, it means we are actually lacking a API contract for
ascending byte[]-lexicographical range scan. Furthermore, a hash-based
byte[]-store would need to actually explicitly sort it's result for
`reverseRange` to not violate the contract.

To me, this raises the question if `range()` actually has a
(non-explicit) contract about returning data in byte[]-lexicographical
order? It seems a lot of people rely on this, and our default stores
actually implement it this way. So if we don't look at `range()` in
isolation, but look at the `ReadOnlyKeyValueStore` interface
holistically, I would also buy the argument that `range()` implies
"ascending "byte[]-lexicographical order". Thoughts?

To be frank: to me, it's pretty clear that the original idea to add
`range()` was to return data in ascending order.


Question 1:
   - Do we believe that the range() contract is ascending
byte[]-lexicographical order right now?

 If yes, I would propose to make it explicit in the JavaDocs.

 If no, I would also propose to make it explicit in the JavaDocs. In
addition, it raises the question if a method `forwardRange()` (for the
lack of a better idea about a name right now) is actually missing to
provide such a contract?


Of course, we always depend on the serialization format for order, and
if users need "logical order" they need to ensure to use a serialization
format that align byte[]-lexicographical order to logical order. But for
the scope of this work, I would not even try to open this can of worms...




Looking into `RangeQuery` the JavaDocs don't say anything about order.
Thus, `RangeQuery#range()` could actually also be implemented by calling
`reverseRange()` without violating the contract as it seems. A hash-base
store could also implement it, without the need to explicitly sort...

What brings be back to my original though about having three types of
results for `Range`
   - no ordering guarantee
   - ascending (we would only give byte[]-lexicographical order)
   - descending (we would only give byte[]-lexicographical order)

Again, I actually believe that the original intent of RangeQuery was to
inherit the ascending order of `ReadOnlyKeyValueStore#range()`... Please
keep me honest about it.  On the other hand, both APIs seems to be
independent enough to not couple them... -- this could actually be a
step into the right direction and would follow the underlying idea of
IQv2 to begin with: decouple semantics for the store interfaces from the
query types and semantics...


OR: we actually say that 

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

[Alieh Saeedi]

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 Matthias J. Sax  wrote:

> 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
> and I assume also `prefixScan()` (even if there is no "reverse" version
> for it).
>
>
> On 10/11/23 10:22 PM, Matthias J. Sax wrote:
> > 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()` methods on
> > `ReadOnlyKeyValueStore` -- the interface itself is not typed (ie, just
> > generics), and we state that we don't guarantee "logical order" because
> > underlying stores are based on `byte[]` type. So far so... well.
> >
> > However, to make matters worse, we are also not explicit if the
> > underlying store implementation *must* return keys is
> > byte[]-lexicographical order or not...
> >
> > For `range()`, I would be kinda willing to accept that there is no
> > ordering guarantee at all -- for example, if the underlying byte[]-store
> > is hash-based and implements a full scan to answer a `range()` it might
> > not be efficient, but also not incorrect if keys are be returned in some
> > "random" (byte[]-)order. In isolation, I don't see an API contract
> > violation.
> >
> > However, `reverseRange` implicitly states with its name, that some
> > "descending order" (base on keys) is expected. Given the JavaDoc comment
> > about "logical" vs "byte[]" order, the contract (at least to me) is
> > clear: returns records in descending byte[]-lexicographical order. --
> > Any other interpretation seems to be off? Curious to hear if you agree
> > or disagree to this interpretation?
> >
> >
> >
> > If this is correct, it means we are actually lacking a API contract for
> > ascending byte[]-lexicographical range scan. Furthermore, a hash-based
> > byte[]-store would need to actually explicitly sort it's result for
> > `reverseRange` to not violate the contract.
> >
> > To me, this raises the question if `range()` actually has a
> > (non-explicit) contract about returning data in byte[]-lexicographical
> > order? It seems a lot of people rely on this, and our default stores
> > actually implement it this way. So if we don't look at `range()` in
> > isolation, but look at the `ReadOnlyKeyValueStore` interface
> > holistically, I would also buy the argument that `range()` implies
> > "ascending "byte[]-lexicographical order". Thoughts?
> >
> > To be frank: to me, it's pretty clear that the original idea to add
> > `range()` was to return data in ascending order.
> >
> >
> > Question 1:
> >   - Do we believe that the range() contract is ascending
> > byte[]-lexicographical order right now?
> >
> > If yes, I would propose to make it explicit in the JavaDocs.
> >
> > If no, I would also propose to make it explicit in the JavaDocs. In
> > addition, it raises the question if a method `forwardRange()` (for the
> > lack of a better idea about a name right now) is actually missing to
> > provide such a contract?
> >
> >
> > Of course, we always depend on the serialization format for order, and
> > if users need "logical order" they need to ensure to use a serialization
> > format that align byte[]-lexicographical order to logical order. But for
> > the scope of this work, I would not even try to open this can of worms...
> >
> >
> >
> >
> > Looking into `RangeQuery` the JavaDocs don't say anything about order.
> > Thus, `RangeQuery#range()` could actually also be implemented by calling
> > `reverseRange()` without violating the contract as it seems. A hash-base
> > store could also implement it, without the need to explicitly sort...
> >
> > What brings be back to my original though about having three types of
> > results for `Range`
> >   - no ordering guarantee
> >   - ascending (we would only give byte[]-lexicographical order)
> >   - descending (we would only give byte[]-lexicographical order)
> >
> > Again, I actually believe that the original intent of RangeQuery was to
> > inherit the ascending order of `ReadOnlyKeyValueStore#range()`... Please
> > keep me honest about it.  On the other hand, both APIs seems to be
> > independent enough to not couple them... -- this could actually be a
> > step into the right direction and would follow the underlying idea of
> > IQv2 to begin with: decouple semantics for the store interfaces from the
> > 

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

[Matthias J. Sax]

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 
and I assume also `prefixScan()` (even if there is no "reverse" version 
for it).



On 10/11/23 10:22 PM, Matthias J. Sax wrote:

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()` methods on 
`ReadOnlyKeyValueStore` -- the interface itself is not typed (ie, just 
generics), and we state that we don't guarantee "logical order" because 
underlying stores are based on `byte[]` type. So far so... well.


However, to make matters worse, we are also not explicit if the 
underlying store implementation *must* return keys is 
byte[]-lexicographical order or not...


For `range()`, I would be kinda willing to accept that there is no 
ordering guarantee at all -- for example, if the underlying byte[]-store 
is hash-based and implements a full scan to answer a `range()` it might 
not be efficient, but also not incorrect if keys are be returned in some 
"random" (byte[]-)order. In isolation, I don't see an API contract 
violation.


However, `reverseRange` implicitly states with its name, that some 
"descending order" (base on keys) is expected. Given the JavaDoc comment 
about "logical" vs "byte[]" order, the contract (at least to me) is 
clear: returns records in descending byte[]-lexicographical order. -- 
Any other interpretation seems to be off? Curious to hear if you agree 
or disagree to this interpretation?




If this is correct, it means we are actually lacking a API contract for 
ascending byte[]-lexicographical range scan. Furthermore, a hash-based 
byte[]-store would need to actually explicitly sort it's result for 
`reverseRange` to not violate the contract.


To me, this raises the question if `range()` actually has a 
(non-explicit) contract about returning data in byte[]-lexicographical 
order? It seems a lot of people rely on this, and our default stores 
actually implement it this way. So if we don't look at `range()` in 
isolation, but look at the `ReadOnlyKeyValueStore` interface 
holistically, I would also buy the argument that `range()` implies 
"ascending "byte[]-lexicographical order". Thoughts?


To be frank: to me, it's pretty clear that the original idea to add 
`range()` was to return data in ascending order.



Question 1:
  - Do we believe that the range() contract is ascending 
byte[]-lexicographical order right now?


    If yes, I would propose to make it explicit in the JavaDocs.

    If no, I would also propose to make it explicit in the JavaDocs. In 
addition, it raises the question if a method `forwardRange()` (for the 
lack of a better idea about a name right now) is actually missing to 
provide such a contract?



Of course, we always depend on the serialization format for order, and 
if users need "logical order" they need to ensure to use a serialization 
format that align byte[]-lexicographical order to logical order. But for 
the scope of this work, I would not even try to open this can of worms...





Looking into `RangeQuery` the JavaDocs don't say anything about order. 
Thus, `RangeQuery#range()` could actually also be implemented by calling 
`reverseRange()` without violating the contract as it seems. A hash-base 
store could also implement it, without the need to explicitly sort...


What brings be back to my original though about having three types of 
results for `Range`

  - no ordering guarantee
  - ascending (we would only give byte[]-lexicographical order)
  - descending (we would only give byte[]-lexicographical order)

Again, I actually believe that the original intent of RangeQuery was to 
inherit the ascending order of `ReadOnlyKeyValueStore#range()`... Please 
keep me honest about it.  On the other hand, both APIs seems to be 
independent enough to not couple them... -- this could actually be a 
step into the right direction and would follow the underlying idea of 
IQv2 to begin with: decouple semantics for the store interfaces from the 
query types and semantics...



OR: we actually say that `RangeQuery#range` did implicitly inherit the 
(non explicit) "ascending byte[]-lexicographical" order of the 
underlying `ReadOnlyKeyValueStore`, and we just need to update the 
(Java)Docs to make it explicit. -- But it might go against the idea of 
IQv2 as stated above.



Furthermore, the consequence would be, that a potential custom 
hash-based store, would need to do extra work to `range()` to do the 
sorting (or of course might reject the query as "not supported"). -- Of 
course, a hash-based store would still need to do 

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

[Matthias J. Sax]

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()` methods on 
`ReadOnlyKeyValueStore` -- the interface itself is not typed (ie, just 
generics), and we state that we don't guarantee "logical order" because 
underlying stores are based on `byte[]` type. So far so... well.


However, to make matters worse, we are also not explicit if the 
underlying store implementation *must* return keys is 
byte[]-lexicographical order or not...


For `range()`, I would be kinda willing to accept that there is no 
ordering guarantee at all -- for example, if the underlying byte[]-store 
is hash-based and implements a full scan to answer a `range()` it might 
not be efficient, but also not incorrect if keys are be returned in some 
"random" (byte[]-)order. In isolation, I don't see an API contract 
violation.


However, `reverseRange` implicitly states with its name, that some 
"descending order" (base on keys) is expected. Given the JavaDoc comment 
about "logical" vs "byte[]" order, the contract (at least to me) is 
clear: returns records in descending byte[]-lexicographical order. -- 
Any other interpretation seems to be off? Curious to hear if you agree 
or disagree to this interpretation?




If this is correct, it means we are actually lacking a API contract for 
ascending byte[]-lexicographical range scan. Furthermore, a hash-based 
byte[]-store would need to actually explicitly sort it's result for 
`reverseRange` to not violate the contract.


To me, this raises the question if `range()` actually has a 
(non-explicit) contract about returning data in byte[]-lexicographical 
order? It seems a lot of people rely on this, and our default stores 
actually implement it this way. So if we don't look at `range()` in 
isolation, but look at the `ReadOnlyKeyValueStore` interface 
holistically, I would also buy the argument that `range()` implies 
"ascending "byte[]-lexicographical order". Thoughts?


To be frank: to me, it's pretty clear that the original idea to add 
`range()` was to return data in ascending order.



Question 1:
 - Do we believe that the range() contract is ascending 
byte[]-lexicographical order right now?


   If yes, I would propose to make it explicit in the JavaDocs.

   If no, I would also propose to make it explicit in the JavaDocs. In 
addition, it raises the question if a method `forwardRange()` (for the 
lack of a better idea about a name right now) is actually missing to 
provide such a contract?



Of course, we always depend on the serialization format for order, and 
if users need "logical order" they need to ensure to use a serialization 
format that align byte[]-lexicographical order to logical order. But for 
the scope of this work, I would not even try to open this can of worms...





Looking into `RangeQuery` the JavaDocs don't say anything about order. 
Thus, `RangeQuery#range()` could actually also be implemented by calling 
`reverseRange()` without violating the contract as it seems. A hash-base 
store could also implement it, without the need to explicitly sort...


What brings be back to my original though about having three types of 
results for `Range`

 - no ordering guarantee
 - ascending (we would only give byte[]-lexicographical order)
 - descending (we would only give byte[]-lexicographical order)

Again, I actually believe that the original intent of RangeQuery was to 
inherit the ascending order of `ReadOnlyKeyValueStore#range()`... Please 
keep me honest about it.  On the other hand, both APIs seems to be 
independent enough to not couple them... -- this could actually be a 
step into the right direction and would follow the underlying idea of 
IQv2 to begin with: decouple semantics for the store interfaces from the 
query types and semantics...



OR: we actually say that `RangeQuery#range` did implicitly inherit the 
(non explicit) "ascending byte[]-lexicographical" order of the 
underlying `ReadOnlyKeyValueStore`, and we just need to update the 
(Java)Docs to make it explicit. -- But it might go against the idea of 
IQv2 as stated above.



Furthermore, the consequence would be, that a potential custom 
hash-based store, would need to do extra work to `range()` to do the 
sorting (or of course might reject the query as "not supported"). -- Of 
course, a hash-based store would still need to do extract work to 
implement `ReadOnlyKeyValueStore#(reverse)Range()` correctly (or also 
throw an `UnsupportedOperationException`... -- However, if we keep store 
interface and query interface independent as intended by IQv2, we would 
allow a hash-based store to implement `RangeQuery#range()` even if the 
store does not support `ReadOnlyKeyValueStore#range()` (or only with 
additional sorting cost); such a decoupling sounds like an 

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

[Hanyu (Peter) Zheng]

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 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 interactive queries (specifically, to
> guarantee correctness when doing a paginated range scan. If the order
> changes, then the "bookmark" we use for pagination would be meaningless).
>
> As such, I still think the KIP as you proposed is a highly useful feature.
> I would just make a note of the semantics in the JavaDoc and also in the
> KIP.
>
> Thanks,
> Colt McNealy
>
> *Founder, LittleHorse.dev*
>
>
> On Mon, Oct 9, 2023 at 2:22 PM Hanyu (Peter) Zheng
>  wrote:
>
> > 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 from {@link
> > java.util.ConcurrentModificationException}s
> >  * and must not return null values.
> >  ** Order is not guaranteed as bytes lexicographical ordering might
> not
> > represent key order.*
> >  *
> >  * @param from The first key that could be in the range, where
> > iteration starts from.
> >  * A null value indicates that the range starts with the
> > first element in the store.
> >  * @param to   The last key that could be in the range, where
> iteration
> > ends.
> >  * A null value indicates that the range ends with the
> last
> > element in the store.
> >  * @return The iterator for this range, from smallest to largest
> bytes.
> >  * @throws InvalidStateStoreException if the store is not initialized
> >  */
> > KeyValueIterator range(K from, K to);
> >
> > /**
> >  * Get a reverse iterator over a given range of keys. This iterator
> > must be closed after use.
> >  * The returned iterator must be safe from {@link
> > java.util.ConcurrentModificationException}s
> >  * and must not return null values.
> >  * *Order is not guaranteed as bytes lexicographical ordering might
> not
> > represent key order.*
> >  *
> >  * @param from The first key that could be in the range, where
> > iteration ends.
> >  * A null value indicates that the range starts with the
> > first element in the store.
> >  * @param to   The last key that could be in the range, where
> iteration
> > starts from.
> >  * A null value indicates that the range ends with the
> last
> > element in the store.
> >  * @return The reverse iterator for this range, from largest to
> > smallest key bytes.
> >  * @throws InvalidStateStoreException if the store is not initialized
> >  */
> > default KeyValueIterator reverseRange(K from, K to) {
> > throw new UnsupportedOperationException();
> > }
> >
> > The query methods of RangeQuery ultimately invoke either the range method
> > or the reverseRange method. However, as per the JavaDoc: the order is not
> > guaranteed, since byte lexicographical ordering may not correspond to the
> > actual key order.
> >
> > Sincerely,
> > Hanyu
> >
> > On Fri, Oct 6, 2023 at 10:00 AM Hanyu (Peter) Zheng  >
> > wrote:
> >
> > > 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 <2,2>.
> > > Partition1 contains key-value pairs: <1,1> and <3,3>.
> > > When executing RangeQuery.withRange(1,3), the results are:
> > >
> > > Partition0: [2]
> > > Partition1: [1, 3]
> > > To support functionalities like reverseRange and reverseAll, we can
> > > introduce the withDescendingKeys() method. For instance, using
> > > RangeQuery.withRange(1,3).withDescendingKeys(), the anticipated results
> > are:
> > >
> > > Partition0: [2]
> > > Partition1: [3, 1]
> > >
> > > In response to Hao's inquiry about the boundary issue, please refer to
> > the
> > > StoreQueryUtils class. The code snippet:
> > >
> > > iterator = kvStore.range(lowerRange.orElse(null),
> > upperRange.orElse(null));
> > > indicates that when implementing range in each store, it's structured
> > like:
> > >
> > > @Override
> > > public KeyValueIterator range(final Bytes from, final
> > Bytes
> > > to) {
> > > if (from != null && to != null && 

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

[Colt McNealy]

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 interactive queries (specifically, to
guarantee correctness when doing a paginated range scan. If the order
changes, then the "bookmark" we use for pagination would be meaningless).

As such, I still think the KIP as you proposed is a highly useful feature.
I would just make a note of the semantics in the JavaDoc and also in the
KIP.

Thanks,
Colt McNealy

*Founder, LittleHorse.dev*


On Mon, Oct 9, 2023 at 2:22 PM Hanyu (Peter) Zheng
 wrote:

> 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 from {@link
> java.util.ConcurrentModificationException}s
>  * and must not return null values.
>  ** Order is not guaranteed as bytes lexicographical ordering might not
> represent key order.*
>  *
>  * @param from The first key that could be in the range, where
> iteration starts from.
>  * A null value indicates that the range starts with the
> first element in the store.
>  * @param to   The last key that could be in the range, where iteration
> ends.
>  * A null value indicates that the range ends with the last
> element in the store.
>  * @return The iterator for this range, from smallest to largest bytes.
>  * @throws InvalidStateStoreException if the store is not initialized
>  */
> KeyValueIterator range(K from, K to);
>
> /**
>  * Get a reverse iterator over a given range of keys. This iterator
> must be closed after use.
>  * The returned iterator must be safe from {@link
> java.util.ConcurrentModificationException}s
>  * and must not return null values.
>  * *Order is not guaranteed as bytes lexicographical ordering might not
> represent key order.*
>  *
>  * @param from The first key that could be in the range, where
> iteration ends.
>  * A null value indicates that the range starts with the
> first element in the store.
>  * @param to   The last key that could be in the range, where iteration
> starts from.
>  * A null value indicates that the range ends with the last
> element in the store.
>  * @return The reverse iterator for this range, from largest to
> smallest key bytes.
>  * @throws InvalidStateStoreException if the store is not initialized
>  */
> default KeyValueIterator reverseRange(K from, K to) {
> throw new UnsupportedOperationException();
> }
>
> The query methods of RangeQuery ultimately invoke either the range method
> or the reverseRange method. However, as per the JavaDoc: the order is not
> guaranteed, since byte lexicographical ordering may not correspond to the
> actual key order.
>
> Sincerely,
> Hanyu
>
> On Fri, Oct 6, 2023 at 10:00 AM Hanyu (Peter) Zheng 
> wrote:
>
> > 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 <2,2>.
> > Partition1 contains key-value pairs: <1,1> and <3,3>.
> > When executing RangeQuery.withRange(1,3), the results are:
> >
> > Partition0: [2]
> > Partition1: [1, 3]
> > To support functionalities like reverseRange and reverseAll, we can
> > introduce the withDescendingKeys() method. For instance, using
> > RangeQuery.withRange(1,3).withDescendingKeys(), the anticipated results
> are:
> >
> > Partition0: [2]
> > Partition1: [3, 1]
> >
> > In response to Hao's inquiry about the boundary issue, please refer to
> the
> > StoreQueryUtils class. The code snippet:
> >
> > iterator = kvStore.range(lowerRange.orElse(null),
> upperRange.orElse(null));
> > indicates that when implementing range in each store, it's structured
> like:
> >
> > @Override
> > public KeyValueIterator range(final Bytes from, final
> Bytes
> > to) {
> > if (from != null && to != null && from.compareTo(to) > 0) {
> > This section performs the necessary checks.
> >
> > Sincerely,
> > Hanyu
> >
> > On Thu, Oct 5, 2023 at 9:52 AM Hanyu (Peter) Zheng 
> > wrote:
> >
> >> 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

[Hanyu (Peter) Zheng]

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 from {@link
java.util.ConcurrentModificationException}s
 * and must not return null values.
 ** Order is not guaranteed as bytes lexicographical ordering might not
represent key order.*
 *
 * @param from The first key that could be in the range, where
iteration starts from.
 * A null value indicates that the range starts with the
first element in the store.
 * @param to   The last key that could be in the range, where iteration
ends.
 * A null value indicates that the range ends with the last
element in the store.
 * @return The iterator for this range, from smallest to largest bytes.
 * @throws InvalidStateStoreException if the store is not initialized
 */
KeyValueIterator range(K from, K to);

/**
 * Get a reverse iterator over a given range of keys. This iterator
must be closed after use.
 * The returned iterator must be safe from {@link
java.util.ConcurrentModificationException}s
 * and must not return null values.
 * *Order is not guaranteed as bytes lexicographical ordering might not
represent key order.*
 *
 * @param from The first key that could be in the range, where
iteration ends.
 * A null value indicates that the range starts with the
first element in the store.
 * @param to   The last key that could be in the range, where iteration
starts from.
 * A null value indicates that the range ends with the last
element in the store.
 * @return The reverse iterator for this range, from largest to
smallest key bytes.
 * @throws InvalidStateStoreException if the store is not initialized
 */
default KeyValueIterator reverseRange(K from, K to) {
throw new UnsupportedOperationException();
}

The query methods of RangeQuery ultimately invoke either the range method
or the reverseRange method. However, as per the JavaDoc: the order is not
guaranteed, since byte lexicographical ordering may not correspond to the
actual key order.

Sincerely,
Hanyu

On Fri, Oct 6, 2023 at 10:00 AM Hanyu (Peter) Zheng 
wrote:

> 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 <2,2>.
> Partition1 contains key-value pairs: <1,1> and <3,3>.
> When executing RangeQuery.withRange(1,3), the results are:
>
> Partition0: [2]
> Partition1: [1, 3]
> To support functionalities like reverseRange and reverseAll, we can
> introduce the withDescendingKeys() method. For instance, using
> RangeQuery.withRange(1,3).withDescendingKeys(), the anticipated results are:
>
> Partition0: [2]
> Partition1: [3, 1]
>
> In response to Hao's inquiry about the boundary issue, please refer to the
> StoreQueryUtils class. The code snippet:
>
> iterator = kvStore.range(lowerRange.orElse(null), upperRange.orElse(null));
> indicates that when implementing range in each store, it's structured like:
>
> @Override
> public KeyValueIterator range(final Bytes from, final Bytes
> to) {
> if (from != null && to != null && from.compareTo(to) > 0) {
> This section performs the necessary checks.
>
> Sincerely,
> Hanyu
>
> On Thu, Oct 5, 2023 at 9:52 AM Hanyu (Peter) Zheng 
> wrote:
>
>> 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.
>>>
>>>
>>> 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
>>> partitions bases). It just happens that RocksDB (and as pointed out by
>>> Hanyu already, also the built-in in-memory store that is base on a
>>> tree-map) allows us to return data ordered by key; as mentioned already,
>>> this guarantee is limited on a per partition basis.
>>>
>>> If there would be custom store base on a hashed key-value store, this
>>> store could implement RangeQuery and return data (even for a single
>>> partition) with no ordering, without violating the contract.
>>>
>>>
>>>
>>> Thus, it could actually make sense, to extend `RangeQuery` and allow
>>> three options: no-order, ascending, descending. For our existing
>>> Rocks/InMemory implementations, no-order could be equal to ascending and
>>> nothing changes effectively, but it might be a better API contract? --
>>> If we assume that there 

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

[Hanyu (Peter) Zheng]

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 <2,2>.
Partition1 contains key-value pairs: <1,1> and <3,3>.
When executing RangeQuery.withRange(1,3), the results are:

Partition0: [2]
Partition1: [1, 3]
To support functionalities like reverseRange and reverseAll, we can
introduce the withDescendingKeys() method. For instance, using
RangeQuery.withRange(1,3).withDescendingKeys(), the anticipated results are:

Partition0: [2]
Partition1: [3, 1]

In response to Hao's inquiry about the boundary issue, please refer to the
StoreQueryUtils class. The code snippet:

iterator = kvStore.range(lowerRange.orElse(null), upperRange.orElse(null));
indicates that when implementing range in each store, it's structured like:

@Override
public KeyValueIterator range(final Bytes from, final Bytes
to) {
if (from != null && to != null && from.compareTo(to) > 0) {
This section performs the necessary checks.

Sincerely,
Hanyu

On Thu, Oct 5, 2023 at 9:52 AM Hanyu (Peter) Zheng 
wrote:

> 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.
>>
>>
>> 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
>> partitions bases). It just happens that RocksDB (and as pointed out by
>> Hanyu already, also the built-in in-memory store that is base on a
>> tree-map) allows us to return data ordered by key; as mentioned already,
>> this guarantee is limited on a per partition basis.
>>
>> If there would be custom store base on a hashed key-value store, this
>> store could implement RangeQuery and return data (even for a single
>> partition) with no ordering, without violating the contract.
>>
>>
>>
>> Thus, it could actually make sense, to extend `RangeQuery` and allow
>> three options: no-order, ascending, descending. For our existing
>> Rocks/InMemory implementations, no-order could be equal to ascending and
>> nothing changes effectively, but it might be a better API contract? --
>> If we assume that there might be a custom hash-based store, such a store
>> could reject a query if "ascending" is required, or might need to do
>> more work to implement it (up to the store maintainer). This is actually
>> the beauty of IQv2 that different stores can pick what queries they want
>> to support.
>>
>>  From an API contract point of view, it seems confusing to say:
>> specifying nothing means no guarantee (or ascending if the store can
>> offer it), but descending can we explicitly request. Thus, a hash-based
>> store, might be able to accept "order not specified query", but would
>> reject "descending". This seems to be somewhat unbalanced?
>>
>> Thus, I am wondering if we should actually add `withAscendingKeys()`,
>> too, even if it won't impact our current RocksDB/In-Memory
>> implementations?
>>
>>
>> The second question is about per-partition or across-partition ordering:
>> it's not possible right now to actually offer across-partition ordering
>> the way IQv2 is setup. The reason is, that the store that implements a
>> query type, is always a single shard. Thus, the implementation does not
>> have access to other shards. It's hard-coded inside Kafka Streams, to
>> query each shared, and to "accumulate" partial results, and return the
>> back to the user. Note that the API is:
>>
>>
>> > StateQueryResult result = KafkaStreams.query(...);
>> > Map> resultPerPartitions =
>> result.getPartitionResults();
>>
>>
>> Thus, if we would want to offer across-partition ordering, we cannot do
>> it right now, because Kafka Streams does not know anything about the
>> semantics of the query it distributes... -- the result is an unknown
>> type . We would need to extend IQv2 with an additional mechanism,
>> that allows users to plug in more custom code to "merge" multiple
>> partitions result into a "global result". This is clearly out-of-scope
>> for this KIP and would require a new KIP by itself.
>>
>> I seems that this contract, which is independent of the query type is
>> not well understood, and thus a big +1 to fix the documentation. I don't
>> think that this KIP must "define" anything, but it might of course be
>> worth to add the explanation why the KIP cannot even offer
>> global-ordering, as it's defined/limited by the IQv2 "framework" itself,
>> not the individual queries.
>>
>>
>>
>> -Matthias
>>
>>
>>
>>
>> On 10/4/23 4:38 PM, Hao Li wrote:
>> > Hi Hanyu,
>> >
>> > Thanks for the KIP! Seems there are already a lot of good discussions. I
>> > only have 

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

[Hanyu (Peter) Zheng]

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.
>
>
> 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
> partitions bases). It just happens that RocksDB (and as pointed out by
> Hanyu already, also the built-in in-memory store that is base on a
> tree-map) allows us to return data ordered by key; as mentioned already,
> this guarantee is limited on a per partition basis.
>
> If there would be custom store base on a hashed key-value store, this
> store could implement RangeQuery and return data (even for a single
> partition) with no ordering, without violating the contract.
>
>
>
> Thus, it could actually make sense, to extend `RangeQuery` and allow
> three options: no-order, ascending, descending. For our existing
> Rocks/InMemory implementations, no-order could be equal to ascending and
> nothing changes effectively, but it might be a better API contract? --
> If we assume that there might be a custom hash-based store, such a store
> could reject a query if "ascending" is required, or might need to do
> more work to implement it (up to the store maintainer). This is actually
> the beauty of IQv2 that different stores can pick what queries they want
> to support.
>
>  From an API contract point of view, it seems confusing to say:
> specifying nothing means no guarantee (or ascending if the store can
> offer it), but descending can we explicitly request. Thus, a hash-based
> store, might be able to accept "order not specified query", but would
> reject "descending". This seems to be somewhat unbalanced?
>
> Thus, I am wondering if we should actually add `withAscendingKeys()`,
> too, even if it won't impact our current RocksDB/In-Memory implementations?
>
>
> The second question is about per-partition or across-partition ordering:
> it's not possible right now to actually offer across-partition ordering
> the way IQv2 is setup. The reason is, that the store that implements a
> query type, is always a single shard. Thus, the implementation does not
> have access to other shards. It's hard-coded inside Kafka Streams, to
> query each shared, and to "accumulate" partial results, and return the
> back to the user. Note that the API is:
>
>
> > StateQueryResult result = KafkaStreams.query(...);
> > Map> resultPerPartitions =
> result.getPartitionResults();
>
>
> Thus, if we would want to offer across-partition ordering, we cannot do
> it right now, because Kafka Streams does not know anything about the
> semantics of the query it distributes... -- the result is an unknown
> type . We would need to extend IQv2 with an additional mechanism,
> that allows users to plug in more custom code to "merge" multiple
> partitions result into a "global result". This is clearly out-of-scope
> for this KIP and would require a new KIP by itself.
>
> I seems that this contract, which is independent of the query type is
> not well understood, and thus a big +1 to fix the documentation. I don't
> think that this KIP must "define" anything, but it might of course be
> worth to add the explanation why the KIP cannot even offer
> global-ordering, as it's defined/limited by the IQv2 "framework" itself,
> not the individual queries.
>
>
>
> -Matthias
>
>
>
>
> On 10/4/23 4:38 PM, Hao Li wrote:
> > 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 of the range
> >   * @param upper The key that specifies the upper bound of the range
> >   * @param  The key type
> >   * @param  The value type
> >   */
> >  public static  RangeQuery withRange(final K lower,
> final K
> > upper) {
> >  return new RangeQuery<>(Optional.ofNullable(lower),
> > Optional.ofNullable(upper), true);
> >  }
> > ```
> > that a `null` in lower or upper parameter means it's unbounded.
> > 2. What's the behavior if lower is 3 and upper is 1? Is it
> IllegalArgument
> > or will this return an empty result? Maybe also clarify this in the
> > document.
> >
> > Thanks,
> > Hao
> >
> >
> > On Wed, Oct 4, 2023 at 9:27 AM Hanyu (Peter) Zheng
> >  wrote:
> >
> >> 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, 3>.
> However,
> >> if we run 

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

[Matthias J. Sax]

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 
partitions bases). It just happens that RocksDB (and as pointed out by 
Hanyu already, also the built-in in-memory store that is base on a 
tree-map) allows us to return data ordered by key; as mentioned already, 
this guarantee is limited on a per partition basis.


If there would be custom store base on a hashed key-value store, this 
store could implement RangeQuery and return data (even for a single 
partition) with no ordering, without violating the contract.




Thus, it could actually make sense, to extend `RangeQuery` and allow 
three options: no-order, ascending, descending. For our existing 
Rocks/InMemory implementations, no-order could be equal to ascending and 
nothing changes effectively, but it might be a better API contract? -- 
If we assume that there might be a custom hash-based store, such a store 
could reject a query if "ascending" is required, or might need to do 
more work to implement it (up to the store maintainer). This is actually 
the beauty of IQv2 that different stores can pick what queries they want 
to support.


From an API contract point of view, it seems confusing to say: 
specifying nothing means no guarantee (or ascending if the store can 
offer it), but descending can we explicitly request. Thus, a hash-based 
store, might be able to accept "order not specified query", but would 
reject "descending". This seems to be somewhat unbalanced?


Thus, I am wondering if we should actually add `withAscendingKeys()`, 
too, even if it won't impact our current RocksDB/In-Memory implementations?



The second question is about per-partition or across-partition ordering: 
it's not possible right now to actually offer across-partition ordering 
the way IQv2 is setup. The reason is, that the store that implements a 
query type, is always a single shard. Thus, the implementation does not 
have access to other shards. It's hard-coded inside Kafka Streams, to 
query each shared, and to "accumulate" partial results, and return the 
back to the user. Note that the API is:




StateQueryResult result = KafkaStreams.query(...);
Map> resultPerPartitions = result.getPartitionResults();



Thus, if we would want to offer across-partition ordering, we cannot do 
it right now, because Kafka Streams does not know anything about the 
semantics of the query it distributes... -- the result is an unknown 
type . We would need to extend IQv2 with an additional mechanism, 
that allows users to plug in more custom code to "merge" multiple 
partitions result into a "global result". This is clearly out-of-scope 
for this KIP and would require a new KIP by itself.


I seems that this contract, which is independent of the query type is 
not well understood, and thus a big +1 to fix the documentation. I don't 
think that this KIP must "define" anything, but it might of course be 
worth to add the explanation why the KIP cannot even offer 
global-ordering, as it's defined/limited by the IQv2 "framework" itself, 
not the individual queries.




-Matthias




On 10/4/23 4:38 PM, Hao Li wrote:

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 of the range
  * @param upper The key that specifies the upper bound of the range
  * @param  The key type
  * @param  The value type
  */
 public static  RangeQuery withRange(final K lower, final K
upper) {
 return new RangeQuery<>(Optional.ofNullable(lower),
Optional.ofNullable(upper), true);
 }
```
that a `null` in lower or upper parameter means it's unbounded.
2. What's the behavior if lower is 3 and upper is 1? Is it IllegalArgument
or will this return an empty result? Maybe also clarify this in the
document.

Thanks,
Hao


On Wed, Oct 4, 2023 at 9:27 AM Hanyu (Peter) Zheng
 wrote:


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, 3>. However,
if we run withRange(1,3).withDescendingKeys(), and still use a Set, the
result will again be a Set of <1,2,3>. This means we won't be able to
determine whether the results have been reversed.

To resolve this ambiguity, I've switched to using a List to record the
results, ensuring the order of retrieval from partitions p0 and p1. So,
withRange(1,3) would yield a List of [2, 1, 3], whereas
withRange(1,3).withDescendingKeys() 

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

[Hao Li]

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 of the range
 * @param upper The key that specifies the upper bound of the range
 * @param  The key type
 * @param  The value type
 */
public static  RangeQuery withRange(final K lower, final K
upper) {
return new RangeQuery<>(Optional.ofNullable(lower),
Optional.ofNullable(upper), true);
}
```
that a `null` in lower or upper parameter means it's unbounded.
2. What's the behavior if lower is 3 and upper is 1? Is it IllegalArgument
or will this return an empty result? Maybe also clarify this in the
document.

Thanks,
Hao


On Wed, Oct 4, 2023 at 9:27 AM Hanyu (Peter) Zheng
 wrote:

> 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, 3>. However,
> if we run withRange(1,3).withDescendingKeys(), and still use a Set, the
> result will again be a Set of <1,2,3>. This means we won't be able to
> determine whether the results have been reversed.
>
> To resolve this ambiguity, I've switched to using a List to record the
> results, ensuring the order of retrieval from partitions p0 and p1. So,
> withRange(1,3) would yield a List of [2, 1, 3], whereas
> withRange(1,3).withDescendingKeys() would produce a List of [2,3,1].
>
> This ordering makes sense since RocksDB sorts its keys, and InMemoryStore
> uses a TreeMap structure, which means the keys are already sorted.
>
> Sincerely,
> Hanyu
>
> On Wed, Oct 4, 2023 at 9:25 AM Hanyu (Peter) Zheng 
> wrote:
>
> > 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 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 

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

[Hanyu (Peter) Zheng]

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, 3>. However,
if we run withRange(1,3).withDescendingKeys(), and still use a Set, the
result will again be a Set of <1,2,3>. This means we won't be able to
determine whether the results have been reversed.

To resolve this ambiguity, I've switched to using a List to record the
results, ensuring the order of retrieval from partitions p0 and p1. So,
withRange(1,3) would yield a List of [2, 1, 3], whereas
withRange(1,3).withDescendingKeys() would produce a List of [2,3,1].

This ordering makes sense since RocksDB sorts its keys, and InMemoryStore
uses a TreeMap structure, which means the keys are already sorted.

Sincerely,
Hanyu

On Wed, Oct 4, 2023 at 9:25 AM Hanyu (Peter) Zheng 
wrote:

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

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

[Hanyu (Peter) Zheng]

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

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

[Hanyu (Peter) Zheng]

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, 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
>> 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 
>> wrote:
>>
>>> 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 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 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
>>  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 
>>> 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

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

[Lucas Brutschy]

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  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 query = RangeQuery.withRange(1,
> 1000).withDescendingKeys();
>
> I think this is more readable than:
>
> final RangeQuery 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 
> > 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 
> >> wrote:
> >>
> >>> 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 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 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 
> > 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
> >>  wrote:
> >>
> >>> Hello Hanyu,
> >>>
> >>> Looking over your kip things mostly make sense but I have a couple of
> >>> comments.
> >>>
> >>>
> >>>

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

[Bruno Cadonna]

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 query = RangeQuery.withRange(1, 
1000).withDescendingKeys();


I think this is more readable than:

final RangeQuery 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 
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 
wrote:


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

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

[Hanyu (Peter) Zheng]

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

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

[Hanyu (Peter) Zheng]

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

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

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

[Hanyu (Peter) Zheng]

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

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

[Hanyu (Peter) Zheng]

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

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

[Hanyu (Peter) Zheng]

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 
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
>  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  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
>> >  wrote:
>> >
>> > > 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 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] 
>> > > 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_medium=email_campaign=ch.email-signature_type.community_content.blog
>> > > >[image:
>> > > Twitter] [image: LinkedIn]
>> > > 

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

[Hanyu (Peter) Zheng]

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 "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  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
> >  wrote:
> >
> > > 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 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] 
> > > 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_medium=email_campaign=ch.email-signature_type.community_content.blog
> > > >[image:
> > > Twitter] [image: LinkedIn]
> > > [image: Slack]
> > > [image: YouTube]
> > > 
> > >
> > > [image: Try Confluent Cloud for Free]
> > > <
> > >
> >
> https://www.confluent.io/get-started?utm_campaign=tm.fm-apac_cd.inbound_source=gmail_medium=organic
> > > >
> > >
> >
>


-- 

[image: Confluent] 
Hanyu (Peter) Zheng he/him/his
Software Engineer Intern
+1 (213) 431-7193 <+1+(213)+431-7193>
Follow us: [image: Blog]

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

[Hanyu (Peter) Zheng]

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 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 prefer “withDescendingKeys()”  to “withDescendingOrder()”
>- I also agree that we should include a section on ordering, but it
>should be fairly straightforward.  The “StateQueryRequest” of IQv2
> allows
>users to specify a partition or partitions, so if ordering is important
>they can elect to provide a single partition in the query.
>
>
> Thanks,
> Bill
>
> On Tue, Oct 3, 2023 at 5: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 "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  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
> > >  wrote:
> > >
> > > > 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 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
> > > > > >
> > > 

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

[Bill Bejeck]

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 prefer “withDescendingKeys()”  to “withDescendingOrder()”
   - I also agree that we should include a section on ordering, but it
   should be fairly straightforward.  The “StateQueryRequest” of IQv2 allows
   users to specify a partition or partitions, so if ordering is important
   they can elect to provide a single partition in the query.


Thanks,
Bill

On Tue, Oct 3, 2023 at 5: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 "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  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
> >  wrote:
> >
> > > 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 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] 
> > > 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_medium=email_campaign=ch.email-signature_type.community_content.blog
> > > >[image:
> > > Twitter] [image: LinkedIn]
> > > 

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

[Walker Carlson]

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  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
>  wrote:
>
> > 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 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] 
> > 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_medium=email_campaign=ch.email-signature_type.community_content.blog
> > >[image:
> > Twitter] [image: LinkedIn]
> > [image: Slack]
> > [image: YouTube]
> > 
> >
> > [image: Try Confluent Cloud for Free]
> > <
> >
> https://www.confluent.io/get-started?utm_campaign=tm.fm-apac_cd.inbound_source=gmail_medium=organic
> > >
> >
>

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

[Colt McNealy]

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

> 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 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] 
> 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_medium=email_campaign=ch.email-signature_type.community_content.blog
> >[image:
> Twitter] [image: LinkedIn]
> [image: Slack]
> [image: YouTube]
> 
>
> [image: Try Confluent Cloud for Free]
> <
> https://www.confluent.io/get-started?utm_campaign=tm.fm-apac_cd.inbound_source=gmail_medium=organic
> >
>

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

[Hanyu (Peter) Zheng]

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 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] 
Hanyu (Peter) Zheng he/him/his
Software Engineer Intern
+1 (213) 431-7193 <+1+(213)+431-7193>
Follow us: [image: Blog]
[image:
Twitter] [image: LinkedIn]
[image: Slack]
[image: YouTube]


[image: Try Confluent Cloud for Free]

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

[Matthias J. Sax]

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

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

[Hanyu (Peter) Zheng]

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]
[image:
Twitter] [image: LinkedIn]
[image: Slack]
[image: YouTube]


[image: Try Confluent Cloud for Free]