Copilot commented on code in PR #7283:
URL: https://github.com/apache/ignite-3/pull/7283#discussion_r2646086012
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestNode.java:
##########
@@ -206,7 +225,8 @@ public boolean handle(FailureContext failureCtx) {
new ExpressionFactoryImpl(
Commons.typeFactory(), 1024,
CaffeineCacheFactory.INSTANCE
),
- 5_000
+ 5_000,
+ SqlPlanToTxSchemaVersionValidator.create(ts ->
CompletableFutures.nullCompletedFuture(), catalogService)
Review Comment:
The call to SqlPlanToTxSchemaVersionValidator.create has incorrect
arguments. The factory method signature is create(SchemaSyncService
schemaSyncService, CatalogService catalogService), but the first argument is a
lambda function (ts -> CompletableFutures.nullCompletedFuture()). This should
use AlwaysSyncedSchemaSyncService (which is already used on line 254) as the
first parameter instead of the lambda.
```suggestion
SqlPlanToTxSchemaVersionValidator.create(new
AlwaysSyncedSchemaSyncService(), catalogService)
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestNode.java:
##########
@@ -187,6 +194,18 @@ public boolean handle(FailureContext failureCtx) {
}
}
+ BiFunction<MultiStepPlan, QueryTransactionWrapper,
CompletableFuture<Void>> validator = (plan, tx) -> {
+ HybridTimestamp ts = tx.unwrap().schemaTimestamp();
+
+ int requiredCatalog =
catalogService.activeCatalogVersion(ts.longValue());
+
+ if (requiredCatalog != plan.catalogVersion()) {
+ return CompletableFuture.failedFuture(new
SqlPlanOutdatedException());
+ }
+
+ return CompletableFutures.nullCompletedFuture();
+ };
+
Review Comment:
The variable 'validator' is defined but never used. It should either be
removed as dead code, or it was intended to be used in place of the
SqlPlanToTxSchemaVersionValidator.create() call on line 229.
```suggestion
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -203,6 +206,7 @@ public class ExecutionServiceImpl<RowT> implements
ExecutionService, LogicalTopo
* @param clockService Clock service.
* @param killCommandHandler Kill command handler.
* @param shutdownTimeout Shutdown timeout.
+ * @param planValidator Validator of the catalog version from the plan
relative to the started transactio.
Review Comment:
The word "transactio" is missing the final "n". It should be "transaction".
```suggestion
* @param planValidator Validator of the catalog version from the plan
relative to the started transaction.
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -258,6 +264,7 @@ public ExecutionServiceImpl(
* @param clockService Clock service.
* @param killCommandHandler Kill command handler.
* @param shutdownTimeout Shutdown timeout.
+ * @param planValidator Validator of the catalog version from the plan
relative to the started transactio.
Review Comment:
The word "transactio" is missing the final "n". It should be "transaction".
```suggestion
* @param planValidator Validator of the catalog version from the plan
relative to the started transaction.
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlPlanToTxSchemaVersionValidator.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.sql.engine;
+
+import static
org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.internal.catalog.CatalogService;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
+import org.apache.ignite.internal.schema.SchemaSyncService;
+import org.apache.ignite.internal.sql.engine.exec.SqlPlanOutdatedException;
+import org.apache.ignite.internal.sql.engine.prepare.MultiStepPlan;
+import org.apache.ignite.internal.sql.engine.tx.QueryTransactionWrapper;
+import org.apache.ignite.internal.tx.InternalTransaction;
+import org.jetbrains.annotations.TestOnly;
+
+/**
+ * SQL query execution plan validator.
+ *
+ * <p>Performs validation of the catalog version from the {@link MultiStepPlan
multi-step plan} relative to the started transaction.
+ */
+public class SqlPlanToTxSchemaVersionValidator {
+ @TestOnly
+ public static final SqlPlanToTxSchemaVersionValidator NOOP = new
NoopSqlPlanToTxSchemaVersionValidator();
+
+ private final SchemaSyncService schemaSyncService;
+ private final CatalogService catalogService;
+
+ /**
+ * Compares the catalog version from the plan with the version of the
active catalog
+ * at the {@link InternalTransaction#schemaTimestamp() schema time} of the
transaction.
+ *
+ * @param plan {@link MultiStepPlan multi-step} execution plan.
+ * @param tx Query transaction wrapper.
+ * @return Successfully completed future if the provided transaction is
explicit or the catalog versions match.
+ * Otherwise returns a future completed with an exception {@link
SqlPlanOutdatedException},
Review Comment:
The javadoc sentence is incomplete - it ends with a comma instead of a
period. The sentence "Otherwise returns a future completed with an exception
{@link SqlPlanOutdatedException}," should end with a period.
```suggestion
* Otherwise returns a future completed with an exception {@link
SqlPlanOutdatedException}.
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/SqlOutdatedPlanTest.java:
##########
@@ -0,0 +1,237 @@
+/*
+ * 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.sql.engine.exec;
+
+import static org.apache.ignite.internal.catalog.CatalogTestUtils.columnParams;
+import static org.apache.ignite.internal.sql.SqlCommon.DEFAULT_SCHEMA_NAME;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.await;
+import static org.apache.ignite.sql.ColumnType.INT32;
+import static org.awaitility.Awaitility.await;
Review Comment:
There are two conflicting static imports for 'await': one from
IgniteTestUtils (line 22) and one from Awaitility (line 24). This creates
ambiguity when 'await' is called in the code. Only one should be imported
statically, and the other should be called with its full qualifier if needed.
```suggestion
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
