You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "zstan (via GitHub)" <gi...@apache.org> on 2023/05/24 10:19:30 UTC
[GitHub] [ignite-3] zstan opened a new pull request, #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
zstan opened a new pull request, #2101:
URL: https://github.com/apache/ignite-3/pull/2101
(no comment)
--
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
[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
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1208968606
##########
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:
why do you use `size` as a trigger?
--
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
[GitHub] [ignite-3] zstan merged pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan merged PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101
--
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
[GitHub] [ignite-3] zstan commented on pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#issuecomment-1568234105
@korlov42 plz check last fixes ?
--
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
[GitHub] [ignite-3] zstan commented on a diff in pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1206901654
##########
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:
It updates in IgniteTableImpl#handleInsertResults
--
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
[GitHub] [ignite-3] zstan commented on a diff in pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1209204686
##########
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:
seems we need somehow discern multiple values from multiple ranges
--
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
[GitHub] [ignite-3] zstan commented on a diff in pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1209003957
##########
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:
done
--
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
[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
Posted by "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
[GitHub] [ignite-3] zstan commented on a diff in pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1205901915
##########
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:
got 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[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
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1208966656
##########
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:
yes, I see this. But still... You have _static_ field that initialised to `false` (default value for AtomicBoolean). You change this field to `true` once the table (not particular table, but random table since the field is static) has processed the very first insert from SQL (but not from KV).
After commit `e9b8fbf` this has even less sense, because you initialised this field to `true`, thus this field literally does nothing.
--
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
[GitHub] [ignite-3] zstan commented on a diff in pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1209203432
##########
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 fear: **multiple ranges**
--
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
[GitHub] [ignite-3] zstan commented on a diff in pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1206897245
##########
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:
as i can see from java doc : **Multiple values or multiple ranges**, thus seems this is not applicable at common.
--
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
[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
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1208982712
##########
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 understand the idea behind summing up applied indexes, but I still don't understand why should we reuse `size` variable (which supposed to be size of the table) for this
--
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
[GitHub] [ignite-3] zstan commented on a diff in pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1205905300
##########
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:
this will work NOT WORSE than before ) correct fix mention in TODO a little bit upper
https://issues.apache.org/jira/browse/IGNITE-19558
--
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
[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
Posted by "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
[GitHub] [ignite-3] zstan commented on a diff in pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1206884156
##########
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:
got 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on a diff in pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1206897245
##########
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:
as i can see from java doc : Multiple values or multiple ranges, thus seems this is not applicable at common.
--
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
[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
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1209939749
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/IgniteTableImpl.java:
##########
@@ -524,35 +526,56 @@ private ColocationGroup partitionedGroup() {
}
private class StatisticsImpl implements Statistic {
- private static final int STATS_CLI_UPDATE_THRESHOLD = 200;
+ private final int updateThreshold = 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) {
- int parts = table.storage().distributionZoneConfiguration().partitions().value();
+ int parts = table.storage().distributionZoneConfiguration().partitions().value();
- long size = 0L;
+ long partitionsRevisionCounter = 0L;
- for (int p = 0; p < parts; ++p) {
- @Nullable MvPartitionStorage part = table.storage().getMvPartition(p);
+ for (int p = 0; p < parts; ++p) {
+ @Nullable MvPartitionStorage part = table.storage().getMvPartition(p);
- if (part == null) {
- continue;
- }
+ if (part == null) {
+ continue;
+ }
+
+ long upd = part.lastAppliedIndex();
+
+ partitionsRevisionCounter += upd;
+ }
+
+ long prev = lastUpd.get();
+
+ if (partitionsRevisionCounter - lastUpd.get() > updateThreshold) {
Review Comment:
```suggestion
if (partitionsRevisionCounter - prev > updateThreshold) {
```
--
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
[GitHub] [ignite-3] zstan commented on a diff in pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1206918462
##########
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 use it like lightweight trigger, of course this update incremented not only on data mutation but also in raft machinery SE people shows me 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[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
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1208956201
##########
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:
what is wrong with multiple values?
--
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
[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
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1208966656
##########
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:
yes, I see this. But still... You have _static_ field that initialised to `false` (default value for AtomicBoolean). You change this field to `true` once the table has processed the very first insert from SQL (but not from KV).
After commit `e9b8fbf` this has even less sense, because you initialised this field to `true`, thus this field literally does nothing.
--
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
[GitHub] [ignite-3] zstan commented on a diff in pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1209141169
##########
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:
got it, fixed
--
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
[GitHub] [ignite-3] zstan commented on a diff in pull request #2101: IGNITE-19488 Sql. Rework statistics, reject hash index usage with enabled search bounds
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2101:
URL: https://github.com/apache/ignite-3/pull/2101#discussion_r1208973712
##########
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:
lastAppliedIndex - means last update revision of raft group who service current partition.
--
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