You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/12/20 11:29:48 UTC
[GitHub] [ignite] alex-plekhanov opened a new pull request #9671: IGNITE-16111 Index rebuild handling
alex-plekhanov opened a new pull request #9671:
URL: https://github.com/apache/ignite/pull/9671
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
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] alex-plekhanov closed pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov closed pull request #9671:
URL: https://github.com/apache/ignite/pull/9671
--
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] korlov42 commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r789042877
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementor.java
##########
@@ -298,13 +301,101 @@ public LogicalRelImplementor(
Supplier<Row> upper = upperCond == null ? null : expressionFactory.rowSource(upperCond);
Function<Row, Row> prj = projects == null ? null : expressionFactory.project(projects, rowType);
+ ColocationGroup grp = ctx.group(rel.sourceId());
+
IgniteIndex idx = tbl.getIndex(rel.indexName());
- ColocationGroup group = ctx.group(rel.sourceId());
+ if (idx != null) {
+ Iterable<Row> rowsIter = idx.scan(ctx, grp, filters, lower, upper, prj, requiredColumns);
+
+ return new ScanNode<>(ctx, rowType, rowsIter);
+ }
+ else {
+ // Index was invalidated after planning, workaround through table-scan -> sort -> index spool.
+ RelCollation collation = TraitUtils.collation(rel);
+
+ boolean filterHasCorrelation = condition != null && RexUtils.hasCorrelation(condition);
+ boolean projectHasCorrelation = projects != null && RexUtils.hasCorrelation(projects);
+ boolean spoolNodeRequired = projectHasCorrelation || filterHasCorrelation;
+ boolean projNodeRequired = projects != null && spoolNodeRequired;
+ boolean hasCollation = collation != null && !collation.getFieldCollations().isEmpty();
+
+ Iterable<Row> rowsIter = tbl.scan(
+ ctx,
+ grp,
+ filterHasCorrelation ? null : filters,
+ projNodeRequired ? null : prj,
+ requiredColumns
+ );
- Iterable<Row> rowsIter = idx.scan(ctx, group, filters, lower, upper, prj, requiredColumns);
+ Node<Row> node = new ScanNode<>(ctx, rowType, rowsIter);
- return new ScanNode<>(ctx, rowType, rowsIter);
+ if (hasCollation || spoolNodeRequired) {
Review comment:
Why do we create a SortNode for an empty collation?
--
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] korlov42 commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r803023650
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteIndexScan.java
##########
@@ -87,9 +78,10 @@ public IgniteIndexScan(
@Nullable List<RexNode> proj,
@Nullable RexNode cond,
@Nullable IndexConditions idxCond,
- @Nullable ImmutableBitSet requiredCols
+ @Nullable ImmutableBitSet requiredCols,
+ @Nullable RelCollation collation
Review comment:
I'm not sure if null should be allowed here.
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementor.java
##########
@@ -298,13 +301,106 @@ public LogicalRelImplementor(
Supplier<Row> upper = upperCond == null ? null : expressionFactory.rowSource(upperCond);
Function<Row, Row> prj = projects == null ? null : expressionFactory.project(projects, rowType);
+ ColocationGroup grp = ctx.group(rel.sourceId());
+
IgniteIndex idx = tbl.getIndex(rel.indexName());
- ColocationGroup group = ctx.group(rel.sourceId());
+ if (idx != null && !tbl.isIndexRebuildInProgress()) {
+ Iterable<Row> rowsIter = idx.scan(ctx, grp, filters, lower, upper, prj, requiredColumns);
+
+ return new ScanNode<>(ctx, rowType, rowsIter);
+ }
+ else {
+ // Index was invalidated after planning, workaround through table-scan -> sort -> index spool.
+ // If there are correlates in filter or project, spool node is required to provide ability to rewind input.
+ // Sort node is required if output should be sorted or if spool node required (to provide search by
+ // index conditions).
+ // Additionally, project node is required in case of spool inserted, since spool requires unmodified
+ // original input for filtering by index conditions.
+ boolean filterHasCorrelation = condition != null && RexUtils.hasCorrelation(condition);
+ boolean projectHasCorrelation = projects != null && RexUtils.hasCorrelation(projects);
+ boolean spoolNodeRequired = projectHasCorrelation || filterHasCorrelation;
+ boolean projNodeRequired = projects != null && spoolNodeRequired;
+
+ Iterable<Row> rowsIter = tbl.scan(
+ ctx,
+ grp,
+ filterHasCorrelation ? null : filters,
+ projNodeRequired ? null : prj,
+ requiredColumns
+ );
- Iterable<Row> rowsIter = idx.scan(ctx, group, filters, lower, upper, prj, requiredColumns);
+ Node<Row> node = new ScanNode<>(ctx, rowType, rowsIter);
- return new ScanNode<>(ctx, rowType, rowsIter);
+ RelCollation collation = rel.collation();
+
+ if ((!spoolNodeRequired && projects != null) || requiredColumns != null) {
+ collation = collation.apply(LogicalScanConverterRule.createMapping(
+ spoolNodeRequired ? null : projects,
+ requiredColumns,
+ tbl.getRowType(typeFactory).getFieldCount()
+ ));
+ }
+
+ boolean sortNodeRequired = !collation.getFieldCollations().isEmpty();
+
+ if (sortNodeRequired) {
+ if (!spoolNodeRequired && projects != null)
+ rowType = rel.getRowType();
Review comment:
SortNode should have the same row type as its input
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/IndexRebuildPlannerTest.java
##########
@@ -0,0 +1,367 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.query.calcite.planner;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.UUID;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Function;
+import java.util.function.Predicate;
+import org.apache.calcite.rel.RelCollation;
+import org.apache.calcite.rel.RelCollations;
+import org.apache.calcite.rel.core.CorrelationId;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexShuttle;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.ImmutableIntList;
+import org.apache.calcite.util.mapping.Mappings;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.processors.query.calcite.exec.ArrayRowHandler;
+import org.apache.ignite.internal.processors.query.calcite.exec.ExecutionContext;
+import org.apache.ignite.internal.processors.query.calcite.exec.LogicalRelImplementor;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.IndexSpoolNode;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.Node;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.ProjectNode;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.ScanNode;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.SortNode;
+import org.apache.ignite.internal.processors.query.calcite.metadata.ColocationGroup;
+import org.apache.ignite.internal.processors.query.calcite.rel.IgniteIndexScan;
+import org.apache.ignite.internal.processors.query.calcite.rel.IgniteRel;
+import org.apache.ignite.internal.processors.query.calcite.rel.IgniteTableScan;
+import org.apache.ignite.internal.processors.query.calcite.schema.IgniteSchema;
+import org.apache.ignite.internal.processors.query.calcite.trait.IgniteDistribution;
+import org.apache.ignite.internal.processors.query.calcite.trait.IgniteDistributions;
+import org.apache.ignite.internal.processors.query.calcite.trait.TraitUtils;
+import org.apache.ignite.internal.processors.query.calcite.util.RexUtils;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Planner test for index rebuild.
+ */
+public class IndexRebuildPlannerTest extends AbstractPlannerTest {
+ /** */
+ private IgniteSchema publicSchema;
+
+ /** */
+ private IndexRebuildAwareTable tbl;
+
+ /** {@inheritDoc} */
+ @Override public void setup() {
+ super.setup();
+
+ RelDataTypeFactory.Builder b = new RelDataTypeFactory.Builder(TYPE_FACTORY);
+
+ b.add("_KEY", TYPE_FACTORY.createJavaType(Object.class));
+ b.add("_VAL", TYPE_FACTORY.createJavaType(Object.class));
+ b.add("ID", TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER));
+ b.add("VAL", TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR));
+
+ tbl = new IndexRebuildAwareTable("TBL", b.build(), 100d);
+
+ tbl.addIndex(RelCollations.of(ImmutableIntList.of(2)), "IDX");
+
+ publicSchema = createSchema(tbl);
+ }
+
+ /** */
+ @Test
+ public void testIndexRebuild() throws Exception {
+ String sql = "SELECT * FROM TBL WHERE id = 0";
+
+ assertPlan(sql, publicSchema, isInstanceOf(IgniteIndexScan.class));
+
+ tbl.markIndexRebuildInProgress(true);
+
+ assertPlan(sql, publicSchema, isInstanceOf(IgniteTableScan.class));
+
+ tbl.markIndexRebuildInProgress(false);
+
+ assertPlan(sql, publicSchema, isInstanceOf(IgniteIndexScan.class));
+ }
+
+ /** */
+ @Test
+ public void testConcurrentIndexRebuildStateChange() throws Exception {
+ String sql = "SELECT * FROM TBL WHERE id = 0";
+
+ AtomicBoolean stop = new AtomicBoolean();
+
+ IgniteInternalFuture<?> fut = GridTestUtils.runAsync(() -> {
+ while (!stop.get()) {
+ tbl.markIndexRebuildInProgress(true);
+ tbl.markIndexRebuildInProgress(false);
+ }
+ });
+
+ try {
+ for (int i = 0; i < 1000; i++) {
+ IgniteRel rel = physicalPlan(sql, publicSchema);
+
+ assertTrue(rel instanceof IgniteTableScan || rel instanceof IgniteIndexScan);
+ }
+ }
+ finally {
+ stop.set(true);
+ }
+
+ fut.get();
+ }
+
+ /** */
+ @Test
+ public void testIndexScanRewriter() throws Exception {
Review comment:
I would prefer to move this test to a separate test class (LogicalRelImplementorSelfTest?)
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/IndexRebuildIntegrationTest.java
##########
@@ -0,0 +1,316 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.query.calcite.integration;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.cluster.ClusterState;
+import org.apache.ignite.configuration.DataRegionConfiguration;
+import org.apache.ignite.configuration.DataStorageConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.cache.query.index.IndexProcessor;
+import org.apache.ignite.internal.managers.indexing.IndexesRebuildTask;
+import org.apache.ignite.internal.processors.cache.GridCacheContext;
+import org.apache.ignite.internal.processors.query.calcite.QueryChecker;
+import org.apache.ignite.internal.processors.query.schema.IndexRebuildCancelToken;
+import org.apache.ignite.internal.processors.query.schema.SchemaIndexCacheVisitorClosure;
+import org.apache.ignite.internal.util.future.GridFutureAdapter;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.hamcrest.CoreMatchers;
+import org.junit.Test;
+
+/**
+ * Test index rebuild.
+ */
+public class IndexRebuildIntegrationTest extends AbstractBasicIntegrationTest {
+ /** Index rebuild init latch. */
+ private static CountDownLatch initLatch;
+
+ /** Index rebuild start latch. */
+ private static CountDownLatch startLatch;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ IndexProcessor.idxRebuildCls = BlockingIndexesRebuildTask.class;
+
+ return super.getConfiguration(igniteInstanceName).setDataStorageConfiguration(
+ new DataStorageConfiguration().setDefaultDataRegionConfiguration(
+ new DataRegionConfiguration().setPersistenceEnabled(true)));
+ }
+
+ /** {@inheritDoc} */
+ @Override protected int nodeCount() {
+ return 2;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ initLatch = null;
+ startLatch = null;
+
+ super.beforeTest();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() {
+ // Override super method to skip caches destroy.
+ cleanQueryPlanCache();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTestsStarted() throws Exception {
+ cleanPersistenceDir();
+
+ super.beforeTestsStarted();
+
+ client.cluster().state(ClusterState.ACTIVE);
+
+ executeSql("CREATE TABLE tbl (id INT PRIMARY KEY, val VARCHAR, val2 VARCHAR) WITH CACHE_NAME=\"test\"");
+ executeSql("CREATE INDEX idx_id_val ON tbl (id DESC, val)");
+ executeSql("CREATE INDEX idx_id_val2 ON tbl (id, val2 DESC)");
+ executeSql("CREATE INDEX idx_val ON tbl (val DESC)");
+
+ for (int i = 0; i < 100; i++)
+ executeSql("INSERT INTO tbl VALUES (?, ?, ?)", i, "val" + i, "val" + i);
+
+ executeSql("CREATE TABLE tbl2 (id INT PRIMARY KEY, val VARCHAR)");
+
+ for (int i = 0; i < 100; i++)
+ executeSql("INSERT INTO tbl2 VALUES (?, ?)", i, "val" + i);
+ }
+
+ /** */
+ @Test
+ public void testRebuildOnInitiatorNode() throws Exception {
+ String sql = "SELECT * FROM tbl WHERE id = 0 AND val='val0'";
+
+ QueryChecker validChecker = assertQuery(grid(0), sql)
+ .matches(QueryChecker.containsIndexScan("PUBLIC", "TBL", "IDX_ID_VAL"))
Review comment:
We need to stop checking query plans in integration tests. There is already a test IndexRebuildPlannerTest which verifies a planner behaviour during index rebuilding. For integration tests the only thing that matters is the result 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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r803359215
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementor.java
##########
@@ -298,13 +301,106 @@ public LogicalRelImplementor(
Supplier<Row> upper = upperCond == null ? null : expressionFactory.rowSource(upperCond);
Function<Row, Row> prj = projects == null ? null : expressionFactory.project(projects, rowType);
+ ColocationGroup grp = ctx.group(rel.sourceId());
+
IgniteIndex idx = tbl.getIndex(rel.indexName());
- ColocationGroup group = ctx.group(rel.sourceId());
+ if (idx != null && !tbl.isIndexRebuildInProgress()) {
+ Iterable<Row> rowsIter = idx.scan(ctx, grp, filters, lower, upper, prj, requiredColumns);
+
+ return new ScanNode<>(ctx, rowType, rowsIter);
+ }
+ else {
+ // Index was invalidated after planning, workaround through table-scan -> sort -> index spool.
+ // If there are correlates in filter or project, spool node is required to provide ability to rewind input.
+ // Sort node is required if output should be sorted or if spool node required (to provide search by
+ // index conditions).
+ // Additionally, project node is required in case of spool inserted, since spool requires unmodified
+ // original input for filtering by index conditions.
+ boolean filterHasCorrelation = condition != null && RexUtils.hasCorrelation(condition);
+ boolean projectHasCorrelation = projects != null && RexUtils.hasCorrelation(projects);
+ boolean spoolNodeRequired = projectHasCorrelation || filterHasCorrelation;
+ boolean projNodeRequired = projects != null && spoolNodeRequired;
+
+ Iterable<Row> rowsIter = tbl.scan(
+ ctx,
+ grp,
+ filterHasCorrelation ? null : filters,
+ projNodeRequired ? null : prj,
+ requiredColumns
+ );
- Iterable<Row> rowsIter = idx.scan(ctx, group, filters, lower, upper, prj, requiredColumns);
+ Node<Row> node = new ScanNode<>(ctx, rowType, rowsIter);
- return new ScanNode<>(ctx, rowType, rowsIter);
+ RelCollation collation = rel.collation();
+
+ if ((!spoolNodeRequired && projects != null) || requiredColumns != null) {
+ collation = collation.apply(LogicalScanConverterRule.createMapping(
+ spoolNodeRequired ? null : projects,
+ requiredColumns,
+ tbl.getRowType(typeFactory).getFieldCount()
+ ));
+ }
+
+ boolean sortNodeRequired = !collation.getFieldCollations().isEmpty();
+
+ if (sortNodeRequired) {
+ if (!spoolNodeRequired && projects != null)
+ rowType = rel.getRowType();
Review comment:
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] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r803346399
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/IndexRebuildPlannerTest.java
##########
@@ -0,0 +1,367 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.query.calcite.planner;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.UUID;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Function;
+import java.util.function.Predicate;
+import org.apache.calcite.rel.RelCollation;
+import org.apache.calcite.rel.RelCollations;
+import org.apache.calcite.rel.core.CorrelationId;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexShuttle;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.ImmutableIntList;
+import org.apache.calcite.util.mapping.Mappings;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.processors.query.calcite.exec.ArrayRowHandler;
+import org.apache.ignite.internal.processors.query.calcite.exec.ExecutionContext;
+import org.apache.ignite.internal.processors.query.calcite.exec.LogicalRelImplementor;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.IndexSpoolNode;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.Node;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.ProjectNode;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.ScanNode;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.SortNode;
+import org.apache.ignite.internal.processors.query.calcite.metadata.ColocationGroup;
+import org.apache.ignite.internal.processors.query.calcite.rel.IgniteIndexScan;
+import org.apache.ignite.internal.processors.query.calcite.rel.IgniteRel;
+import org.apache.ignite.internal.processors.query.calcite.rel.IgniteTableScan;
+import org.apache.ignite.internal.processors.query.calcite.schema.IgniteSchema;
+import org.apache.ignite.internal.processors.query.calcite.trait.IgniteDistribution;
+import org.apache.ignite.internal.processors.query.calcite.trait.IgniteDistributions;
+import org.apache.ignite.internal.processors.query.calcite.trait.TraitUtils;
+import org.apache.ignite.internal.processors.query.calcite.util.RexUtils;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Planner test for index rebuild.
+ */
+public class IndexRebuildPlannerTest extends AbstractPlannerTest {
+ /** */
+ private IgniteSchema publicSchema;
+
+ /** */
+ private IndexRebuildAwareTable tbl;
+
+ /** {@inheritDoc} */
+ @Override public void setup() {
+ super.setup();
+
+ RelDataTypeFactory.Builder b = new RelDataTypeFactory.Builder(TYPE_FACTORY);
+
+ b.add("_KEY", TYPE_FACTORY.createJavaType(Object.class));
+ b.add("_VAL", TYPE_FACTORY.createJavaType(Object.class));
+ b.add("ID", TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER));
+ b.add("VAL", TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR));
+
+ tbl = new IndexRebuildAwareTable("TBL", b.build(), 100d);
+
+ tbl.addIndex(RelCollations.of(ImmutableIntList.of(2)), "IDX");
+
+ publicSchema = createSchema(tbl);
+ }
+
+ /** */
+ @Test
+ public void testIndexRebuild() throws Exception {
+ String sql = "SELECT * FROM TBL WHERE id = 0";
+
+ assertPlan(sql, publicSchema, isInstanceOf(IgniteIndexScan.class));
+
+ tbl.markIndexRebuildInProgress(true);
+
+ assertPlan(sql, publicSchema, isInstanceOf(IgniteTableScan.class));
+
+ tbl.markIndexRebuildInProgress(false);
+
+ assertPlan(sql, publicSchema, isInstanceOf(IgniteIndexScan.class));
+ }
+
+ /** */
+ @Test
+ public void testConcurrentIndexRebuildStateChange() throws Exception {
+ String sql = "SELECT * FROM TBL WHERE id = 0";
+
+ AtomicBoolean stop = new AtomicBoolean();
+
+ IgniteInternalFuture<?> fut = GridTestUtils.runAsync(() -> {
+ while (!stop.get()) {
+ tbl.markIndexRebuildInProgress(true);
+ tbl.markIndexRebuildInProgress(false);
+ }
+ });
+
+ try {
+ for (int i = 0; i < 1000; i++) {
+ IgniteRel rel = physicalPlan(sql, publicSchema);
+
+ assertTrue(rel instanceof IgniteTableScan || rel instanceof IgniteIndexScan);
+ }
+ }
+ finally {
+ stop.set(true);
+ }
+
+ fut.get();
+ }
+
+ /** */
+ @Test
+ public void testIndexScanRewriter() throws Exception {
Review comment:
It still will be a subclass of AbstractPlannerTest since it requires planning methods. There will be code duplication with the current test (we need IndexRebuildAwareTable and table initialization). Is it ok?
--
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] korlov42 commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r793338127
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteIndexScan.java
##########
@@ -39,13 +41,18 @@
/** */
private final long sourceId;
+ /** */
+ private final RelCollation idxCollation;
+
/**
* Constructor used for deserialization.
*
* @param input Serialized representation.
*/
public IgniteIndexScan(RelInput input) {
- super(changeTraits(input, IgniteConvention.INSTANCE));
+ super(changeTraits(input, IgniteConvention.INSTANCE, input.getCollation()));
Review comment:
probably, it would be better to introduce a separate field, rather than modify traits. For example, the Sort relation does so
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rule/logical/ExposeIndexRule.java
##########
@@ -68,6 +68,9 @@ private static boolean preMatch(IgniteLogicalTableScan scan) {
RexNode condition = scan.condition();
ImmutableBitSet requiredCols = scan.requiredColumns();
+ if (igniteTable.isIndexRebuildInProgress())
Review comment:
does it make sense to check it in preMatch?
--
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] korlov42 commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r803388673
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/IndexRebuildIntegrationTest.java
##########
@@ -0,0 +1,316 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.query.calcite.integration;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.cluster.ClusterState;
+import org.apache.ignite.configuration.DataRegionConfiguration;
+import org.apache.ignite.configuration.DataStorageConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.cache.query.index.IndexProcessor;
+import org.apache.ignite.internal.managers.indexing.IndexesRebuildTask;
+import org.apache.ignite.internal.processors.cache.GridCacheContext;
+import org.apache.ignite.internal.processors.query.calcite.QueryChecker;
+import org.apache.ignite.internal.processors.query.schema.IndexRebuildCancelToken;
+import org.apache.ignite.internal.processors.query.schema.SchemaIndexCacheVisitorClosure;
+import org.apache.ignite.internal.util.future.GridFutureAdapter;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.hamcrest.CoreMatchers;
+import org.junit.Test;
+
+/**
+ * Test index rebuild.
+ */
+public class IndexRebuildIntegrationTest extends AbstractBasicIntegrationTest {
+ /** Index rebuild init latch. */
+ private static CountDownLatch initLatch;
+
+ /** Index rebuild start latch. */
+ private static CountDownLatch startLatch;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ IndexProcessor.idxRebuildCls = BlockingIndexesRebuildTask.class;
+
+ return super.getConfiguration(igniteInstanceName).setDataStorageConfiguration(
+ new DataStorageConfiguration().setDefaultDataRegionConfiguration(
+ new DataRegionConfiguration().setPersistenceEnabled(true)));
+ }
+
+ /** {@inheritDoc} */
+ @Override protected int nodeCount() {
+ return 2;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ initLatch = null;
+ startLatch = null;
+
+ super.beforeTest();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() {
+ // Override super method to skip caches destroy.
+ cleanQueryPlanCache();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTestsStarted() throws Exception {
+ cleanPersistenceDir();
+
+ super.beforeTestsStarted();
+
+ client.cluster().state(ClusterState.ACTIVE);
+
+ executeSql("CREATE TABLE tbl (id INT PRIMARY KEY, val VARCHAR, val2 VARCHAR) WITH CACHE_NAME=\"test\"");
+ executeSql("CREATE INDEX idx_id_val ON tbl (id DESC, val)");
+ executeSql("CREATE INDEX idx_id_val2 ON tbl (id, val2 DESC)");
+ executeSql("CREATE INDEX idx_val ON tbl (val DESC)");
+
+ for (int i = 0; i < 100; i++)
+ executeSql("INSERT INTO tbl VALUES (?, ?, ?)", i, "val" + i, "val" + i);
+
+ executeSql("CREATE TABLE tbl2 (id INT PRIMARY KEY, val VARCHAR)");
+
+ for (int i = 0; i < 100; i++)
+ executeSql("INSERT INTO tbl2 VALUES (?, ?)", i, "val" + i);
+ }
+
+ /** */
+ @Test
+ public void testRebuildOnInitiatorNode() throws Exception {
+ String sql = "SELECT * FROM tbl WHERE id = 0 AND val='val0'";
+
+ QueryChecker validChecker = assertQuery(grid(0), sql)
+ .matches(QueryChecker.containsIndexScan("PUBLIC", "TBL", "IDX_ID_VAL"))
Review comment:
This functionality is covered by the planner test. Besides, even with such a validation you still can't be sure you are verifying the right scenario, because for explain call the query will be optimized the second time, hence the plan could be different.
--
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] alex-plekhanov closed pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov closed pull request #9671:
URL: https://github.com/apache/ignite/pull/9671
--
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] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r803348337
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/IndexRebuildIntegrationTest.java
##########
@@ -0,0 +1,316 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.query.calcite.integration;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.cluster.ClusterState;
+import org.apache.ignite.configuration.DataRegionConfiguration;
+import org.apache.ignite.configuration.DataStorageConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.cache.query.index.IndexProcessor;
+import org.apache.ignite.internal.managers.indexing.IndexesRebuildTask;
+import org.apache.ignite.internal.processors.cache.GridCacheContext;
+import org.apache.ignite.internal.processors.query.calcite.QueryChecker;
+import org.apache.ignite.internal.processors.query.schema.IndexRebuildCancelToken;
+import org.apache.ignite.internal.processors.query.schema.SchemaIndexCacheVisitorClosure;
+import org.apache.ignite.internal.util.future.GridFutureAdapter;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.hamcrest.CoreMatchers;
+import org.junit.Test;
+
+/**
+ * Test index rebuild.
+ */
+public class IndexRebuildIntegrationTest extends AbstractBasicIntegrationTest {
+ /** Index rebuild init latch. */
+ private static CountDownLatch initLatch;
+
+ /** Index rebuild start latch. */
+ private static CountDownLatch startLatch;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ IndexProcessor.idxRebuildCls = BlockingIndexesRebuildTask.class;
+
+ return super.getConfiguration(igniteInstanceName).setDataStorageConfiguration(
+ new DataStorageConfiguration().setDefaultDataRegionConfiguration(
+ new DataRegionConfiguration().setPersistenceEnabled(true)));
+ }
+
+ /** {@inheritDoc} */
+ @Override protected int nodeCount() {
+ return 2;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ initLatch = null;
+ startLatch = null;
+
+ super.beforeTest();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() {
+ // Override super method to skip caches destroy.
+ cleanQueryPlanCache();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTestsStarted() throws Exception {
+ cleanPersistenceDir();
+
+ super.beforeTestsStarted();
+
+ client.cluster().state(ClusterState.ACTIVE);
+
+ executeSql("CREATE TABLE tbl (id INT PRIMARY KEY, val VARCHAR, val2 VARCHAR) WITH CACHE_NAME=\"test\"");
+ executeSql("CREATE INDEX idx_id_val ON tbl (id DESC, val)");
+ executeSql("CREATE INDEX idx_id_val2 ON tbl (id, val2 DESC)");
+ executeSql("CREATE INDEX idx_val ON tbl (val DESC)");
+
+ for (int i = 0; i < 100; i++)
+ executeSql("INSERT INTO tbl VALUES (?, ?, ?)", i, "val" + i, "val" + i);
+
+ executeSql("CREATE TABLE tbl2 (id INT PRIMARY KEY, val VARCHAR)");
+
+ for (int i = 0; i < 100; i++)
+ executeSql("INSERT INTO tbl2 VALUES (?, ?)", i, "val" + i);
+ }
+
+ /** */
+ @Test
+ public void testRebuildOnInitiatorNode() throws Exception {
+ String sql = "SELECT * FROM tbl WHERE id = 0 AND val='val0'";
+
+ QueryChecker validChecker = assertQuery(grid(0), sql)
+ .matches(QueryChecker.containsIndexScan("PUBLIC", "TBL", "IDX_ID_VAL"))
Review comment:
I disagree here. If the plan will be changed for some of these queries and there will be no index scan rewrite anymore, nobody will notice it, functionality became uncovered by tests and easily can be broken.
--
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] alex-plekhanov commented on pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#issuecomment-1039928392
Merged to sql-calcite branch.
--
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] korlov42 commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r789049504
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementor.java
##########
@@ -298,13 +301,101 @@ public LogicalRelImplementor(
Supplier<Row> upper = upperCond == null ? null : expressionFactory.rowSource(upperCond);
Function<Row, Row> prj = projects == null ? null : expressionFactory.project(projects, rowType);
+ ColocationGroup grp = ctx.group(rel.sourceId());
+
IgniteIndex idx = tbl.getIndex(rel.indexName());
- ColocationGroup group = ctx.group(rel.sourceId());
+ if (idx != null) {
+ Iterable<Row> rowsIter = idx.scan(ctx, grp, filters, lower, upper, prj, requiredColumns);
+
+ return new ScanNode<>(ctx, rowType, rowsIter);
+ }
+ else {
+ // Index was invalidated after planning, workaround through table-scan -> sort -> index spool.
+ RelCollation collation = TraitUtils.collation(rel);
+
+ boolean filterHasCorrelation = condition != null && RexUtils.hasCorrelation(condition);
+ boolean projectHasCorrelation = projects != null && RexUtils.hasCorrelation(projects);
+ boolean spoolNodeRequired = projectHasCorrelation || filterHasCorrelation;
+ boolean projNodeRequired = projects != null && spoolNodeRequired;
+ boolean hasCollation = collation != null && !collation.getFieldCollations().isEmpty();
+
+ Iterable<Row> rowsIter = tbl.scan(
+ ctx,
+ grp,
+ filterHasCorrelation ? null : filters,
+ projNodeRequired ? null : prj,
+ requiredColumns
+ );
- Iterable<Row> rowsIter = idx.scan(ctx, group, filters, lower, upper, prj, requiredColumns);
+ Node<Row> node = new ScanNode<>(ctx, rowType, rowsIter);
- return new ScanNode<>(ctx, rowType, rowsIter);
+ if (hasCollation || spoolNodeRequired) {
Review comment:
I think I got it... Let's add a comment here describing possible outcomes of this rewriting and the reasons of such outcomes
--
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] alex-plekhanov commented on pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#issuecomment-1039928392
Merged to sql-calcite branch.
--
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] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r804437430
##########
File path: modules/calcite/src/test/java/org/apache/ignite/testsuites/ExecutionTestSuite.java
##########
@@ -53,6 +54,7 @@
IntersectExecutionTest.class,
RuntimeSortedIndexTest.class,
LimitExecutionTest.class,
+ LogicalRelImplementorTest.class,
Review comment:
Fixed
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteIndexScan.java
##########
@@ -111,11 +104,13 @@ private IgniteIndexScan(
@Nullable List<RexNode> proj,
@Nullable RexNode cond,
@Nullable IndexConditions idxCond,
- @Nullable ImmutableBitSet requiredCols
+ @Nullable ImmutableBitSet requiredCols,
+ @Nullable RelCollation collation
Review comment:
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] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r803359142
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteIndexScan.java
##########
@@ -87,9 +78,10 @@ public IgniteIndexScan(
@Nullable List<RexNode> proj,
@Nullable RexNode cond,
@Nullable IndexConditions idxCond,
- @Nullable ImmutableBitSet requiredCols
+ @Nullable ImmutableBitSet requiredCols,
+ @Nullable RelCollation collation
Review comment:
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] korlov42 commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r788851578
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rule/logical/ExposeIndexRule.java
##########
@@ -71,7 +71,10 @@ private static boolean preMatch(IgniteLogicalTableScan scan) {
.map(idx -> idx.toRel(cluster, optTable, proj, condition, requiredCols))
.collect(Collectors.toList());
- assert !indexes.isEmpty();
+ indexes.removeIf(Objects::isNull);
Review comment:
perhaps, it would be a bit better to filter out null with a stream API `.filter(Objects::nonNull)` before collecting them to a `indexes` collection
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteIndexScan.java
##########
@@ -126,7 +121,9 @@ private IgniteIndexScan(
/** {@inheritDoc} */
@Override protected RelWriter explainTerms0(RelWriter pw) {
return super.explainTerms0(pw)
- .itemIf("sourceId", sourceId, sourceId != -1);
+ .itemIf("sourceId", sourceId, sourceId != -1)
+ .item("collation", collation())
+ .item("idxCollation", idxCollation);
Review comment:
why do we need both `collation` and `idxCollation`?
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteIndexScan.java
##########
@@ -39,13 +41,18 @@
/** */
private final long sourceId;
+ /** */
+ private final RelCollation idxCollation;
+
/**
* Constructor used for deserialization.
*
* @param input Serialized representation.
*/
public IgniteIndexScan(RelInput input) {
- super(changeTraits(input, IgniteConvention.INSTANCE));
+ super(changeTraits(input, IgniteConvention.INSTANCE, input.getCollation()));
Review comment:
I don't sure it's a good idea to restore traits after deserialization. We either should do this for every node or don't do it at all.
Well, there is a few nodes (e.g. Exchange) which have an assertion inside their constructor, so `changeTraits` is used to overcome this problem. But for other cases I would prefer to avoid using it
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteIndexScan.java
##########
@@ -39,13 +41,18 @@
/** */
private final long sourceId;
+ /** */
+ private final RelCollation idxCollation;
Review comment:
let's rename this to just `collation`
--
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] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r792359518
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteIndexScan.java
##########
@@ -126,7 +121,9 @@ private IgniteIndexScan(
/** {@inheritDoc} */
@Override protected RelWriter explainTerms0(RelWriter pw) {
return super.explainTerms0(pw)
- .itemIf("sourceId", sourceId, sourceId != -1);
+ .itemIf("sourceId", sourceId, sourceId != -1)
+ .item("collation", collation())
+ .item("idxCollation", idxCollation);
Review comment:
`idxCollation` for index bounds, since `collation` an be trimmed.
I've removed `idxCollation` now, and add collation restoration by index bound.
--
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] korlov42 commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r804416578
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/IndexRebuildIntegrationTest.java
##########
@@ -0,0 +1,316 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.query.calcite.integration;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.cluster.ClusterState;
+import org.apache.ignite.configuration.DataRegionConfiguration;
+import org.apache.ignite.configuration.DataStorageConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.cache.query.index.IndexProcessor;
+import org.apache.ignite.internal.managers.indexing.IndexesRebuildTask;
+import org.apache.ignite.internal.processors.cache.GridCacheContext;
+import org.apache.ignite.internal.processors.query.calcite.QueryChecker;
+import org.apache.ignite.internal.processors.query.schema.IndexRebuildCancelToken;
+import org.apache.ignite.internal.processors.query.schema.SchemaIndexCacheVisitorClosure;
+import org.apache.ignite.internal.util.future.GridFutureAdapter;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.hamcrest.CoreMatchers;
+import org.junit.Test;
+
+/**
+ * Test index rebuild.
+ */
+public class IndexRebuildIntegrationTest extends AbstractBasicIntegrationTest {
+ /** Index rebuild init latch. */
+ private static CountDownLatch initLatch;
+
+ /** Index rebuild start latch. */
+ private static CountDownLatch startLatch;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ IndexProcessor.idxRebuildCls = BlockingIndexesRebuildTask.class;
+
+ return super.getConfiguration(igniteInstanceName).setDataStorageConfiguration(
+ new DataStorageConfiguration().setDefaultDataRegionConfiguration(
+ new DataRegionConfiguration().setPersistenceEnabled(true)));
+ }
+
+ /** {@inheritDoc} */
+ @Override protected int nodeCount() {
+ return 2;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ initLatch = null;
+ startLatch = null;
+
+ super.beforeTest();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() {
+ // Override super method to skip caches destroy.
+ cleanQueryPlanCache();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTestsStarted() throws Exception {
+ cleanPersistenceDir();
+
+ super.beforeTestsStarted();
+
+ client.cluster().state(ClusterState.ACTIVE);
+
+ executeSql("CREATE TABLE tbl (id INT PRIMARY KEY, val VARCHAR, val2 VARCHAR) WITH CACHE_NAME=\"test\"");
+ executeSql("CREATE INDEX idx_id_val ON tbl (id DESC, val)");
+ executeSql("CREATE INDEX idx_id_val2 ON tbl (id, val2 DESC)");
+ executeSql("CREATE INDEX idx_val ON tbl (val DESC)");
+
+ for (int i = 0; i < 100; i++)
+ executeSql("INSERT INTO tbl VALUES (?, ?, ?)", i, "val" + i, "val" + i);
+
+ executeSql("CREATE TABLE tbl2 (id INT PRIMARY KEY, val VARCHAR)");
+
+ for (int i = 0; i < 100; i++)
+ executeSql("INSERT INTO tbl2 VALUES (?, ?)", i, "val" + i);
+ }
+
+ /** */
+ @Test
+ public void testRebuildOnInitiatorNode() throws Exception {
+ String sql = "SELECT * FROM tbl WHERE id = 0 AND val='val0'";
+
+ QueryChecker validChecker = assertQuery(grid(0), sql)
+ .matches(QueryChecker.containsIndexScan("PUBLIC", "TBL", "IDX_ID_VAL"))
Review comment:
My point is you don't need to duplicate tests from planner test in integration test suite. And you don't need to duplicate test from LogicalRelImplementorTest too. Both planner part and rewriter part are covered.
Imo, for the integration part, we need to check if the cluster returns a valid result in the event that an index rebuild is triggered.
--
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] korlov42 commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r803383623
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/IndexRebuildPlannerTest.java
##########
@@ -0,0 +1,367 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.query.calcite.planner;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.UUID;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Function;
+import java.util.function.Predicate;
+import org.apache.calcite.rel.RelCollation;
+import org.apache.calcite.rel.RelCollations;
+import org.apache.calcite.rel.core.CorrelationId;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexShuttle;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.ImmutableIntList;
+import org.apache.calcite.util.mapping.Mappings;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.processors.query.calcite.exec.ArrayRowHandler;
+import org.apache.ignite.internal.processors.query.calcite.exec.ExecutionContext;
+import org.apache.ignite.internal.processors.query.calcite.exec.LogicalRelImplementor;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.IndexSpoolNode;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.Node;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.ProjectNode;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.ScanNode;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.SortNode;
+import org.apache.ignite.internal.processors.query.calcite.metadata.ColocationGroup;
+import org.apache.ignite.internal.processors.query.calcite.rel.IgniteIndexScan;
+import org.apache.ignite.internal.processors.query.calcite.rel.IgniteRel;
+import org.apache.ignite.internal.processors.query.calcite.rel.IgniteTableScan;
+import org.apache.ignite.internal.processors.query.calcite.schema.IgniteSchema;
+import org.apache.ignite.internal.processors.query.calcite.trait.IgniteDistribution;
+import org.apache.ignite.internal.processors.query.calcite.trait.IgniteDistributions;
+import org.apache.ignite.internal.processors.query.calcite.trait.TraitUtils;
+import org.apache.ignite.internal.processors.query.calcite.util.RexUtils;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Planner test for index rebuild.
+ */
+public class IndexRebuildPlannerTest extends AbstractPlannerTest {
+ /** */
+ private IgniteSchema publicSchema;
+
+ /** */
+ private IndexRebuildAwareTable tbl;
+
+ /** {@inheritDoc} */
+ @Override public void setup() {
+ super.setup();
+
+ RelDataTypeFactory.Builder b = new RelDataTypeFactory.Builder(TYPE_FACTORY);
+
+ b.add("_KEY", TYPE_FACTORY.createJavaType(Object.class));
+ b.add("_VAL", TYPE_FACTORY.createJavaType(Object.class));
+ b.add("ID", TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER));
+ b.add("VAL", TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR));
+
+ tbl = new IndexRebuildAwareTable("TBL", b.build(), 100d);
+
+ tbl.addIndex(RelCollations.of(ImmutableIntList.of(2)), "IDX");
+
+ publicSchema = createSchema(tbl);
+ }
+
+ /** */
+ @Test
+ public void testIndexRebuild() throws Exception {
+ String sql = "SELECT * FROM TBL WHERE id = 0";
+
+ assertPlan(sql, publicSchema, isInstanceOf(IgniteIndexScan.class));
+
+ tbl.markIndexRebuildInProgress(true);
+
+ assertPlan(sql, publicSchema, isInstanceOf(IgniteTableScan.class));
+
+ tbl.markIndexRebuildInProgress(false);
+
+ assertPlan(sql, publicSchema, isInstanceOf(IgniteIndexScan.class));
+ }
+
+ /** */
+ @Test
+ public void testConcurrentIndexRebuildStateChange() throws Exception {
+ String sql = "SELECT * FROM TBL WHERE id = 0";
+
+ AtomicBoolean stop = new AtomicBoolean();
+
+ IgniteInternalFuture<?> fut = GridTestUtils.runAsync(() -> {
+ while (!stop.get()) {
+ tbl.markIndexRebuildInProgress(true);
+ tbl.markIndexRebuildInProgress(false);
+ }
+ });
+
+ try {
+ for (int i = 0; i < 1000; i++) {
+ IgniteRel rel = physicalPlan(sql, publicSchema);
+
+ assertTrue(rel instanceof IgniteTableScan || rel instanceof IgniteIndexScan);
+ }
+ }
+ finally {
+ stop.set(true);
+ }
+
+ fut.get();
+ }
+
+ /** */
+ @Test
+ public void testIndexScanRewriter() throws Exception {
Review comment:
If you need a template rel, you could use its serialized form and just deserialize it whenever you need it. As for code duplication, the table class could be moved to a separate class.
--
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] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r803535369
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/IndexRebuildPlannerTest.java
##########
@@ -0,0 +1,367 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.query.calcite.planner;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.UUID;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Function;
+import java.util.function.Predicate;
+import org.apache.calcite.rel.RelCollation;
+import org.apache.calcite.rel.RelCollations;
+import org.apache.calcite.rel.core.CorrelationId;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexShuttle;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.calcite.util.ImmutableIntList;
+import org.apache.calcite.util.mapping.Mappings;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.processors.query.calcite.exec.ArrayRowHandler;
+import org.apache.ignite.internal.processors.query.calcite.exec.ExecutionContext;
+import org.apache.ignite.internal.processors.query.calcite.exec.LogicalRelImplementor;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.IndexSpoolNode;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.Node;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.ProjectNode;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.ScanNode;
+import org.apache.ignite.internal.processors.query.calcite.exec.rel.SortNode;
+import org.apache.ignite.internal.processors.query.calcite.metadata.ColocationGroup;
+import org.apache.ignite.internal.processors.query.calcite.rel.IgniteIndexScan;
+import org.apache.ignite.internal.processors.query.calcite.rel.IgniteRel;
+import org.apache.ignite.internal.processors.query.calcite.rel.IgniteTableScan;
+import org.apache.ignite.internal.processors.query.calcite.schema.IgniteSchema;
+import org.apache.ignite.internal.processors.query.calcite.trait.IgniteDistribution;
+import org.apache.ignite.internal.processors.query.calcite.trait.IgniteDistributions;
+import org.apache.ignite.internal.processors.query.calcite.trait.TraitUtils;
+import org.apache.ignite.internal.processors.query.calcite.util.RexUtils;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.junit.Test;
+
+/**
+ * Planner test for index rebuild.
+ */
+public class IndexRebuildPlannerTest extends AbstractPlannerTest {
+ /** */
+ private IgniteSchema publicSchema;
+
+ /** */
+ private IndexRebuildAwareTable tbl;
+
+ /** {@inheritDoc} */
+ @Override public void setup() {
+ super.setup();
+
+ RelDataTypeFactory.Builder b = new RelDataTypeFactory.Builder(TYPE_FACTORY);
+
+ b.add("_KEY", TYPE_FACTORY.createJavaType(Object.class));
+ b.add("_VAL", TYPE_FACTORY.createJavaType(Object.class));
+ b.add("ID", TYPE_FACTORY.createSqlType(SqlTypeName.INTEGER));
+ b.add("VAL", TYPE_FACTORY.createSqlType(SqlTypeName.VARCHAR));
+
+ tbl = new IndexRebuildAwareTable("TBL", b.build(), 100d);
+
+ tbl.addIndex(RelCollations.of(ImmutableIntList.of(2)), "IDX");
+
+ publicSchema = createSchema(tbl);
+ }
+
+ /** */
+ @Test
+ public void testIndexRebuild() throws Exception {
+ String sql = "SELECT * FROM TBL WHERE id = 0";
+
+ assertPlan(sql, publicSchema, isInstanceOf(IgniteIndexScan.class));
+
+ tbl.markIndexRebuildInProgress(true);
+
+ assertPlan(sql, publicSchema, isInstanceOf(IgniteTableScan.class));
+
+ tbl.markIndexRebuildInProgress(false);
+
+ assertPlan(sql, publicSchema, isInstanceOf(IgniteIndexScan.class));
+ }
+
+ /** */
+ @Test
+ public void testConcurrentIndexRebuildStateChange() throws Exception {
+ String sql = "SELECT * FROM TBL WHERE id = 0";
+
+ AtomicBoolean stop = new AtomicBoolean();
+
+ IgniteInternalFuture<?> fut = GridTestUtils.runAsync(() -> {
+ while (!stop.get()) {
+ tbl.markIndexRebuildInProgress(true);
+ tbl.markIndexRebuildInProgress(false);
+ }
+ });
+
+ try {
+ for (int i = 0; i < 1000; i++) {
+ IgniteRel rel = physicalPlan(sql, publicSchema);
+
+ assertTrue(rel instanceof IgniteTableScan || rel instanceof IgniteIndexScan);
+ }
+ }
+ finally {
+ stop.set(true);
+ }
+
+ fut.get();
+ }
+
+ /** */
+ @Test
+ public void testIndexScanRewriter() throws Exception {
Review comment:
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] korlov42 commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r804408788
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteIndexScan.java
##########
@@ -111,11 +104,13 @@ private IgniteIndexScan(
@Nullable List<RexNode> proj,
@Nullable RexNode cond,
@Nullable IndexConditions idxCond,
- @Nullable ImmutableBitSet requiredCols
+ @Nullable ImmutableBitSet requiredCols,
+ @Nullable RelCollation collation
Review comment:
Did you forgot to remove annotation 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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r803530067
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/IndexRebuildIntegrationTest.java
##########
@@ -0,0 +1,316 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.query.calcite.integration;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.cluster.ClusterState;
+import org.apache.ignite.configuration.DataRegionConfiguration;
+import org.apache.ignite.configuration.DataStorageConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.cache.query.index.IndexProcessor;
+import org.apache.ignite.internal.managers.indexing.IndexesRebuildTask;
+import org.apache.ignite.internal.processors.cache.GridCacheContext;
+import org.apache.ignite.internal.processors.query.calcite.QueryChecker;
+import org.apache.ignite.internal.processors.query.schema.IndexRebuildCancelToken;
+import org.apache.ignite.internal.processors.query.schema.SchemaIndexCacheVisitorClosure;
+import org.apache.ignite.internal.util.future.GridFutureAdapter;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.hamcrest.CoreMatchers;
+import org.junit.Test;
+
+/**
+ * Test index rebuild.
+ */
+public class IndexRebuildIntegrationTest extends AbstractBasicIntegrationTest {
+ /** Index rebuild init latch. */
+ private static CountDownLatch initLatch;
+
+ /** Index rebuild start latch. */
+ private static CountDownLatch startLatch;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ IndexProcessor.idxRebuildCls = BlockingIndexesRebuildTask.class;
+
+ return super.getConfiguration(igniteInstanceName).setDataStorageConfiguration(
+ new DataStorageConfiguration().setDefaultDataRegionConfiguration(
+ new DataRegionConfiguration().setPersistenceEnabled(true)));
+ }
+
+ /** {@inheritDoc} */
+ @Override protected int nodeCount() {
+ return 2;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ initLatch = null;
+ startLatch = null;
+
+ super.beforeTest();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() {
+ // Override super method to skip caches destroy.
+ cleanQueryPlanCache();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTestsStarted() throws Exception {
+ cleanPersistenceDir();
+
+ super.beforeTestsStarted();
+
+ client.cluster().state(ClusterState.ACTIVE);
+
+ executeSql("CREATE TABLE tbl (id INT PRIMARY KEY, val VARCHAR, val2 VARCHAR) WITH CACHE_NAME=\"test\"");
+ executeSql("CREATE INDEX idx_id_val ON tbl (id DESC, val)");
+ executeSql("CREATE INDEX idx_id_val2 ON tbl (id, val2 DESC)");
+ executeSql("CREATE INDEX idx_val ON tbl (val DESC)");
+
+ for (int i = 0; i < 100; i++)
+ executeSql("INSERT INTO tbl VALUES (?, ?, ?)", i, "val" + i, "val" + i);
+
+ executeSql("CREATE TABLE tbl2 (id INT PRIMARY KEY, val VARCHAR)");
+
+ for (int i = 0; i < 100; i++)
+ executeSql("INSERT INTO tbl2 VALUES (?, ?)", i, "val" + i);
+ }
+
+ /** */
+ @Test
+ public void testRebuildOnInitiatorNode() throws Exception {
+ String sql = "SELECT * FROM tbl WHERE id = 0 AND val='val0'";
+
+ QueryChecker validChecker = assertQuery(grid(0), sql)
+ .matches(QueryChecker.containsIndexScan("PUBLIC", "TBL", "IDX_ID_VAL"))
Review comment:
Then we need to reuse the same queries for integration and planning tests. And we should ensure that queries in planning tests can't be changed without corresponding changes to integration tests. Perhaps, introduce some constants with queries. But I think it's not much convenient, so it's better to just recheck the required rel op in integration tests.
Without concurrent workload, the plan should be the same for explain and the query. It's much more likely that plan in planner test for the same query will be different than plan for integration test (and there are a lot of such cases currently).
--
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] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r792359864
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteIndexScan.java
##########
@@ -39,13 +41,18 @@
/** */
private final long sourceId;
+ /** */
+ private final RelCollation idxCollation;
+
/**
* Constructor used for deserialization.
*
* @param input Serialized representation.
*/
public IgniteIndexScan(RelInput input) {
- super(changeTraits(input, IgniteConvention.INSTANCE));
+ super(changeTraits(input, IgniteConvention.INSTANCE, input.getCollation()));
Review comment:
Here we need information about collation on remote nodes, for other rels we don't need such information.
--
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] korlov42 commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
korlov42 commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r804408443
##########
File path: modules/calcite/src/test/java/org/apache/ignite/testsuites/ExecutionTestSuite.java
##########
@@ -53,6 +54,7 @@
IntersectExecutionTest.class,
RuntimeSortedIndexTest.class,
LimitExecutionTest.class,
+ LogicalRelImplementorTest.class,
Review comment:
Execution test suite is supposed to verify actual execution of particular node or group of nodes. Let's add this test to the IgniteCalciteTestSuite.
--
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] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r792361096
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rule/logical/ExposeIndexRule.java
##########
@@ -71,7 +71,10 @@ private static boolean preMatch(IgniteLogicalTableScan scan) {
.map(idx -> idx.toRel(cluster, optTable, proj, condition, requiredCols))
.collect(Collectors.toList());
- assert !indexes.isEmpty();
+ indexes.removeIf(Objects::isNull);
Review comment:
This block was rewritten, so there is no null check is required now.
--
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] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r796634352
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteIndexScan.java
##########
@@ -39,13 +41,18 @@
/** */
private final long sourceId;
+ /** */
+ private final RelCollation idxCollation;
+
/**
* Constructor used for deserialization.
*
* @param input Serialized representation.
*/
public IgniteIndexScan(RelInput input) {
- super(changeTraits(input, IgniteConvention.INSTANCE));
+ super(changeTraits(input, IgniteConvention.INSTANCE, input.getCollation()));
Review comment:
> Perhaps, it's better to store index collation in the "collation" field and restore output collation on remote nodes by index collation and projects.
Hmm, but in this case sort node will be inserted even if it's not required (for example, when we just filter values by index), looks like it's not an optimal solution.
--
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] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r792358729
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementor.java
##########
@@ -298,13 +301,101 @@ public LogicalRelImplementor(
Supplier<Row> upper = upperCond == null ? null : expressionFactory.rowSource(upperCond);
Function<Row, Row> prj = projects == null ? null : expressionFactory.project(projects, rowType);
+ ColocationGroup grp = ctx.group(rel.sourceId());
+
IgniteIndex idx = tbl.getIndex(rel.indexName());
- ColocationGroup group = ctx.group(rel.sourceId());
+ if (idx != null) {
+ Iterable<Row> rowsIter = idx.scan(ctx, grp, filters, lower, upper, prj, requiredColumns);
+
+ return new ScanNode<>(ctx, rowType, rowsIter);
+ }
+ else {
+ // Index was invalidated after planning, workaround through table-scan -> sort -> index spool.
+ RelCollation collation = TraitUtils.collation(rel);
+
+ boolean filterHasCorrelation = condition != null && RexUtils.hasCorrelation(condition);
+ boolean projectHasCorrelation = projects != null && RexUtils.hasCorrelation(projects);
+ boolean spoolNodeRequired = projectHasCorrelation || filterHasCorrelation;
+ boolean projNodeRequired = projects != null && spoolNodeRequired;
+ boolean hasCollation = collation != null && !collation.getFieldCollations().isEmpty();
+
+ Iterable<Row> rowsIter = tbl.scan(
+ ctx,
+ grp,
+ filterHasCorrelation ? null : filters,
+ projNodeRequired ? null : prj,
+ requiredColumns
+ );
- Iterable<Row> rowsIter = idx.scan(ctx, group, filters, lower, upper, prj, requiredColumns);
+ Node<Row> node = new ScanNode<>(ctx, rowType, rowsIter);
- return new ScanNode<>(ctx, rowType, rowsIter);
+ if (hasCollation || spoolNodeRequired) {
Review comment:
Comment added
--
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] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r792360432
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteIndexScan.java
##########
@@ -39,13 +41,18 @@
/** */
private final long sourceId;
+ /** */
+ private final RelCollation idxCollation;
Review comment:
There is already `collation` (in traits), so it will be confusion. (but `idxCollation` already removed)
--
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] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r796603349
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteIndexScan.java
##########
@@ -39,13 +41,18 @@
/** */
private final long sourceId;
+ /** */
+ private final RelCollation idxCollation;
+
/**
* Constructor used for deserialization.
*
* @param input Serialized representation.
*/
public IgniteIndexScan(RelInput input) {
- super(changeTraits(input, IgniteConvention.INSTANCE));
+ super(changeTraits(input, IgniteConvention.INSTANCE, input.getCollation()));
Review comment:
Sort relations don't have projections and "required columns", so sorting collation and trait collation are the same here. For index scans index collation and trait collation is different and there is confusion. Perhaps, it's better to store index collation in the "collation" field and restore output collation on remote nodes by index collation and projects. It's much easier than restoring index collation by bounds as currently implemented in this patch. WDYT?
--
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] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r793723361
##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rule/logical/ExposeIndexRule.java
##########
@@ -68,6 +68,9 @@ private static boolean preMatch(IgniteLogicalTableScan scan) {
RexNode condition = scan.condition();
ImmutableBitSet requiredCols = scan.requiredColumns();
+ if (igniteTable.isIndexRebuildInProgress())
Review comment:
Calcite has some invariant and asserts preMatch after the rule is processed, so, in case of concurrent index rebuild flag change, there will be assertion error.
--
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] alex-plekhanov commented on a change in pull request #9671: IGNITE-16111 Index rebuild handling
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #9671:
URL: https://github.com/apache/ignite/pull/9671#discussion_r804450691
##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/IndexRebuildIntegrationTest.java
##########
@@ -0,0 +1,316 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.query.calcite.integration;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.cluster.ClusterState;
+import org.apache.ignite.configuration.DataRegionConfiguration;
+import org.apache.ignite.configuration.DataStorageConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.cache.query.index.IndexProcessor;
+import org.apache.ignite.internal.managers.indexing.IndexesRebuildTask;
+import org.apache.ignite.internal.processors.cache.GridCacheContext;
+import org.apache.ignite.internal.processors.query.calcite.QueryChecker;
+import org.apache.ignite.internal.processors.query.schema.IndexRebuildCancelToken;
+import org.apache.ignite.internal.processors.query.schema.SchemaIndexCacheVisitorClosure;
+import org.apache.ignite.internal.util.future.GridFutureAdapter;
+import org.apache.ignite.testframework.GridTestUtils;
+import org.hamcrest.CoreMatchers;
+import org.junit.Test;
+
+/**
+ * Test index rebuild.
+ */
+public class IndexRebuildIntegrationTest extends AbstractBasicIntegrationTest {
+ /** Index rebuild init latch. */
+ private static CountDownLatch initLatch;
+
+ /** Index rebuild start latch. */
+ private static CountDownLatch startLatch;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ IndexProcessor.idxRebuildCls = BlockingIndexesRebuildTask.class;
+
+ return super.getConfiguration(igniteInstanceName).setDataStorageConfiguration(
+ new DataStorageConfiguration().setDefaultDataRegionConfiguration(
+ new DataRegionConfiguration().setPersistenceEnabled(true)));
+ }
+
+ /** {@inheritDoc} */
+ @Override protected int nodeCount() {
+ return 2;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ initLatch = null;
+ startLatch = null;
+
+ super.beforeTest();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() {
+ // Override super method to skip caches destroy.
+ cleanQueryPlanCache();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTestsStarted() throws Exception {
+ cleanPersistenceDir();
+
+ super.beforeTestsStarted();
+
+ client.cluster().state(ClusterState.ACTIVE);
+
+ executeSql("CREATE TABLE tbl (id INT PRIMARY KEY, val VARCHAR, val2 VARCHAR) WITH CACHE_NAME=\"test\"");
+ executeSql("CREATE INDEX idx_id_val ON tbl (id DESC, val)");
+ executeSql("CREATE INDEX idx_id_val2 ON tbl (id, val2 DESC)");
+ executeSql("CREATE INDEX idx_val ON tbl (val DESC)");
+
+ for (int i = 0; i < 100; i++)
+ executeSql("INSERT INTO tbl VALUES (?, ?, ?)", i, "val" + i, "val" + i);
+
+ executeSql("CREATE TABLE tbl2 (id INT PRIMARY KEY, val VARCHAR)");
+
+ for (int i = 0; i < 100; i++)
+ executeSql("INSERT INTO tbl2 VALUES (?, ?)", i, "val" + i);
+ }
+
+ /** */
+ @Test
+ public void testRebuildOnInitiatorNode() throws Exception {
+ String sql = "SELECT * FROM tbl WHERE id = 0 AND val='val0'";
+
+ QueryChecker validChecker = assertQuery(grid(0), sql)
+ .matches(QueryChecker.containsIndexScan("PUBLIC", "TBL", "IDX_ID_VAL"))
Review comment:
It's not a duplication, LogicalRelImplementorTest doesn't check execution node internals (and can't check it without reflection or without exposing these internals).
The integration part does not check the plan, it checks result, but ensures that this result was received using the correct code path.
--
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