AMashenkov commented on code in PR #2413:
URL: https://github.com/apache/ignite-3/pull/2413#discussion_r1285685618
##########
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:
1. `SqlSchemaManager` is a general interface, which provides method to get
schema by name and version. It doesn't assumed any connection with Catalog, and
whether version should match catalog version or not.
I think we shouldn't fix any naming here.
2. `CatalogSqlSchemaManager is an implementation that backed by catalog. We
assume schema version is mapped to catalog version as 1:1.
3. Actually, current implementation `SqlSchemaManager` does not support
versions at all. So, any naming is possible here, as it make no sense.
Maybe, we should either use `CatalogSqlSchemaManager` as `sqlSchemaManager`
type, or add a method to `SqlSchemaManager` interface that will resolve a
version by tx timestamp. 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
