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