Re: [PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
mjsax merged PR #14477: URL: https://github.com/apache/kafka/pull/14477 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
hanyuzheng7 commented on code in PR #14477:
URL: https://github.com/apache/kafka/pull/14477#discussion_r1364836021
##
streams/src/test/java/org/apache/kafka/streams/integration/IQv2StoreIntegrationTest.java:
##
@@ -1586,12 +1593,15 @@ public void shouldHandleKeyQuery(
public void shouldHandleRangeQuery(
final Optional lower,
final Optional upper,
+final boolean isKeyAscending,
final Function valueExtactor,
-final Set expectedValue) {
+final List expectedValue) {
Review Comment:
It seems that exiting code using expectedValue and actualValue for other
methods, I will update them all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
hanyuzheng7 commented on code in PR #14477:
URL: https://github.com/apache/kafka/pull/14477#discussion_r1364836021
##
streams/src/test/java/org/apache/kafka/streams/integration/IQv2StoreIntegrationTest.java:
##
@@ -1586,12 +1593,15 @@ public void shouldHandleKeyQuery(
public void shouldHandleRangeQuery(
final Optional lower,
final Optional upper,
+final boolean isKeyAscending,
final Function valueExtactor,
-final Set expectedValue) {
+final List expectedValue) {
Review Comment:
It seems that exiting code using expectedValue and actualValue for other
methods, I will update them all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
hanyuzheng7 commented on code in PR #14477:
URL: https://github.com/apache/kafka/pull/14477#discussion_r1364832374
##
streams/src/test/java/org/apache/kafka/streams/integration/IQv2StoreIntegrationTest.java:
##
@@ -1586,12 +1593,15 @@ public void shouldHandleKeyQuery(
public void shouldHandleRangeQuery(
final Optional lower,
final Optional upper,
+final boolean isKeyAscending,
final Function valueExtactor,
-final Set expectedValue) {
+final List expectedValue) {
Review Comment:
ok, I will update it.
##
streams/src/test/java/org/apache/kafka/streams/integration/IQv2StoreIntegrationTest.java:
##
@@ -1604,9 +1614,10 @@ public void shouldHandleRangeQuery(
if (result.getGlobalResult() != null) {
fail("global tables aren't implemented");
} else {
-final Set actualValue = new HashSet<>();
+final List actualValue = new ArrayList<>();
Review Comment:
ok, I will update it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
hanyuzheng7 commented on PR #14477: URL: https://github.com/apache/kafka/pull/14477#issuecomment-1769829820 > LGTM. Two nits to improve variable names. > > As discussed in person, we also should update JavaDocs for `RangeQuery`, and `ReadOnlyKeyValueStore` to make it more explicit what ordering guarantees are provided. > > Furthermore, we also need to update `docs/streams/...` > > We can either add all this to this PR, or you do follow up PR. Just let me know. I believe it would be best to open a new PR dedicated solely to updating the Javadoc. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
mjsax commented on code in PR #14477:
URL: https://github.com/apache/kafka/pull/14477#discussion_r1364552242
##
streams/src/test/java/org/apache/kafka/streams/integration/IQv2StoreIntegrationTest.java:
##
@@ -1586,12 +1593,15 @@ public void shouldHandleKeyQuery(
public void shouldHandleRangeQuery(
final Optional lower,
final Optional upper,
+final boolean isKeyAscending,
final Function valueExtactor,
-final Set expectedValue) {
+final List expectedValue) {
Review Comment:
Nit: `expectedValue[s]`
##
streams/src/test/java/org/apache/kafka/streams/integration/IQv2StoreIntegrationTest.java:
##
@@ -1604,9 +1614,10 @@ public void shouldHandleRangeQuery(
if (result.getGlobalResult() != null) {
fail("global tables aren't implemented");
} else {
-final Set actualValue = new HashSet<>();
+final List actualValue = new ArrayList<>();
Review Comment:
nit: `actualValue[s]`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
hanyuzheng7 commented on code in PR #14477: URL: https://github.com/apache/kafka/pull/14477#discussion_r1349442309 ## streams/src/test/java/org/apache/kafka/streams/integration/IQv2StoreIntegrationTest.java: ## @@ -816,29 +816,65 @@ private void shouldHandleRangeQueries(final Function extractor) shouldHandleRangeQuery( Optional.of(1), Optional.of(3), +true, extractor, -mkSet(1, 2, 3) +Arrays.asList(2, 1, 3) ); shouldHandleRangeQuery( Optional.of(1), Optional.empty(), +true, extractor, -mkSet(1, 2, 3) +Arrays.asList(2, 1, 3) ); shouldHandleRangeQuery( Optional.empty(), Optional.of(1), +true, extractor, -mkSet(0, 1) +Arrays.asList(0, 1) ); shouldHandleRangeQuery( Optional.empty(), Optional.empty(), +true, extractor, -mkSet(0, 1, 2, 3) +Arrays.asList(0, 2, 1, 3) + Review Comment: Yes, that look weird, I have already removed extra lines. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
hanyuzheng7 commented on code in PR #14477: URL: https://github.com/apache/kafka/pull/14477#discussion_r1349442309 ## streams/src/test/java/org/apache/kafka/streams/integration/IQv2StoreIntegrationTest.java: ## @@ -816,29 +816,65 @@ private void shouldHandleRangeQueries(final Function extractor) shouldHandleRangeQuery( Optional.of(1), Optional.of(3), +true, extractor, -mkSet(1, 2, 3) +Arrays.asList(2, 1, 3) ); shouldHandleRangeQuery( Optional.of(1), Optional.empty(), +true, extractor, -mkSet(1, 2, 3) +Arrays.asList(2, 1, 3) ); shouldHandleRangeQuery( Optional.empty(), Optional.of(1), +true, extractor, -mkSet(0, 1) +Arrays.asList(0, 1) ); shouldHandleRangeQuery( Optional.empty(), Optional.empty(), +true, extractor, -mkSet(0, 1, 2, 3) +Arrays.asList(0, 2, 1, 3) + Review Comment: Yes, that look wried, I have already removed extra lines. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
hanyuzheng7 commented on code in PR #14477:
URL: https://github.com/apache/kafka/pull/14477#discussion_r1349442196
##
streams/src/main/java/org/apache/kafka/streams/state/internals/MeteredKeyValueStore.java:
##
@@ -254,17 +254,37 @@ protected QueryResult runRangeQuery(final Query
query,
final QueryResult result;
final RangeQuery typedQuery = (RangeQuery) query;
final RangeQuery rawRangeQuery;
+final boolean isKeyAscending = typedQuery.isKeyAscending();
if (typedQuery.getLowerBound().isPresent() &&
typedQuery.getUpperBound().isPresent()) {
-rawRangeQuery = RangeQuery.withRange(
-keyBytes(typedQuery.getLowerBound().get()),
-keyBytes(typedQuery.getUpperBound().get())
-);
+if (isKeyAscending) {
+rawRangeQuery = RangeQuery.withRange(
+keyBytes(typedQuery.getLowerBound().get()),
+keyBytes(typedQuery.getUpperBound().get())
+);
+} else {
+rawRangeQuery = (RangeQuery) (RangeQuery)
RangeQuery.withRange(
+keyBytes(typedQuery.getLowerBound().get()),
+keyBytes(typedQuery.getUpperBound().get())
+).withDescendingKeys();
Review Comment:
Thank you, I choose lift the final for rawRangeQuery and have already
updated it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
mjsax commented on code in PR #14477: URL: https://github.com/apache/kafka/pull/14477#discussion_r1348228440 ## streams/src/test/java/org/apache/kafka/streams/integration/IQv2StoreIntegrationTest.java: ## @@ -816,29 +816,65 @@ private void shouldHandleRangeQueries(final Function extractor) shouldHandleRangeQuery( Optional.of(1), Optional.of(3), +true, extractor, -mkSet(1, 2, 3) +Arrays.asList(2, 1, 3) ); shouldHandleRangeQuery( Optional.of(1), Optional.empty(), +true, extractor, -mkSet(1, 2, 3) +Arrays.asList(2, 1, 3) ); shouldHandleRangeQuery( Optional.empty(), Optional.of(1), +true, extractor, -mkSet(0, 1) +Arrays.asList(0, 1) ); shouldHandleRangeQuery( Optional.empty(), Optional.empty(), +true, extractor, -mkSet(0, 1, 2, 3) +Arrays.asList(0, 2, 1, 3) + Review Comment: Seems the formatting is off here... the empty line should be after the close `);` ``` Arrays.asList(0, 2, 1, 3) ); shouldHandleRangeQuery( ``` It's already messy in the existing code -- can you clean it up everywhere? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
mjsax commented on code in PR #14477:
URL: https://github.com/apache/kafka/pull/14477#discussion_r1348227721
##
streams/src/main/java/org/apache/kafka/streams/state/internals/MeteredKeyValueStore.java:
##
@@ -254,17 +254,37 @@ protected QueryResult runRangeQuery(final Query
query,
final QueryResult result;
final RangeQuery typedQuery = (RangeQuery) query;
final RangeQuery rawRangeQuery;
+final boolean isKeyAscending = typedQuery.isKeyAscending();
if (typedQuery.getLowerBound().isPresent() &&
typedQuery.getUpperBound().isPresent()) {
-rawRangeQuery = RangeQuery.withRange(
-keyBytes(typedQuery.getLowerBound().get()),
-keyBytes(typedQuery.getUpperBound().get())
-);
+if (isKeyAscending) {
+rawRangeQuery = RangeQuery.withRange(
+keyBytes(typedQuery.getLowerBound().get()),
+keyBytes(typedQuery.getUpperBound().get())
+);
+} else {
+rawRangeQuery = (RangeQuery) (RangeQuery)
RangeQuery.withRange(
+keyBytes(typedQuery.getLowerBound().get()),
+keyBytes(typedQuery.getUpperBound().get())
+).withDescendingKeys();
Review Comment:
However, it seem code is a little bit redundant here. I think it would make
sense to lift the `final` for `rawRangeQuery` and do the following:
```
rawRangeQuery = RangeQuery.withRange(
keyBytes(typedQuery.getLowerBound().get()),
keyBytes(typedQuery.getUpperBound().get())
);
if (!isKeyAscending)
rawRangeQuery = rawRangeQuery.withDescendingKeys();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
mjsax commented on code in PR #14477:
URL: https://github.com/apache/kafka/pull/14477#discussion_r1348226816
##
streams/src/main/java/org/apache/kafka/streams/state/internals/MeteredKeyValueStore.java:
##
@@ -254,17 +254,37 @@ protected QueryResult runRangeQuery(final Query
query,
final QueryResult result;
final RangeQuery typedQuery = (RangeQuery) query;
final RangeQuery rawRangeQuery;
+final boolean isKeyAscending = typedQuery.isKeyAscending();
if (typedQuery.getLowerBound().isPresent() &&
typedQuery.getUpperBound().isPresent()) {
-rawRangeQuery = RangeQuery.withRange(
-keyBytes(typedQuery.getLowerBound().get()),
-keyBytes(typedQuery.getUpperBound().get())
-);
+if (isKeyAscending) {
+rawRangeQuery = RangeQuery.withRange(
+keyBytes(typedQuery.getLowerBound().get()),
+keyBytes(typedQuery.getUpperBound().get())
+);
+} else {
+rawRangeQuery = (RangeQuery) (RangeQuery)
RangeQuery.withRange(
+keyBytes(typedQuery.getLowerBound().get()),
+keyBytes(typedQuery.getUpperBound().get())
+).withDescendingKeys();
Review Comment:
Was looking into this. It's a type erasure issue. You need to specify the
type expliclity for this case when calling the static method so avoid the casts:
```
rawRangeQuery = RangeQuery.withRange(
keyBytes(typedQuery.getLowerBound().get()),
keyBytes(typedQuery.getUpperBound().get())
).withDescendingKeys();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
hanyuzheng7 commented on PR #14477: URL: https://github.com/apache/kafka/pull/14477#issuecomment-1745523946 > Hey this is a public API change right? > > I think you need to update the kip to include the change and get it voted on > > I think this the range query kip https://cwiki.apache.org/confluence/display/KAFKA/KIP-805%3A+Add+range+and+scan+query+over+kv-store+in+IQv2 ok -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
wcarlson5 commented on PR #14477: URL: https://github.com/apache/kafka/pull/14477#issuecomment-1745466006 Hey this is a public API change right? I think you need to update the kip to include the change and get it voted on I think this the range query kip https://cwiki.apache.org/confluence/display/KAFKA/KIP-805%3A+Add+range+and+scan+query+over+kv-store+in+IQv2 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] KAFKA-15527: Add reverseRange and reverseAll query over kv-store in IQv2 [kafka]
hanyuzheng7 opened a new pull request, #14477: URL: https://github.com/apache/kafka/pull/14477 ### Summary Update an implementation of the Query interface, introduced in [KIP-796: Interactive Query v2](https://cwiki.apache.org/confluence/display/KAFKA/KIP-796%3A+Interactive+Query+v2) , to support reverseRange and reverseAll. Use bounded query to achieve reverseRange and use unbounded query to achieve reverseAll. To achieve reverseRange and reverseAll, we add new flag variable revese in RangeQuery class, to help us to do the rangeQuery or reverseQuery. ### Test plan This time, we aim to implement reverseRange and reverseAll. To ensure the accuracy of the results for both, modifications to IQv2StoreIntegrationTest are necessary. Previously, we stored query results in a set, which doesn't retain order. I've since switched to using a list to store the query results. This allows us to discern the differences between rangeQuery and reverseQuery. ### Committer Checklist (excluded from commit message) - [ ] Verify design and implementation - [ ] Verify test coverage and CI build status - [ ] Verify documentation (including upgrade notes) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
