Hey Alieh thanks for the KIP, Weighing in on the AsOf vs Until debate I think either is fine from a natural language perspective. Personally AsOf makes more sense to me where until gives me the idea that the query is making a change. It's totally a connotative difference and not that important. I think as of is pretty frequently used in point of time queries.
Also for these methods it makes sense to drop the "get" We don't normally use that in getters * The key that was specified for this query. */ public K getKey(); /** * The starting time point of the query, if specified */ public Optional<Instant> getFromTimestamp(); /** * The ending time point of the query, if specified */ public Optional<Instant> getAsOfTimestamp(); Other than that I didn't have too much to add. Overall I like the direction of the KIP and think the funcatinlyt is all there! best, Walker On Mon, Oct 2, 2023 at 10:46 PM Matthias J. Sax <mj...@apache.org> wrote: > Thanks for the updated KIP. Overall I like it. > > Victoria raises a very good point, and I personally tend to prefer (I > believe so does Victoria, but it's not totally clear from her email) if > a range query would not return any tombstones, ie, only two records in > Victoria's example. Thus, it seems best to include a `validTo` ts-field > to `VersionedRecord` -- otherwise, the retrieved result cannot be > interpreted correctly. > > Not sure what others think about it. > > I would also be open to actually add a `includeDeletes()` (or > `includeTombstones()`) method/flag (disabled by default) to allow users > to get all tombstone: this would only be helpful if there are two > consecutive tombstone though (if I got it right), so not sure if we want > to add it or not -- it seems also possible to add it later if there is > user demand for it, so it might be a premature addition as this point? > > > Nit: > > > the public interface ValueIterator is used > > "is used" -> "is added" (otherwise it sounds like as if `ValueIterator` > exist already) > > > > Should we also add a `.within(fromTs, toTs)` (or maybe some better > name?) to allow specifying both bounds at once? The existing > `RangeQuery` does the same for specifying the key-range, so might be > good to add for time-range too? > > > > -Matthias > > > On 9/6/23 5:01 AM, Bruno Cadonna wrote: > > In my last e-mail I missed to finish a sentence. > > > > "I think from a KIP" > > > > should be > > > > "I think the KIP looks good!" > > > > > > On 9/6/23 1:59 PM, Bruno Cadonna wrote: > >> Hi Alieh, > >> > >> Thanks for the KIP! > >> > >> I think from a KIP > >> > >> 1. > >> I propose to throw an IllegalArgumentException or an > >> IllegalStateException for meaningless combinations. In any case, the > >> KIP should specify what exception is thrown. > >> > >> 2. > >> Why does not specifying a range return the latest version? I would > >> expect that it returns all versions since an empty lower or upper > >> limit is interpreted as no limit. > >> > >> 3. > >> I second Matthias comment about replacing "asOf" with "until" or "to". > >> > >> 4. > >> Do we need "allVersions()"? As I said above I would return all > >> versions if no limits are specified. I think if we get rid of > >> allVersions() there might not be any meaningless combinations anymore. > >> If a user applies twice the same limit like for example > >> MultiVersionedKeyQuery.with(key).from(t1).from(t2) the last one wins. > >> > >> 5. > >> Could you add some more examples with time ranges to the example > section? > >> > >> 6. > >> The KIP misses the test plan section. > >> > >> 7. > >> I propose to rename the class to "MultiVersionKeyQuery" since we are > >> querying multiple versions of the same key. > >> > >> 8. > >> Could you also add withAscendingTimestamps()? IMO it gives users the > >> possibility to make their code more readable instead of only relying > >> on the default. > >> > >> Best, > >> Bruno > >> > >> > >> On 8/17/23 4:13 AM, Matthias J. Sax wrote: > >>> Thanks for splitting this part into a separate KIP! > >>> > >>> For `withKey()` we should be explicit that `null` is not allowed. > >>> > >>> (Looking into existing `KeyQuery` it seems the JavaDocs don't cover > >>> this either -- would you like to do a tiny cleanup PR for this, or > >>> fix on-the-side in one of your PRs?) > >>> > >>> > >>> > >>>> The key query returns all the records that are valid in the time > >>>> range starting from the timestamp {@code fromTimestamp}. > >>> > >>> In the JavaDocs you use the phrase `are valid` -- I think we need to > >>> explain what "valid" means? It might even be worth to add some > >>> examples. It's annoying, but being precise if kinda important. > >>> > >>> With regard to KIP-962, should we allow `null` for time bounds ? The > >>> JavaDocs should also be explicit if `null` is allowed or not and what > >>> the semantics are if allowed. > >>> > >>> > >>> > >>> You are using `asOf()` however, because we are doing time-range > >>> queries, to me using `until()` to describe the upper bound would > >>> sound better (I am not a native speaker though, so maybe I am off?) > >>> > >>> > >>>> The key query returns all the records that have timestamp <= {@code > >>>> asOfTimestamp}. > >>> > >>> This is only correct if not lower-bound is set, right? > >>> > >>> > >>> In your reply to KIP-960 you mentioned: > >>> > >>>> the meaningless combinations are prevented by throwing exceptions. > >>> > >>> We should add corresponding JavaDocs like: > >>> > >>> @throws IllegalArgumentException if {@code fromTimestamp} is > >>> equal or > >>> larger than {@code untilTimestamp} > >>> > >>> Or something similar. > >>> > >>> > >>> With regard to KIP-960: if we need to introduce a `VersionedKeyQuery` > >>> class for single-key-single-ts lookup, would we need to find a new > >>> name for the query class of this KIP, given that the return type is > >>> different? > >>> > >>> > >>> -Matthias > >>> > >>> > >>> > >>> On 8/16/23 10:57 AM, 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 single-key, multi-timestamp interactive queries here. You can > >>>> see all > >>>> the addressed reviews on the following page. Thanks in advance. > >>>> > >>>> KIP-968: Support single-key_multi-timestamp interactive queries > >>>> (IQv2) for > >>>> versioned state stores > >>>> < > https://cwiki.apache.org/confluence/display/KAFKA/KIP-968%3A+Support+single-key_multi-timestamp+interactive+queries+%28IQv2%29+for+versioned+state+stores > > > >>>> > >>>> I look forward to your feedback! > >>>> > >>>> Cheers, > >>>> Alieh > >>>> >