Thanks, Matthias and Bruno. Here is a list of updates: 1. I changed the variable and method names as I did for KIP-968, as follows: - "fromTimestamp" -> fromTime - "asOfTimestamp"-> toTime - "from(instant)" -> fromTime(instant)" - asOf(instant)"->toTime(instant) 2. As Bruno suggested for KIP-968, I added `orderByKey()`, `withAscendingKeys()`, and `withAscendingTimestamps()` methods for user readability. 3. I updated the "Example" section as well.
Some points: 1. Even though the kip suggests adding the `get(k lowerkeybound, k upperkeybound, long fromtime, long totime)` method to the interface, I added this method to the `rocksdbversionedstore` class for now. 2. Matthias, you mentioned a very important point. How can a user retrieve the latest value? We have the same issue with kip-968 as well. Asking a user to call `toTime(max)` violates the API design rules, as you mentioned. So I think we must have a `latest()` method for both KIP-968 and KIP-969. What do you think about that? Cheers, Alieh On Thu, Oct 12, 2023 at 6:33 AM Matthias J. Sax <mj...@apache.org> wrote: > Thanks for the update. > > > > To retrieve > >> the latest value(s), the user must call just the asOf method with > the MAX > >> value (asOf(MAX)). The same applies to KIP-968. Do you think it is > clumsy, > >> Matthias? > > > Well, in KIP-968 calling `asOf` and passing in a timestamp is optional, > and default is "latest", right? So while `asOf(MAX)` does the same > thing, practically users would never call `asOf` for a "latest" query? > > In this KIP, we enforce that users give us a key range (we have the 4 > static entry point methods to define a query for this), and we say we > default to "no bounds" for time range by default. > > The existing `RangeQuery` allows to query a range of keys for existing > stores. It seems to be a common pattern to query a key-range on latest. > -- in the current proposal, users would need to do: > > MultiVersionedRangeQuery.withKeyRange(startKey, endKey).asOf(MAX); > > Would like to hear from others if we think that's good user experience? > If we agree to accept this, I think we should explain how to do this in > the JavaDocs (and also regular docs... --- otherwise, I can already > anticipate user question on all question-asking-channels how to do a > "normal key range query". IMHO, the problem is not that the code itself > it too clumsy, but that it's totally not obvious to uses how to express > it without actually explaining it to them. It basically violated the API > design rule "make it easy to use / simple things should be easy". > > Btw: We could also re-use `RangeQuery` and add am implementation to > `VersionedStateStore` to just accept this query type, with "key range > over latest" semantics. -- The issue is of course, that uses need to > know that the query would return `ValueAndTimestamp` and not plain `V` > (or we add a translation step to unwrap the value, but we would lose the > "validFrom" timestamp -- validTo would be `null`). Because type safety > is a general issue in IQv2 it would not make it worse (in the strict > sense), but I am also not sure if we want to dig an even deeper hole... > > > -Matthias > > > On 10/10/23 11:55 AM, Alieh Saeedi wrote: > > Thanks, Matthias and Bruno, for the feedback on KIP-969. Here is a > summary > > of the updates I made to the KIP: > > > > 1. I liked the idea of renaming methods as Matthias suggested. > > 2. I removed the allversions() method as I did in KIP-968. To > retrieve > > the latest value(s), the user must call just the asOf method with > the MAX > > value (asOf(MAX)). The same applies to KIP-968. Do you think it is > clumsy, > > Matthias? > > 3. I added a method to the *VersionedKeyValueStore *interface, as I > did > > for KIP-968. > > 4. Matthias: I do not get what you mean by your second comment. Isn't > > the KIP already explicit about that? > > > > > I assume, results are returned by timestamp for each key. The KIP > > should be explicit about it. > > > > > > Cheers, > > Alieh > > > > > > > > On Tue, Oct 3, 2023 at 6:07 AM Matthias J. Sax <mj...@apache.org> wrote: > > > >> Thanks for updating the KIP. > >> > >> Not sure if I agree or not with Bruno's idea to split the query types > >> further? In the end, we split them only because there is three different > >> return types: single value, value-iterator, key-value-iterator. > >> > >> What do we gain by splitting out single-ts-range-key? In the end, for > >> range-ts-range-key the proposed class is necessary and is a superset > >> (one can set both timestamps to the same value, for single-ts lookup). > >> > >> The mentioned simplification might apply to "single-ts-range-key" but I > >> don't see a simplification for the proposed (and necessary) query type? > >> > >> On the other hand, I see an advantage of a single-ts-range-key for > >> querying over the "latest version" with a range of keys. For a > >> single-ts-range-key query, this it would be the default (similar to > >> VersionedKeyQuery with not asOf-timestamped defined). > >> > >> In the current version of the KIP, (if we agree that default should > >> actually return "all versions" not "latest" -- this default was > >> suggested by Bruno on KIP-968 and makes sense to me, so we would need to > >> have the same default here to stay consistent), users would need to pass > >> in `from(Long.MAX).to(Long.MAX)` (if I got this right) to query the > >> latest point in time only, what seems to be clumsy? Or we could add a > >> `lastestKeyOnly` option to `MultiVersionedRangeQuery`, but it does seems > >> a little clumsy, too. > >> > >> > >> > >>> The overall order of the returned records is by Key > >> > >> I assume, results are returned by timestamp for each key. The KIP should > >> be explicit about it. > >> > >> > >> > >> To be very explicit, should we rename the methods to specify the key > bound? > >> > >> - withRange -> withKeyRange > >> - withLowerBound -> withLowerKeyBound > >> - withUpperBound -> withUpperKeyBound > >> - withNoBounds -> allKeys (or withNoKeyBounds, but we use > >> `allVersions` and not `noTimeBound` and should align the naming?) > >> > >> > >> > >> -Matthias > >> > >> > >> On 9/6/23 5:25 AM, Bruno Cadonna wrote: > >>> Hi Alieh, > >>> > >>> Thanks for the KIP! > >>> > >>> One high level comment/question: > >>> > >>> I assume you separated single key queries into two classes because > >>> versioned key queries return a single value and multi version key > >>> queries return iterators. Although, range queries always return > >>> iterators, it would make sense to also separate range queries for > >>> versioned state stores into range queries that return one single > version > >>> of the keys within a range and range queries that return multiple > >>> version of the keys within a range, IMO. That would reduce the > >>> meaningless combinations. > >>> WDYT? > >>> > >>> Best, > >>> Bruno > >>> > >>> On 8/16/23 8:01 PM, Alieh Saeedi wrote: > >>>> Hi all, > >>>> > >>>> I splitted KIP-960 > >>>> < > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-960%3A+Support+single-key_single-timestamp+interactive+queries+%28IQv2%29+for+versioned+state+stores > >>> > >>>> into > >>>> three separate KIPs. Therefore, please continue discussions about > range > >>>> interactive queries here. You can see all the addressed reviews on the > >>>> following page. Thanks in advance. > >>>> > >>>> KIP-969: Support range interactive queries (IQv2) for versioned state > >>>> stores > >>>> < > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-969%3A+Support+range+interactive+queries+%28IQv2%29+for+versioned+state+stores > >>> > >>>> > >>>> I look forward to your feedback! > >>>> > >>>> Cheers, > >>>> Alieh > >>>> > >> > > >