[GitHub] [ignite-3] xtern commented on a diff in pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds

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

xtern commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1205722535


##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/IgniteTableImpl.java:
##########
@@ -617,21 +645,27 @@ private static <RowT> CompletableFuture<List<RowT>> handleInsertResults(
             RowHandler<RowT> handler,
             CompletableFuture<List<RowT>>[] futs
     ) {
+        updateStat.set(true);
+
         return CompletableFuture.allOf(futs)
                 .thenApply(response -> {
-                    List<String> conflictRows = new ArrayList<>();
+                    List<String> conflictRows = null;
 
                     for (CompletableFuture<List<RowT>> future : futs) {
                         List<RowT> values = future.join();
 
+                        if (conflictRows == null && values != null && !values.isEmpty()) {
+                            conflictRows = new ArrayList<>(values.size());
+                        }
+
                         if (values != null) {
                             for (RowT row : values) {
                                 conflictRows.add(handler.toString(row));
                             }
                         }
                     }
 
-                    if (!conflictRows.isEmpty()) {
+                    if (conflictRows != null && !conflictRows.isEmpty()) {

Review Comment:
   As I see it, if `conflictRows` is not `null`, it cannot be empty.



##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/IgniteTableImpl.java:
##########
@@ -531,16 +536,17 @@ private ColocationGroup partitionedGroup() {
     }
 
     private class StatisticsImpl implements Statistic {
-        private static final int STATS_CLI_UPDATE_THRESHOLD = 200;
+        private static final int STATS_UPDATE_THRESHOLD = DistributionZoneManager.DEFAULT_PARTITION_COUNT;
 
-        AtomicInteger statReqCnt = new AtomicInteger();
+        private final AtomicLong lastUpd = new AtomicLong();
 
-        private volatile long localRowCnt;
+        private volatile long localRowCnt = 0L;
 
         /** {@inheritDoc} */
         @Override
+        // TODO: need to be refactored https://issues.apache.org/jira/browse/IGNITE-19558
         public Double getRowCount() {
-            if (statReqCnt.getAndIncrement() % STATS_CLI_UPDATE_THRESHOLD == 0) {
+            if (updateStat.get()) {

Review Comment:
   Will this work correctly if node is started with persisted data?



##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/IgniteTableImpl.java:
##########
@@ -617,21 +645,27 @@ private static <RowT> CompletableFuture<List<RowT>> handleInsertResults(
             RowHandler<RowT> handler,
             CompletableFuture<List<RowT>>[] futs
     ) {
+        updateStat.set(true);
+
         return CompletableFuture.allOf(futs)
                 .thenApply(response -> {
-                    List<String> conflictRows = new ArrayList<>();
+                    List<String> conflictRows = null;
 
                     for (CompletableFuture<List<RowT>> future : futs) {
                         List<RowT> values = future.join();
 
+                        if (conflictRows == null && values != null && !values.isEmpty()) {
+                            conflictRows = new ArrayList<>(values.size());
+                        }
+
                         if (values != null) {
                             for (RowT row : values) {
                                 conflictRows.add(handler.toString(row));
                             }
                         }

Review Comment:
   I suggest not repeating the condition `values != null`.
   
   ```suggestion
                           if (values == null || values.isEmpty()) {
                               continue;
                           }
   
                           if (conflictRows == null) {
                               conflictRows = new ArrayList<>(values.size());
                           }
   
                           for (RowT row : values) {
                               conflictRows.add(handler.toString(row));
                           }
   ```



##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/IndexScanNode.java:
##########
@@ -173,7 +173,7 @@ private Publisher<RowT> partitionPublisher(PartitionWithTerm partWithTerm, @Null
             }
         } else {
             assert schemaIndex.type() == Type.HASH;
-            assert cond != null && cond.lower() != null : "Invalid hash index condition.";
+            assert cond != null && (cond.lower() != null || cond.upper() != null) : "Invalid hash index condition.";

Review Comment:
   Why we need such change?
   We don't use upper bound here and seems this will be equal to 
   ```
   assert cond != null  : "Invalid hash index condition.";
   ```



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