You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "AMashenkov (via GitHub)" <gi...@apache.org> on 2023/06/14 08:32:54 UTC
[GitHub] [ignite-3] AMashenkov opened a new pull request, #2192: IGNITE-17765 Sql. Introduce cache for parsed statements
AMashenkov opened a new pull request, #2192:
URL: https://github.com/apache/ignite-3/pull/2192
https://issues.apache.org/jira/browse/IGNITE-17765
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2192: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2192:
URL: https://github.com/apache/ignite-3/pull/2192#discussion_r1229311140
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/CacheKey.java:
##########
@@ -74,25 +66,24 @@ public boolean equals(Object o) {
CacheKey cacheKey = (CacheKey) o;
- if (!schemaName.equals(cacheKey.schemaName)) {
+ if (catalogVersion != cacheKey.catalogVersion){
return false;
}
if (!query.equals(cacheKey.query)) {
return false;
}
- if (!Objects.equals(contextKey, cacheKey.contextKey)) {
+ if (!query.equals(cacheKey.query)) {
Review Comment:
you've checked `query` already
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] AMashenkov commented on pull request #2192: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on PR #2192:
URL: https://github.com/apache/ignite-3/pull/2192#issuecomment-1598659423
- Cache plans for original query string as for normalized query string. Cache coherence for original and normalized string is not supported.
- Fixed EXPLAIN to use cached plan
- Tests 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-3] korlov42 commented on a diff in pull request #2192: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2192:
URL: https://github.com/apache/ignite-3/pull/2192#discussion_r1229460128
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##########
@@ -383,121 +398,187 @@ private void registerIndexListener(IndexEvent evt, AbstractIndexEventListener ls
private CompletableFuture<AsyncSqlCursor<List<Object>>> querySingle0(
SessionId sessionId,
- QueryContext context,
+ QueryContext queryContext,
String sql,
Object... params
) {
Session session = sessionManager.session(sessionId);
if (session == null) {
- return CompletableFuture.failedFuture(
+ return failedFuture(
new SqlException(SESSION_NOT_FOUND_ERR, format("Session not found [{}]", sessionId)));
}
+ QueryCancel queryCancel = new QueryCancel();
+
+ try {
+ registerQueryCancel(session, new QueryCancel());
+ } catch (IllegalStateException ex) {
+ return failedFuture(new IgniteInternalException(SESSION_EXPIRED_ERR,
+ format("Session has been expired [{}]", session.sessionId()), ex));
+ }
+
String schemaName = session.properties().get(QueryProperty.DEFAULT_SCHEMA);
- InternalTransaction outerTx = context.unwrap(InternalTransaction.class);
+ InternalTransaction outerTx = queryContext.unwrap(InternalTransaction.class);
+ boolean implicitTx = outerTx == null;
- QueryCancel queryCancel = new QueryCancel();
+ // TODO: IGNITE-19497 Switch to Catalog manager and uncomment next lines.
+ long timestamp = outerTx == null ? clock.nowLong() : outerTx.startTimestamp().longValue();
+ // int plannerCatalogVersion = catalogManager.activeCatalogVersion(timestamp);
+ // int schemaId = catalogManager.schema(plannerCatalogVersion).id();
+ // SchemaPlus schema = sqlSchemaManager.schema(schemaName, plannerCatalogVersion);
- AsyncCloseable closeableResource = () -> CompletableFuture.runAsync(
- queryCancel::cancel,
- taskExecutor
- );
+ int plannerCatalogVersion = 0;
+ SchemaPlus schema = sqlSchemaManager.schema(schemaName);
- queryCancel.add(() -> session.unregisterResource(closeableResource));
+ if (schema == null) {
+ return failedFuture(new SchemaNotFoundException(schemaName));
+ }
- try {
- session.registerResource(closeableResource);
- } catch (IllegalStateException ex) {
- return CompletableFuture.failedFuture(new IgniteInternalException(SESSION_EXPIRED_ERR,
- format("Session has been expired [{}]", session.sessionId()), ex));
+ CacheKey cacheKey = new CacheKey(plannerCatalogVersion, schemaName, sql, params);
+
+ BaseQueryContext plannerContext = BaseQueryContext.builder()
+ .frameworkConfig(Frameworks.newConfigBuilder(FRAMEWORK_CONFIG).defaultSchema(schema).build())
+ .logger(LOG)
+ .cancel(queryCancel)
+ .parameters(params)
+ .plannerTimeout(PLANNER_TIMEOUT)
+ .build();
+
+ CompletableFuture<QueryPlan> planFuture = queryCache.get(cacheKey);
+
+ if (planFuture == null) {
+ planFuture = CompletableFuture.supplyAsync(() -> {
+ // Parse query
+ StatementParseResult parseResult = IgniteSqlParser.parse(sql, StatementParseResult.MODE);
+ SqlNode sqlNode = parseResult.statement();
+
+ // Validate statement
+ validateParsedStatement(queryContext, parseResult, sqlNode, params);
+
+ return sqlNode;
+ }, taskExecutor).thenCompose(sqlNode -> {
+ if (skipCache(Commons.getQueryType(sqlNode))) {
+ // Prepare query plan without caching.
+ //TODO: IGNITE-17765 Can we make a sync callhere ? drop planning pool? or move parsing to planing pool?
+ return prepareSvc.prepareAsync(sqlNode, plannerContext);
+ }
+
+ // Try query plan for normalized query, or create a new one asynchronously.
+ CacheKey normalizedQueryCacheKey = new CacheKey(plannerCatalogVersion, schemaName, sqlNode.toString(), params);
+
+ CompletableFuture<QueryPlan> planFuture0 = queryCache.computeIfAbsent(
+ normalizedQueryCacheKey,
+ k -> prepareSvc.prepareAsync(sqlNode, plannerContext)
+ );
+
+ queryCache.putIfAbsent(cacheKey, planFuture0);
+
+ // Copy shared plan.
+ return planFuture0.thenApply(QueryPlan::copy);
+ });
+ } else {
+ // Copy shared plan.
+ planFuture = planFuture.thenApply(QueryPlan::copy);
}
- CompletableFuture<Void> start = new CompletableFuture<>();
+ return planFuture
+ .thenComposeAsync(plan -> {
+ // Validate plan
+ if (SqlQueryType.DDL == plan.type() && outerTx != null) {
+ return failedFuture(new SqlException(UNSUPPORTED_DDL_OPERATION_ERR, "DDL doesn't support transactions."));
+ }
+
+ InternalTransaction tx = implicitTx ? txManager.begin(plan.type() != DML) : outerTx;
+
+ try {
+ /* TODO IGNITE-19497 Restart planning if plan cannot be used in transaction.
+ int txCatalogVersion = catalogManager.activeCatalogVersion(tx.startTimestamp().longValue());
+
+ if (implicitTx && plannerCatalogVersion != txCatalogVersion) {
+ LOG.info("Retry query planning: plannerCatalogVersion={}, txCatalogVersion={}",
+ plannerCatalogVersion, txCatalogVersion);
+
+ // TODO IGNITE-19497 Prevent infinite planning, e.g. by setting max number of tries.
+ return tx.rollbackAsync()
+ .thenComposeAsync(ignore -> querySingle0(sessionId, queryContext, sql, params), taskExecutor);
+ } */
Review Comment:
the same, let's remove all unrelated code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] lowka commented on a diff in pull request #2192: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #2192:
URL: https://github.com/apache/ignite-3/pull/2192#discussion_r1229388536
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##########
@@ -374,121 +389,190 @@ private void registerIndexListener(IndexEvent evt, AbstractIndexEventListener ls
private CompletableFuture<AsyncSqlCursor<List<Object>>> querySingle0(
SessionId sessionId,
- QueryContext context,
+ QueryContext queryContext,
String sql,
Object... params
) {
Session session = sessionManager.session(sessionId);
if (session == null) {
- return CompletableFuture.failedFuture(
+ return failedFuture(
new SqlException(SESSION_NOT_FOUND_ERR, format("Session not found [{}]", sessionId)));
}
+ QueryCancel queryCancel;
+
+ try {
+ queryCancel = createQueryCancel(session);
+ } catch (IllegalStateException ex) {
+ return failedFuture(new IgniteInternalException(SESSION_EXPIRED_ERR,
+ format("Session has been expired [{}]", session.sessionId()), ex));
+ }
+
String schemaName = session.properties().get(QueryProperty.DEFAULT_SCHEMA);
- InternalTransaction outerTx = context.unwrap(InternalTransaction.class);
+ InternalTransaction outerTx = queryContext.unwrap(InternalTransaction.class);
+ long timestamp = outerTx == null ? clock.nowLong() : outerTx.startTimestamp().longValue();
- QueryCancel queryCancel = new QueryCancel();
+ boolean implicitTx = outerTx == null;
- AsyncCloseable closeableResource = () -> CompletableFuture.runAsync(
- queryCancel::cancel,
- taskExecutor
- );
+ // TODO: IGNITE-19497 Switch to Catalog manager and uncomment next lines.
+ // int plannerCatalogVersion = catalogManager.activeCatalogVersion(timestamp);
+ // int schemaId = catalogManager.schema(plannerCatalogVersion).id();
+ // SchemaPlus schema = sqlSchemaManager.schema(schemaName, plannerCatalogVersion);
- queryCancel.add(() -> session.unregisterResource(closeableResource));
+ int plannerCatalogVersion = 0;
+ SchemaPlus schema = sqlSchemaManager.schema(schemaName);
- try {
- session.registerResource(closeableResource);
- } catch (IllegalStateException ex) {
- return CompletableFuture.failedFuture(new IgniteInternalException(SESSION_EXPIRED_ERR,
- format("Session has been expired [{}]", session.sessionId()), ex));
+ if (schema == null) {
+ return failedFuture(new SchemaNotFoundException(schemaName));
}
- CompletableFuture<Void> start = new CompletableFuture<>();
+ CacheKey cacheKey = new CacheKey(plannerCatalogVersion, schemaName, sql, params);
+
+ BaseQueryContext plannerContext = BaseQueryContext.builder()
+ .frameworkConfig(Frameworks.newConfigBuilder(FRAMEWORK_CONFIG).defaultSchema(schema).build())
+ .logger(LOG)
+ .cancel(queryCancel)
+ .parameters(params)
+ .plannerTimeout(PLANNER_TIMEOUT)
+ .build();
+
+ CompletableFuture<QueryPlan> planFuture = queryCache.get(cacheKey);
+
+ if (planFuture == null) {
+ planFuture = CompletableFuture.supplyAsync(() -> {
Review Comment:
I think it would be better to specify an executor here since `CompletableFuture.supplyAsync(task)` uses ForkJoin's common pool.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2192: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2192:
URL: https://github.com/apache/ignite-3/pull/2192#discussion_r1229443576
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/CacheKey.java:
##########
@@ -74,25 +65,24 @@ public boolean equals(Object o) {
CacheKey cacheKey = (CacheKey) o;
- if (!schemaName.equals(cacheKey.schemaName)) {
+ if (catalogVersion != cacheKey.catalogVersion) {
return false;
}
if (!query.equals(cacheKey.query)) {
return false;
}
- if (!Objects.equals(contextKey, cacheKey.contextKey)) {
+ if (!schemaName.equals(cacheKey.schemaName)) {
return false;
}
- return Arrays.deepEquals(paramTypes, cacheKey.paramTypes);
+ return Arrays.equals(paramTypes, cacheKey.paramTypes);
}
@Override
public int hashCode() {
int result = schemaName.hashCode();
result = 31 * result + query.hashCode();
- result = 31 * result + (contextKey != null ? contextKey.hashCode() : 0);
Review Comment:
catalogVersion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2192: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2192:
URL: https://github.com/apache/ignite-3/pull/2192#discussion_r1229448869
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##########
@@ -383,121 +398,187 @@ private void registerIndexListener(IndexEvent evt, AbstractIndexEventListener ls
private CompletableFuture<AsyncSqlCursor<List<Object>>> querySingle0(
SessionId sessionId,
- QueryContext context,
+ QueryContext queryContext,
String sql,
Object... params
) {
Session session = sessionManager.session(sessionId);
if (session == null) {
- return CompletableFuture.failedFuture(
+ return failedFuture(
new SqlException(SESSION_NOT_FOUND_ERR, format("Session not found [{}]", sessionId)));
}
+ QueryCancel queryCancel = new QueryCancel();
+
+ try {
+ registerQueryCancel(session, new QueryCancel());
Review Comment:
Why do you need new object in the session?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2192: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2192:
URL: https://github.com/apache/ignite-3/pull/2192#discussion_r1229458481
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##########
@@ -383,121 +398,187 @@ private void registerIndexListener(IndexEvent evt, AbstractIndexEventListener ls
private CompletableFuture<AsyncSqlCursor<List<Object>>> querySingle0(
SessionId sessionId,
- QueryContext context,
+ QueryContext queryContext,
String sql,
Object... params
) {
Session session = sessionManager.session(sessionId);
if (session == null) {
- return CompletableFuture.failedFuture(
+ return failedFuture(
new SqlException(SESSION_NOT_FOUND_ERR, format("Session not found [{}]", sessionId)));
}
+ QueryCancel queryCancel = new QueryCancel();
+
+ try {
+ registerQueryCancel(session, new QueryCancel());
+ } catch (IllegalStateException ex) {
+ return failedFuture(new IgniteInternalException(SESSION_EXPIRED_ERR,
+ format("Session has been expired [{}]", session.sessionId()), ex));
+ }
+
String schemaName = session.properties().get(QueryProperty.DEFAULT_SCHEMA);
- InternalTransaction outerTx = context.unwrap(InternalTransaction.class);
+ InternalTransaction outerTx = queryContext.unwrap(InternalTransaction.class);
+ boolean implicitTx = outerTx == null;
- QueryCancel queryCancel = new QueryCancel();
+ // TODO: IGNITE-19497 Switch to Catalog manager and uncomment next lines.
+ long timestamp = outerTx == null ? clock.nowLong() : outerTx.startTimestamp().longValue();
+ // int plannerCatalogVersion = catalogManager.activeCatalogVersion(timestamp);
+ // int schemaId = catalogManager.schema(plannerCatalogVersion).id();
+ // SchemaPlus schema = sqlSchemaManager.schema(schemaName, plannerCatalogVersion);
Review Comment:
Let's not jump to conclusions about how unrelated tickets might look like
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2192: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2192:
URL: https://github.com/apache/ignite-3/pull/2192#discussion_r1229435684
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/SqlQueryProcessor.java:
##########
@@ -247,7 +261,8 @@ public synchronized void start() {
busyLock
);
- sqlSchemaManager.registerListener(prepareSvc);
+ //TODO IGNITE-17765 Do we need cache clear as cache already aware of catalog versioning.
Review Comment:
well, we should not refer in TODO to the ticket under which this TODO is being 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-3] AMashenkov closed pull request #2192: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov closed pull request #2192: IGNITE-17765 Sql. Introduce cache for parsed statements
URL: https://github.com/apache/ignite-3/pull/2192
--
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