tkalkirill commented on code in PR #2413:
URL: https://github.com/apache/ignite-3/pull/2413#discussion_r1285379681
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestNode.java:
##########
@@ -103,7 +103,7 @@ public class TestNode implements LifecycleAware {
this.nodeName = nodeName;
var ps = new PrepareServiceImpl(nodeName, 0,
mock(DdlSqlToCommandConverter.class), PLANNING_TIMEOUT,
mock(MetricManager.class));
this.prepareService = registerService(ps);
- this.schema = schemaManager.schema("PUBLIC");
+ this.schema = schemaManager.schema("PUBLIC", -1);
Review Comment:
Please replace with `CatalogService#DEFAULT_SCHEMA_NAME`
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogService.java:
##########
@@ -78,5 +79,13 @@ public interface CatalogService {
*/
int latestCatalogVersion();
+ /**
+ * Returns a future, which completes, when catalog of given version will
be available.
+ *
+ * @param version Catalog version to wait for.
+ * @return Operation future.
Review Comment:
```suggestion
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/SqlSchemaManager.java:
##########
@@ -39,18 +34,15 @@ public interface SqlSchemaManager {
* Returns a table by given id.
*
* @param id An id of required table.
- *
* @return The table.
*/
IgniteTable tableById(int id);
/**
- * Wait for {@code ver} schema version, just a stub, need to be removed
after IGNITE-18733.
- */
- CompletableFuture<?> actualSchemaAsync(long ver);
-
- /**
- * Returns a required schema if specified, or default schema otherwise.
+ * Returns a future to wait for given SQL schema version readiness.
+ *
+ * @param version SQL schema version to wait.
+ * @return Operation future.
Review Comment:
```suggestion
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/PredefinedSchemaManager.java:
##########
@@ -61,35 +60,23 @@ public class PredefinedSchemaManager implements
SqlSchemaManager {
schema.getTableNames().stream()
.map(schema::getTable)
.map(IgniteTable.class::cast)
- .collect(Collectors.toMap(IgniteTable::id,
Function.identity()))
+
.collect(CollectionUtils.toIntMapCollector(IgniteTable::id,
Function.identity()))
Review Comment:
please use static imports
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/schema/SqlSchemaManagerTest.java:
##########
@@ -147,7 +143,7 @@ public void testOnTableDroppedHandler() {
testRevisionRegister.moveForward();
- Table schemaTable = sqlSchemaManager.schema("PUBLIC").getTable("T");
+ Table schemaTable =
sqlSchemaManager.latestSchema("PUBLIC").getTable("T");
Review Comment:
Please use `CatalogService#DEFAULT_SCHEMA_NAME`
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##########
@@ -443,9 +444,15 @@ private CompletableFuture<AsyncSqlCursor<List<Object>>>
querySingle0(
boolean implicitTxRequired = outerTx == null;
- tx.set(implicitTxRequired ? txManager.begin(!rwOp, null) :
outerTx);
+ InternalTransaction currentTx = implicitTxRequired ?
txManager.begin(!rwOp, null) : outerTx;
- SchemaPlus schema = sqlSchemaManager.schema(schemaName);
+ tx.set(currentTx);
+
+ // TODO IGNITE-18733: wait for actual metadata for TX.
+ HybridTimestamp txTimestamp = currentTx.startTimestamp();
+ int observableCatalogVersion =
catalogManager.activeCatalogVersion(txTimestamp.longValue());
+
+ SchemaPlus schema = sqlSchemaManager.schema(schemaName,
observableCatalogVersion);
Review Comment:
This is where I get confused too.
We get the catalog version and use it as the schema version, which may not
be true.
Maybe we need, by analogy with the catalog, to wait for the schema version
by `txTimestamp`?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -239,13 +239,13 @@ private AsyncCursor<List<Object>> executeQuery(
return queryManager.execute(tx, plan);
}
- private BaseQueryContext createQueryContext(UUID queryId, @Nullable String
schema, Object[] params) {
+ private BaseQueryContext createQueryContext(UUID queryId, long version,
@Nullable String schema, Object[] params) {
Review Comment:
`long version` -> `long schemaVersion`
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/SqlSchemaManager.java:
##########
@@ -39,18 +34,15 @@ public interface SqlSchemaManager {
* Returns a table by given id.
*
* @param id An id of required table.
- *
* @return The table.
*/
IgniteTable tableById(int id);
/**
- * Wait for {@code ver} schema version, just a stub, need to be removed
after IGNITE-18733.
- */
- CompletableFuture<?> actualSchemaAsync(long ver);
-
- /**
- * Returns a required schema if specified, or default schema otherwise.
+ * Returns a future to wait for given SQL schema version readiness.
+ *
+ * @param version SQL schema version to wait.
+ * @return Operation future.
*/
- SchemaPlus activeSchema(@Nullable String name, long timestamp);
+ CompletableFuture<?> schemaReadyFuture(long version);
Review Comment:
I am confused by the type of the return value of the future, I propose to
change it to `Void`, we will wait for the version of the scheme and then we
will receive it separately as we need.
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/PredefinedSchemaManager.java:
##########
@@ -61,35 +60,23 @@ public class PredefinedSchemaManager implements
SqlSchemaManager {
schema.getTableNames().stream()
.map(schema::getTable)
.map(IgniteTable.class::cast)
- .collect(Collectors.toMap(IgniteTable::id,
Function.identity()))
+
.collect(CollectionUtils.toIntMapCollector(IgniteTable::id,
Function.identity()))
);
}
}
- /** {@inheritDoc} */
- @Override
- public SchemaPlus schema(@Nullable String schema) {
- return schema == null ? root : root.getSubSchema(schema);
- }
-
/** {@inheritDoc} */
@Override
public SchemaPlus schema(String name, int version) {
Review Comment:
Please note that the **name** may be `null` in the base class and its
implementations.
--
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]
