[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r341481518
##
File path:
modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/GridCacheFullTextQuerySelfTest.java
##
@@ -120,28 +165,86 @@ public void testTextQueryWithKeepBinary() throws
Exception {
checkTextQuery(false, true);
}
+/**
+ * @throws Exception In case of error.
+ */
+@Test
+public void testTextQueryWithKeepBinaryLimited() throws Exception {
+checkTextQuery(null, QUERY_LIMIT, false, true);
+}
+
/**
* @throws Exception In case of error.
*/
@Test
public void testTextQuery() throws Exception {
-checkTextQuery(false, true);
+checkTextQuery(false, false);
+}
+
+/**
+ * @throws Exception In case of error.
+ */
+@Test
+public void testTextQueryLimited() throws Exception {
+checkTextQuery(null, QUERY_LIMIT, false, false);
+}
+
+/**
+ * @throws Exception In case of error.
+ */
+@Test
+public void testTextQueryLimitedConcurrent() throws Exception {
+final IgniteEx ignite = grid(0);
+
+final String clause = "1*";
+
+// 1. Populate cache with data, calculating expected count in parallel.
+Set exp = populateCache(ignite, false, MAX_ITEM_COUNT,
(IgnitePredicate)x -> String.valueOf(x).startsWith("1"));
+
+ExecutorService executor = Executors.newFixedThreadPool(N_THREADS);
+
+IntStream.range(0, N_THREADS).forEach((i) ->
executor.submit(textQueryTask(ignite, clause, exp)));
Review comment:
It seems errors from submitted tasks do not propagate to main test thread.
Consequently failures in tasks will not fail the test. I can suggest here to
use
`org.apache.ignite.testframework.GridTestUtils#runMultiThreaded(java.lang.Runnable,
int, java.lang.String)` or someting similar to
`java.util.concurrent.ExecutorService#invokeAll(java.util.Collection>)` (and waiting on futures).
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r341479699
##
File path:
modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/GridCacheFullTextQuerySelfTest.java
##
@@ -59,6 +63,31 @@
/** Cache name */
private static final String PERSON_CACHE = "Person";
+/** Limitation to query response size */
+private static final int QUERY_LIMIT = 5;
+
+/** Concurrent threads number */
+private static final int N_THREADS = 20;
+
+/**
+ * Container for expected values and all available entries
+ */
+private static final class TestPair {
+/** */
+public final Set expected;
+
+/** */
+public final List> all = new ArrayList<>();
+
+/** Constructor
Review comment:
Minor. More common formatting is
```
/**
* Constructor
*
* @param exp expected values set.
*/
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery URL: https://github.com/apache/ignite/pull/6917#discussion_r341476695 ## File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryAdapter.java ## @@ -98,6 +98,9 @@ /** */ private final IgniteBiPredicate filter; +/** Limits returned records quantity*/ Review comment: Minor. Missing space before `*/`. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r341479189
##
File path:
modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/GridCacheFullTextQuerySelfTest.java
##
@@ -120,28 +165,86 @@ public void testTextQueryWithKeepBinary() throws
Exception {
checkTextQuery(false, true);
}
+/**
+ * @throws Exception In case of error.
+ */
+@Test
+public void testTextQueryWithKeepBinaryLimited() throws Exception {
+checkTextQuery(null, QUERY_LIMIT, false, true);
+}
+
/**
* @throws Exception In case of error.
*/
@Test
public void testTextQuery() throws Exception {
-checkTextQuery(false, true);
+checkTextQuery(false, false);
+}
+
+/**
+ * @throws Exception In case of error.
+ */
+@Test
+public void testTextQueryLimited() throws Exception {
+checkTextQuery(null, QUERY_LIMIT, false, false);
+}
+
+/**
+ * @throws Exception In case of error.
+ */
+@Test
+public void testTextQueryLimitedConcurrent() throws Exception {
+final IgniteEx ignite = grid(0);
+
+final String clause = "1*";
+
+// 1. Populate cache with data, calculating expected count in parallel.
+Set exp = populateCache(ignite, false, MAX_ITEM_COUNT,
(IgnitePredicate)x -> String.valueOf(x).startsWith("1"));
+
+ExecutorService executor = Executors.newFixedThreadPool(N_THREADS);
+
+IntStream.range(0, N_THREADS).forEach((i) ->
executor.submit(textQueryTask(ignite, clause, exp)));
+
+executor.shutdown();
+executor.awaitTermination(1, TimeUnit.MINUTES);
+
+clearCache(ignite);
+}
+
+/**
+ * Creates Runnable for TextQuery
+ *
+ * @param ignite Ignite insance.
+ * @param clause Query clause.
+ * @param exp Expected results for validation.
+ * @return TextQuery and validation wrapped into Runnable functional
interface.
+ */
+@NotNull private Runnable textQueryTask(IgniteEx ignite, String clause,
Set exp) {
+return () -> {
+try {
+TextQuery qry = new TextQuery<>(Person.class, clause);
+validateQueryResults(ignite, qry, exp, false);
+}
+catch (Exception e) {
+fail(e.getMessage());
+}
+};
}
/**
* @param loc local query flag.
* @param keepBinary keep binary flag.
*/
private void checkTextQuery(boolean loc, boolean keepBinary) throws
Exception {
-checkTextQuery(null, loc, keepBinary);
+checkTextQuery(null, 0, loc, keepBinary);
}
/**
* @param clause Query clause.
* @param loc local query flag.
* @param keepBinary keep binary flag.
*/
-private void checkTextQuery(String clause, boolean loc, boolean
keepBinary) throws Exception {
+private void checkTextQuery(String clause, int limit, boolean loc, boolean
keepBinary) throws Exception {
Review comment:
Missing `@param limit Limit.` in 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r341475748
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##
@@ -327,9 +331,15 @@ protected void onNodeLeft(UUID evtNodeId) {
protected void enqueue(Collection col) {
assert Thread.holdsLock(this);
-queue.add((Collection)col);
-
-cnt.addAndGet(col.size());
+if(limitCnt <= 0 || limitCnt >= col.size()) {
+queue.add((Collection)col);
+cnt.addAndGet(col.size());
+limitCnt -= col.size();
+} else {
+int toAdd = limitCnt;
+queue.add(new ArrayList(col).subList(0, toAdd));
Review comment:
Unfortunately it is a common practice in Ignite. We do not have cheap
performance measurement feedback loop, consequently we are trying to use faster
approaches but possibly sacrificing code simplicity. There were a lot of cases
from a real practice where a single "innocent" code line (not lock and not
system call) caused visible performance drop.
Also I can thing about another option. Apparently a collection coming to
`enqueue` method is always a `List`. We can try to do a refactoring and change
an argument type to `List`. Then `List.sublist` method could be used directly.
Possibly, it deserves a separate ticket. What do you think?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r340565019
##
File path:
modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/GridCacheFullTextQuerySelfTest.java
##
@@ -59,6 +63,26 @@
/** Cache name */
private static final String PERSON_CACHE = "Person";
+/** Limitation to query response size */
+private static final int QUERY_LIMIT = 5;
+
+/** Concurrent threads number */
+private static final int N_THREADS = 20;
+
+/**
+ * Container for expected values and all available entries
+ */
+private static final class TestPair {
+
Review comment:
Please remove this empty line and add javadocs to class members (automated
checks require 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r340566732
##
File path:
modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/GridCacheFullTextQuerySelfTest.java
##
@@ -120,28 +160,87 @@ public void testTextQueryWithKeepBinary() throws
Exception {
checkTextQuery(false, true);
}
+/**
+ * @throws Exception In case of error.
+ */
+@Test
+public void testTextQueryWithKeepBinaryLimited() throws Exception {
+checkTextQuery(null, QUERY_LIMIT, false, true);
+}
+
/**
* @throws Exception In case of error.
*/
@Test
public void testTextQuery() throws Exception {
-checkTextQuery(false, true);
+checkTextQuery(false, false);
+}
+
+/**
+ * @throws Exception In case of error.
+ */
+@Test
+public void testTextQueryLimited() throws Exception {
+checkTextQuery(null, QUERY_LIMIT, false, false);
+}
+
+/**
+ * @throws Exception In case of error.
+ */
+@Test
+public void testTextQueryLimitedConcurrent() throws Exception {
+final IgniteEx ignite = grid(0);
+
+final String clause = "1*";
+
+// 1. Populate cache with data, calculating expected count in parallel.
+Set exp = populateCache(ignite, false, MAX_ITEM_COUNT,
(IgnitePredicate)x -> String.valueOf(x).startsWith("1"));
+
+ExecutorService executor = Executors.newFixedThreadPool(N_THREADS);
+
+IntStream.range(0, N_THREADS).forEach((i) ->
executor.submit(textQueryTask(ignite, clause, exp)));
+
+executor.shutdown();
+executor.awaitTermination(1, TimeUnit.MINUTES);
+
+clearCache(ignite);
+}
+
+/**
+ * Creates Runnable for TextQuery
+ *
+ * @param ignite Ignite insance.
+ * @param clause Query clause.
+ * @param exp Expected results for validation.
+ * @return TextQuery and validation wrapped into Runnable functional
interface.
+ */
+
+@NotNull private Runnable textQueryTask(IgniteEx ignite, String clause,
Set exp) {
+return () -> {
+try {
+TextQuery qry = new TextQuery<>(Person.class,
clause).setLocal(false);
Review comment:
It seems no need to call `setLocal(false)` as it is a default value.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery URL: https://github.com/apache/ignite/pull/6917#discussion_r340557310 ## File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java ## @@ -60,37 +59,65 @@ /** Logger. */ protected static IgniteLogger log; -/** */ +/** Review comment: We permit _single-line_ javadoc comments for fields. Please use them (in all changed places in this PR). 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r340563317
##
File path:
modules/core/src/test/java/org/apache/ignite/internal/processors/cache/GridCacheFullTextQueryMultithreadedSelfTest.java
##
@@ -76,6 +76,7 @@ public void testH2Text() throws Exception {
final int keyCnt = 5000;
final int logFreq = 50;
final String txt = "Value";
+final int limit = 1000;
Review comment:
Not sure that we should change the behavior in old test. What are the
arguments to do 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r340557578
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##
@@ -325,17 +354,34 @@ protected void onNodeLeft(UUID evtNodeId) {
* @param col Collection.
*/
protected void enqueue(Collection col) {
+
Review comment:
Redundant empty line. Can lead to automatic style check failure.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery URL: https://github.com/apache/ignite/pull/6917#discussion_r340562521 ## File path: modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java ## @@ -2821,11 +2821,12 @@ public void remove(GridCacheContext cctx, CacheDataRow row) * @param filters Key and value filters. * @param Key type. * @param Value type. + * @param limit Limits response records count. If 0 or less, the limit considered to be Integer.MAX_VALUE, that is virtually no limit. Review comment: Clarification is needed why it is no more than `Integer.MAX_VALUE`? Cannot we have more with `GridCloseableIterator`? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r340569106
##
File path:
modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/GridCacheFullTextQuerySelfTest.java
##
@@ -212,86 +311,109 @@ private static void clearCache(IgniteEx ignite) {
*
* @throws IgniteCheckedException if failed.
*/
-private static void validateQueryResults(IgniteEx ignite, Query qry,
Set exp,
+private static void validateQueryResults(IgniteEx ignite, TextQuery qry,
Set exp,
boolean keepBinary) throws IgniteCheckedException {
IgniteCache cache = ignite.cache(PERSON_CACHE);
if (keepBinary) {
IgniteCache cache0 = cache.withKeepBinary();
try (QueryCursor> cursor =
cache0.query(qry)) {
-Set exp0 = new HashSet<>(exp);
-
-List> all = new ArrayList<>();
-
-for (Cache.Entry entry :
cursor.getAll()) {
-all.add(entry);
-assertEquals(entry.getKey().toString(),
entry.getValue().field("name"));
+TestPair testPair = processExpectedWithBinary(exp, cursor);
-assertEquals(entry.getKey(),
entry.getValue().field("age"));
-
-exp0.remove(entry.getKey());
-}
-
-checkForMissedKeys(ignite, exp0, all);
+assertResult(ignite, qry, testPair);
}
try (QueryCursor> cursor =
cache0.query(qry)) {
-Set exp0 = new HashSet<>(exp);
-
-List> all = new ArrayList<>();
-for (Cache.Entry entry :
cursor.getAll()) {
-all.add(entry);
+TestPair testPair = processExpectedWithBinary(exp, cursor);
-assertEquals(entry.getKey().toString(),
entry.getValue().field("name"));
-
-assertEquals(entry.getKey(),
entry.getValue().field("age"));
-
-exp0.remove(entry.getKey());
-}
-
-checkForMissedKeys(ignite, exp0, all);
+assertResult(ignite, qry, testPair);
}
}
else {
try (QueryCursor> cursor =
cache.query(qry)) {
-Set exp0 = new HashSet<>(exp);
-List> all = new ArrayList<>();
+TestPair testPair = processExpected(exp, cursor);
-for (Cache.Entry entry : cursor.getAll()) {
-all.add(entry);
+assertResult(ignite, qry, testPair);
-assertEquals(entry.getKey().toString(),
entry.getValue().name);
+}
-assertEquals(entry.getKey(),
Integer.valueOf(entry.getValue().age));
+try (QueryCursor> cursor =
cache.query(qry)) {
-exp0.remove(entry.getKey());
-}
+TestPair testPair = processExpected(exp, cursor);
-checkForMissedKeys(ignite, exp0, all);
+assertResult(ignite, qry, testPair);
}
+}
+}
-try (QueryCursor> cursor =
cache.query(qry)) {
-Set exp0 = new HashSet<>(exp);
+/**
+ * Checks query for missed keys or if limit is set - for limitation
correctness.
+ *
+ * @param ignite Ignite context.
+ * @param qry Initial text query.
+ * @param testPair pair containing expected and all entries.
+ * @throws IgniteCheckedException if key check failed.
+ */
+private static void assertResult(IgniteEx ignite, TextQuery qry,
+TestPair testPair) throws IgniteCheckedException {
+if (qry.getLimit() > 0) {
+assertTrue(testPair.all.size() <= QUERY_LIMIT);
Review comment:
Should not we `checkForMissedKeys` here as well? Checking that we got lesser
than limit seems insufficient. By the way are there really more matching items
than specified limit?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r340566090
##
File path:
modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/GridCacheFullTextQuerySelfTest.java
##
@@ -120,28 +160,87 @@ public void testTextQueryWithKeepBinary() throws
Exception {
checkTextQuery(false, true);
}
+/**
+ * @throws Exception In case of error.
+ */
+@Test
+public void testTextQueryWithKeepBinaryLimited() throws Exception {
+checkTextQuery(null, QUERY_LIMIT, false, true);
+}
+
/**
* @throws Exception In case of error.
*/
@Test
public void testTextQuery() throws Exception {
-checkTextQuery(false, true);
+checkTextQuery(false, false);
+}
+
+/**
+ * @throws Exception In case of error.
+ */
+@Test
+public void testTextQueryLimited() throws Exception {
+checkTextQuery(null, QUERY_LIMIT, false, false);
+}
+
+/**
+ * @throws Exception In case of error.
+ */
+@Test
+public void testTextQueryLimitedConcurrent() throws Exception {
+final IgniteEx ignite = grid(0);
+
+final String clause = "1*";
+
+// 1. Populate cache with data, calculating expected count in parallel.
+Set exp = populateCache(ignite, false, MAX_ITEM_COUNT,
(IgnitePredicate)x -> String.valueOf(x).startsWith("1"));
+
+ExecutorService executor = Executors.newFixedThreadPool(N_THREADS);
+
+IntStream.range(0, N_THREADS).forEach((i) ->
executor.submit(textQueryTask(ignite, clause, exp)));
+
+executor.shutdown();
+executor.awaitTermination(1, TimeUnit.MINUTES);
+
+clearCache(ignite);
+}
+
+/**
+ * Creates Runnable for TextQuery
+ *
+ * @param ignite Ignite insance.
+ * @param clause Query clause.
+ * @param exp Expected results for validation.
+ * @return TextQuery and validation wrapped into Runnable functional
interface.
+ */
+
Review comment:
Extra empty line.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery URL: https://github.com/apache/ignite/pull/6917#discussion_r340564453 ## File path: modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java ## @@ -2821,11 +2821,12 @@ public void remove(GridCacheContext cctx, CacheDataRow row) * @param filters Key and value filters. * @param Key type. * @param Value type. + * @param limit Limits response records count. If 0 or less, the limit considered to be Integer.MAX_VALUE, that is virtually no limit. Review comment: Ah, it seems that we use Lucene in that way. How does Lucene interpret `Integer.MAX_VALUE`? Can it return more than `Integer.MAX_VALUE`? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r340572639
##
File path:
modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/GridCacheFullTextQuerySelfTest.java
##
@@ -212,86 +311,109 @@ private static void clearCache(IgniteEx ignite) {
*
* @throws IgniteCheckedException if failed.
*/
-private static void validateQueryResults(IgniteEx ignite, Query qry,
Set exp,
+private static void validateQueryResults(IgniteEx ignite, TextQuery qry,
Set exp,
boolean keepBinary) throws IgniteCheckedException {
IgniteCache cache = ignite.cache(PERSON_CACHE);
if (keepBinary) {
IgniteCache cache0 = cache.withKeepBinary();
try (QueryCursor> cursor =
cache0.query(qry)) {
-Set exp0 = new HashSet<>(exp);
-
-List> all = new ArrayList<>();
-
-for (Cache.Entry entry :
cursor.getAll()) {
-all.add(entry);
-assertEquals(entry.getKey().toString(),
entry.getValue().field("name"));
+TestPair testPair = processExpectedWithBinary(exp, cursor);
-assertEquals(entry.getKey(),
entry.getValue().field("age"));
-
-exp0.remove(entry.getKey());
-}
-
-checkForMissedKeys(ignite, exp0, all);
+assertResult(ignite, qry, testPair);
}
try (QueryCursor> cursor =
cache0.query(qry)) {
-Set exp0 = new HashSet<>(exp);
-
-List> all = new ArrayList<>();
-for (Cache.Entry entry :
cursor.getAll()) {
-all.add(entry);
+TestPair testPair = processExpectedWithBinary(exp, cursor);
-assertEquals(entry.getKey().toString(),
entry.getValue().field("name"));
-
-assertEquals(entry.getKey(),
entry.getValue().field("age"));
-
-exp0.remove(entry.getKey());
-}
-
-checkForMissedKeys(ignite, exp0, all);
+assertResult(ignite, qry, testPair);
}
}
else {
try (QueryCursor> cursor =
cache.query(qry)) {
-Set exp0 = new HashSet<>(exp);
-List> all = new ArrayList<>();
+TestPair testPair = processExpected(exp, cursor);
-for (Cache.Entry entry : cursor.getAll()) {
-all.add(entry);
+assertResult(ignite, qry, testPair);
-assertEquals(entry.getKey().toString(),
entry.getValue().name);
+}
-assertEquals(entry.getKey(),
Integer.valueOf(entry.getValue().age));
+try (QueryCursor> cursor =
cache.query(qry)) {
-exp0.remove(entry.getKey());
-}
+TestPair testPair = processExpected(exp, cursor);
-checkForMissedKeys(ignite, exp0, all);
+assertResult(ignite, qry, testPair);
}
+}
+}
-try (QueryCursor> cursor =
cache.query(qry)) {
-Set exp0 = new HashSet<>(exp);
+/**
+ * Checks query for missed keys or if limit is set - for limitation
correctness.
+ *
+ * @param ignite Ignite context.
+ * @param qry Initial text query.
+ * @param testPair pair containing expected and all entries.
+ * @throws IgniteCheckedException if key check failed.
+ */
+private static void assertResult(IgniteEx ignite, TextQuery qry,
+TestPair testPair) throws IgniteCheckedException {
+if (qry.getLimit() > 0) {
+assertTrue(testPair.all.size() <= QUERY_LIMIT);
+}
+else {
Review comment:
According to [code
style](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-BracesandIdentation)
braces should not be used for single-line statements after _if_ and _else_.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338491251
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##
@@ -327,9 +331,15 @@ protected void onNodeLeft(UUID evtNodeId) {
protected void enqueue(Collection col) {
assert Thread.holdsLock(this);
-queue.add((Collection)col);
-
-cnt.addAndGet(col.size());
+if(limitCnt <= 0 || limitCnt >= col.size()) {
+queue.add((Collection)col);
+cnt.addAndGet(col.size());
+limitCnt -= col.size();
+} else {
+int toAdd = limitCnt;
+queue.add(new ArrayList(col).subList(0, toAdd));
Review comment:
Default page size is 1024. But it can be configured by user, so it is
outside of our control. If we do not trust _streams_ (there might be reasons
for it) then we can resort to good old procedural style:
```java
Iterator it = ((Collection)col).iterator();
ArrayList sublist = new ArrayList<>(toAdd);
for (int i = 0; i < toAdd; i++)
sublist.add(it.next());
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338491251
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##
@@ -327,9 +331,15 @@ protected void onNodeLeft(UUID evtNodeId) {
protected void enqueue(Collection col) {
assert Thread.holdsLock(this);
-queue.add((Collection)col);
-
-cnt.addAndGet(col.size());
+if(limitCnt <= 0 || limitCnt >= col.size()) {
+queue.add((Collection)col);
+cnt.addAndGet(col.size());
+limitCnt -= col.size();
+} else {
+int toAdd = limitCnt;
+queue.add(new ArrayList(col).subList(0, toAdd));
Review comment:
Default page size is 1024. But it can be configured by user, so it is
outside of our control. If we do not trust _streams_ (there might be reasons
for it) then we can resort to good old procedural style:
```java
Iterator it = ((Collection)col).iterator();
ArrayList sublist = new ArrayList<>();
for (int i = 0; i < toAdd; i++)
sublist.add(it.next());
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338485139
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##
@@ -330,15 +333,18 @@ protected void onNodeLeft(UUID evtNodeId) {
*/
protected void enqueue(Collection col) {
assert Thread.holdsLock(this);
-
-if(limitCnt <= 0 || limitCnt >= col.size()) {
-queue.add((Collection)col);
-cnt.addAndGet(col.size());
-limitCnt -= col.size();
-} else {
-int toAdd = limitCnt;
-queue.add(new ArrayList(col).subList(0, toAdd));
-cnt.addAndGet(toAdd);
+if(!limitReached){
+if(capacity <= 0 || capacity >= col.size()) {
Review comment:
I suppose a bug is hidden here. If we got `capacity == col.size()`, then
`capacity` becomes 0 but `limitReached` flag is still false. Subsequent
`enqueue` calls will add items to the queue unconditionally. It seems can be
fixed here by changing `>=` to `>` (`capacity > col.size()`). But let me
suggest my version for your justification:
```
if (limitDisabled) {
queue.add((Collection)col);
cnt.addAndGet(col.size());
}
else {
if (capacity >= col.size()) {
queue.add((Collection)col);
capacity -= col.size();
cnt.addAndGet(col.size());
}
else if (capacity > 0) {
queue.add(new ArrayList<>((Collection)col).subList(0, capacity));
capacity = 0;
cnt.addAndGet(capacity);
}
}
```
By the way, can we have `col.size() == 0` here?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338470344
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##
@@ -330,15 +333,18 @@ protected void onNodeLeft(UUID evtNodeId) {
*/
protected void enqueue(Collection col) {
assert Thread.holdsLock(this);
-
-if(limitCnt <= 0 || limitCnt >= col.size()) {
-queue.add((Collection)col);
-cnt.addAndGet(col.size());
-limitCnt -= col.size();
-} else {
-int toAdd = limitCnt;
-queue.add(new ArrayList(col).subList(0, toAdd));
-cnt.addAndGet(toAdd);
+if(!limitReached){
+if(capacity <= 0 || capacity >= col.size()) {
+queue.add((Collection)col);
+cnt.addAndGet(col.size());
+capacity -= col.size();
+} else {
Review comment:
According to codey-style `else` should start from new line.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338469873
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##
@@ -330,15 +333,18 @@ protected void onNodeLeft(UUID evtNodeId) {
*/
protected void enqueue(Collection col) {
assert Thread.holdsLock(this);
-
-if(limitCnt <= 0 || limitCnt >= col.size()) {
-queue.add((Collection)col);
-cnt.addAndGet(col.size());
-limitCnt -= col.size();
-} else {
-int toAdd = limitCnt;
-queue.add(new ArrayList(col).subList(0, toAdd));
-cnt.addAndGet(toAdd);
+if(!limitReached){
+if(capacity <= 0 || capacity >= col.size()) {
Review comment:
Space between `if` and `(`.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338469465
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##
@@ -330,15 +333,18 @@ protected void onNodeLeft(UUID evtNodeId) {
*/
protected void enqueue(Collection col) {
assert Thread.holdsLock(this);
-
-if(limitCnt <= 0 || limitCnt >= col.size()) {
-queue.add((Collection)col);
-cnt.addAndGet(col.size());
-limitCnt -= col.size();
-} else {
-int toAdd = limitCnt;
-queue.add(new ArrayList(col).subList(0, toAdd));
-cnt.addAndGet(toAdd);
+if(!limitReached){
Review comment:
Space between `if` and `(`, space between `)` and `{`.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery URL: https://github.com/apache/ignite/pull/6917#discussion_r338486353 ## File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryAdapter.java ## @@ -234,6 +234,7 @@ public GridCacheQueryAdapter( * @param part Partition. * @param clsName Class name. * @param clause Clause. + * @param limit Response limit. Set to -1 for no limits. Review comment: Did not we break consistency? Other javadocs claim `0 or less means no limit`. Could you please elaborate? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338485139
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##
@@ -330,15 +333,18 @@ protected void onNodeLeft(UUID evtNodeId) {
*/
protected void enqueue(Collection col) {
assert Thread.holdsLock(this);
-
-if(limitCnt <= 0 || limitCnt >= col.size()) {
-queue.add((Collection)col);
-cnt.addAndGet(col.size());
-limitCnt -= col.size();
-} else {
-int toAdd = limitCnt;
-queue.add(new ArrayList(col).subList(0, toAdd));
-cnt.addAndGet(toAdd);
+if(!limitReached){
+if(capacity <= 0 || capacity >= col.size()) {
Review comment:
I suppose a bug is hidden here. If we got `capacity == col.size()`, then
`capacity` becomes 0 but `limitReached` flag is still false. Subsequent
`enqueue` calls will add items to the queue unconditionally. It seems can be
fixed here by changing `>=` to `>` (`capacity > col.size()`). But let me
suggest my version for your justification:
```java
if (limitDisabled) {
queue.add((Collection)col);
cnt.addAndGet(col.size());
}
else {
if (capacity >= col.size()) {
queue.add((Collection)col);
capacity -= col.size();
cnt.addAndGet(col.size());
}
else if (capacity > 0) {
queue.add(new ArrayList<>((Collection)col).subList(0, capacity));
capacity = 0;
cnt.addAndGet(capacity);
}
}
```
By the way, can we have `col.size() == 0` here?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338463231
##
File path:
modules/core/src/main/java/org/apache/ignite/cache/query/TextQuery.java
##
@@ -79,6 +82,19 @@ public TextQuery(String type, String txt) {
setText(txt);
}
+/**
+ * Constructs query for the given search string.
+ *
+ * @param type Type.
+ * @param txt Search string.
+ * @param limit Limits response records count. If 0 or less, the limit
considered to be Integer.MAX_VALUE, that is virtually no limit.
+ */
+public TextQuery(String type, String txt, int limit) {
Review comment:
Makes sense. Thanks for explanation.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338014193
##
File path:
modules/core/src/main/java/org/apache/ignite/cache/query/TextQuery.java
##
@@ -79,6 +82,19 @@ public TextQuery(String type, String txt) {
setText(txt);
}
+/**
+ * Constructs query for the given search string.
+ *
+ * @param type Type.
+ * @param txt Search string.
+ * @param limit Limits response records count. If 0 or less, the limit
considered to be Integer.MAX_VALUE, that is virtually no limit.
+ */
+public TextQuery(String type, String txt, int limit) {
Review comment:
I suggest to get rid of constructors with `limit` arguments and keep only
`setLimit` method. We use such approach in this class and for other query types
(e.g. `SqlFieldsQuery`).
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery URL: https://github.com/apache/ignite/pull/6917#discussion_r338019589 ## File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryRequest.java ## @@ -257,6 +260,7 @@ public GridCacheQueryRequest( GridCacheQueryType type, boolean fields, String clause, +int limit, Review comment: Missing `@param` in constructor 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338016454
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryAdapter.java
##
@@ -389,6 +394,18 @@ public int pageSize() {
return this;
}
+public int limit() {
Review comment:
Missing 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338017781
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryManager.java
##
@@ -2748,22 +2749,23 @@ public int size() {
* @param clsName Query class name.
* @param search Search clause.
* @param keepBinary Keep binary flag.
+ * @param limit Limits response records count. If 0 or less, the limit
considered to be Integer.MAX_VALUE, that is virtually no limit.
Review comment:
Wrong order of arguments (`limit` should be before `keepBinary`).
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338052992
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##
@@ -327,9 +331,15 @@ protected void onNodeLeft(UUID evtNodeId) {
protected void enqueue(Collection col) {
assert Thread.holdsLock(this);
-queue.add((Collection)col);
-
-cnt.addAndGet(col.size());
+if(limitCnt <= 0 || limitCnt >= col.size()) {
+queue.add((Collection)col);
+cnt.addAndGet(col.size());
+limitCnt -= col.size();
+} else {
+int toAdd = limitCnt;
+queue.add(new ArrayList(col).subList(0, toAdd));
Review comment:
Construction `new ArrayList(col).subList(0, toAdd)` looks suboptimal as
whole input is copied to new ArrayList. One-liner
`col.stream().limit(n).collect(Collectors.toList())` or manual filling (for
performance reasons) of new ArrayList with `toAdd` elements might be better.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338015321
##
File path:
modules/core/src/main/java/org/apache/ignite/cache/query/TextQuery.java
##
@@ -144,6 +173,26 @@ public String getText() {
return this;
}
+/**
+ * Gets limit to response records count.
+ *
+ * @return Limit value.
+ */
+public int getLimit() {
+return limit;
+}
+
+/**
+ * Sets limit to response records count.
+ *
+ * @param limit If 0 or less, the limit considered to be
Integer.MAX_VALUE, that is virtually no limit.
+ * @return {@code this} For chaining.
+ */
+public TextQuery setLimit(int limit) {
+this.limit = limit;
Review comment:
For code style consistency (check other setters) there should be empty line
between this and a next statement.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338046780
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryFutureAdapter.java
##
@@ -327,9 +331,15 @@ protected void onNodeLeft(UUID evtNodeId) {
protected void enqueue(Collection col) {
assert Thread.holdsLock(this);
-queue.add((Collection)col);
-
-cnt.addAndGet(col.size());
+if(limitCnt <= 0 || limitCnt >= col.size()) {
+queue.add((Collection)col);
+cnt.addAndGet(col.size());
+limitCnt -= col.size();
+} else {
+int toAdd = limitCnt;
Review comment:
As query can return more items than requested limit it worth adding a
concurrent test to increase chances that final behavior will be correct.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338025179
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/IgniteCacheProxyImpl.java
##
@@ -520,13 +520,13 @@ public CountDownLatch getInitLatch(){
}
/**
- * @param filter Filter.
+ * @param query Filter.
Review comment:
Should be at least `query Query`.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery URL: https://github.com/apache/ignite/pull/6917#discussion_r338016343 ## File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryAdapter.java ## @@ -251,6 +254,7 @@ public GridCacheQueryAdapter( @Nullable Integer part, @Nullable String clsName, String clause, +int limit, Review comment: Missing `@param` in javadocs. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] pavlukhin commented on a change in pull request #6917: IGNITE-12189 Implementation correct limit for TextQuery
pavlukhin commented on a change in pull request #6917: IGNITE-12189
Implementation correct limit for TextQuery
URL: https://github.com/apache/ignite/pull/6917#discussion_r338016727
##
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/GridCacheQueryAdapter.java
##
@@ -389,6 +394,18 @@ public int pageSize() {
return this;
}
+public int limit() {
+return limit;
+}
+
+/** {@inheritDoc} */
+@Override
+public CacheQuery limit(int limit) {
Review comment:
`@Overrding public` should be on the same line according to Ignite code
style.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
