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

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

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


##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rule/logical/ExposeIndexRule.java:
##########
@@ -31,6 +31,7 @@
 import org.apache.calcite.util.ImmutableBitSet;
 import org.apache.ignite.internal.sql.engine.rel.logical.IgniteLogicalIndexScan;
 import org.apache.ignite.internal.sql.engine.rel.logical.IgniteLogicalTableScan;
+import org.apache.ignite.internal.sql.engine.schema.IgniteIndex.Type;

Review Comment:
   why do you need this import? 



##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/AbstractIndexScan.java:
##########
@@ -111,33 +112,44 @@ public RelNode accept(RexShuttle shuttle) {
         return super.accept(shuttle);
     }
 
-    /**
-     * Get index name.
-     * TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
-     */
+    /** Return index name. */
     public String indexName() {
         return idxName;
     }
 
+    /** Index type. */
+    public Type indexType() {
+        return type;
+    }
+
     /** {@inheritDoc} */
     @Override
     public RelOptCost computeSelfCost(RelOptPlanner planner, RelMetadataQuery mq) {
         double rows = table.getRowCount();
 
-        double cost;
+        double cost = 0;
+
+        if (type == Type.HASH) {
+            boolean notExact = (searchBounds() == null)
+                    || (searchBounds().stream().anyMatch(bound -> bound.type() != SearchBounds.Type.EXACT));

Review Comment:
   I think, `SearchBounds.Type.MULTI` type is legit as well



##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rel/AbstractIndexScan.java:
##########
@@ -111,33 +112,44 @@ public RelNode accept(RexShuttle shuttle) {
         return super.accept(shuttle);
     }
 
-    /**
-     * Get index name.
-     * TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
-     */
+    /** Return index name. */
     public String indexName() {
         return idxName;
     }
 
+    /** Index type. */
+    public Type indexType() {

Review Comment:
   indexType is not used



##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/IgniteTableImpl.java:
##########
@@ -552,14 +559,36 @@ public Double getRowCount() {
                         continue;
                     }
 
-                    try {
-                        size += part.rowsCount();
-                    } catch (StorageRebalanceException ignore) {
-                        // No-op.
-                    }
+                    long upd = part.lastAppliedIndex();
+
+                    size += upd;

Review Comment:
   I'm not sure what size you are counting here



##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/HashIndexPlannerTest.java:
##########
@@ -88,6 +90,27 @@ public void hashIndexIsNotAppliedForRangeCondition() throws Exception {
         String invalidPlanMsg = "Invalid plan:\n" + RelOptUtil.toString(phys);
 
         assertThat(invalidPlanMsg, scan, notNullValue());
+
+        // Can`t use hash index scan with range.
+        String sqlGT = "SELECT /*+ DISABLE_RULE('LogicalTableScanConverterRule')*/ id FROM test_tbl WHERE val >= 10";
+
+        IgniteTestUtils.assertThrowsWithCause(() -> physicalPlan(sqlGT, schema), CannotPlanException.class,

Review Comment:
   let's split this test in order to have single test case per method



##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/IgniteTableImpl.java:
##########
@@ -111,6 +114,9 @@ public class IgniteTableImpl extends AbstractTable implements IgniteTable, Updat
 
     private final PartitionExtractor partitionExtractor;
 
+    /** Triggers statistic update. */
+    private static AtomicBoolean updateStat = new AtomicBoolean();

Review Comment:
   don't quite understand why do we need this flag. It only prevents from updating stats before first insert, but why?



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