[GitHub] [ignite] timoninmaxim commented on a diff in pull request #10767: IGNITE-16619 IndexQuery should support limit

["timoninmaxim (via GitHub)" <gi...@apache.org>]

timoninmaxim commented on code in PR #10767:
URL: https://github.com/apache/ignite/pull/10767#discussion_r1219719004


##########
modules/indexing/src/test/java/org/apache/ignite/cache/query/IndexQueryTestSuite.java:
##########
@@ -46,7 +46,8 @@
     ThinClientIndexQueryTest.class,

Review Comment:
   Let's test the functionality with thin client too.



##########
modules/indexing/src/test/java/org/apache/ignite/cache/query/IndexQueryLimitTest.java:
##########
@@ -0,0 +1,350 @@
+package org.apache.ignite.cache.query;
+
+import org.apache.ignite.Ignite;
+import org.apache.ignite.IgniteCache;
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.IgniteDataStreamer;
+import org.apache.ignite.cache.CacheAtomicityMode;
+import org.apache.ignite.cache.CacheMode;
+import org.apache.ignite.cache.query.annotations.QuerySqlField;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.processors.cache.query.QueryCursorEx;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import javax.cache.Cache;
+import java.util.*;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.IntUnaryOperator;
+import java.util.stream.Stream;
+
+import static org.apache.ignite.cache.CacheAtomicityMode.TRANSACTIONAL;
+import static org.apache.ignite.cache.CacheMode.PARTITIONED;
+import static org.apache.ignite.cache.CacheMode.REPLICATED;
+import static org.apache.ignite.cache.query.IndexQueryCriteriaBuilder.*;
+
+/** */
+@RunWith(Parameterized.class)
+public class IndexQueryLimitTest extends GridCommonAbstractTest {
+    /** */
+    private static final String CACHE = "TEST_CACHE";
+
+    /** */
+    private static final String IDX = "PERSON_ID_IDX";
+
+    /** */
+    private static final String DESC_IDX = "PERSON_DESCID_IDX";
+
+    /** */
+    private static final int CNT = 10_000;
+
+    /** */
+    private Ignite crd;
+
+    /** */
+    private IgniteCache<Long, Person> cache;
+
+    /** */
+    @Parameterized.Parameter
+    public int qryParallelism;
+
+    /** */
+    @Parameterized.Parameter(1)
+    public CacheAtomicityMode atomicityMode;
+
+    /** */
+    @Parameterized.Parameter(2)
+    public CacheMode cacheMode;
+
+    /** */
+    @Parameterized.Parameter(3)
+    public String node;
+
+    /** */
+    @Parameterized.Parameter(4)
+    public int backups;
+
+    /** */
+    @Parameterized.Parameter(5)
+    public String idxName;
+
+    /** Number of duplicates of indexed value. */
+    @Parameterized.Parameter(6)
+    public int duplicates;
+
+    /** */
+    @Parameterized.Parameters(name = "qryPar={0} atomicity={1} mode={2} node={3} backups={4} idxName={5} duplicates={6}")

Review Comment:
   Too many params for this simple functionality. Let's make test simple, like tests for other params - IndexQueryLocalTest, IndexQueryKeepBinaryTest.
   
   It's enough to check single cache, single index, single backup, with/without duplicates. 



##########
modules/core/src/main/java/org/apache/ignite/cache/query/IndexQuery.java:
##########
@@ -94,11 +97,34 @@ public IndexQuery(Class<?> valCls, @Nullable String idxName) {
      * @param idxName Index name.
      */
     public IndexQuery(String valType, @Nullable String idxName) {
+        this(valType, idxName, 0);
+    }
+
+    /**
+     * Specify index with cache value class, index name and limit.
+     *
+     * @param valCls Cache value class.
+     * @param idxName Index name.
+     * @param limit Limits response records count. If 0 or less, considered to be no limit.
+     */
+    public IndexQuery(Class<?> valCls, @Nullable String idxName, int limit) {

Review Comment:
   Let's set `limit` with the setter method only. As we discussed, this parameter has limited use cases, like `max`. No need to specify it within constructor. 



##########
modules/core/src/main/java/org/apache/ignite/cache/query/IndexQuery.java:
##########
@@ -152,6 +178,27 @@ public String getIndexName() {
         return idxName;
     }
 
+    /**
+     * 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, considered to be no limit.
+     * @return {@code this} For chaining.
+     */
+    public IndexQuery<K, V> setLimit(int limit) {
+        this.limit = limit;

Review Comment:
   Let's ensure that limit is positive 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.

To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org