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/20 12:05:48 UTC
[GitHub] [ignite-3] AMashenkov opened a new pull request, #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
AMashenkov opened a new pull request, #2226:
URL: https://github.com/apache/ignite-3/pull/2226
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] xtern commented on a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1243470496
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -296,4 +364,67 @@ private ResultSetMetadata resultSetMetadata(
}
);
}
+
+ private static SqlNode parse(String query, QueryContext queryContext, BaseQueryContext ctx) {
+ StatementParseResult parseResult = IgniteSqlParser.parse(query, StatementParseResult.MODE);
+
+ SqlNode sqlNode = parseResult.statement();
+
+ validateParsedStatement(queryContext, parseResult, sqlNode, ctx.parameters());
+
+ return sqlNode;
+ }
+
+ private static boolean skipCache(SqlNode sqlNode) {
+ SqlKind kind = sqlNode.getKind();
+
+ switch (kind) {
+ case SELECT:
+ case INSERT:
+ return false;
Review Comment:
I don't quite understand why we don't caching only 'INSERT' instead of caching all DML queries?
For example, I have the following results of `InsertBenchmark` if we changing `insert` query to something with UPDATE: `update usertable set field3='a' where ycsb_key=?`
Patch not applied:
```
Benchmark (fsync) Mode Cnt Score Error Units
InsertBenchmark.sqlInsert false avgt 1.288 ms/op
```
Patch applied:
```
Benchmark (fsync) Mode Cnt Score Error Units
InsertBenchmark.sqlInsert false avgt 3.553 ms/op
```
--
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 #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#issuecomment-1598650926
- Implemented indirect cache for query plans: query string -> normalized query srting -> query plan
- 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 #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1245059403
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestNode.java:
##########
@@ -81,7 +80,7 @@
public class TestNode implements LifecycleAware {
private final String nodeName;
private final SchemaPlus schema;
- private final PrepareService prepareService;
+ private final PrepareServiceImpl prepareService;
Review Comment:
TestNode must use interface, not implementation
--
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] xtern commented on a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1245634852
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PrepareServiceSelfTest.java:
##########
@@ -0,0 +1,294 @@
+/*
+ * 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.planner;
+
+import static org.apache.ignite.internal.sql.engine.prepare.PrepareServiceImpl.validateParsedStatement;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureExceptionMatcher.willThrow;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static org.apache.ignite.internal.util.ArrayUtils.OBJECT_EMPTY_ARRAY;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Stream;
+import org.apache.calcite.runtime.CalciteContextException;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.ignite.internal.schema.NativeTypes;
+import org.apache.ignite.internal.sql.engine.QueryContext;
+import org.apache.ignite.internal.sql.engine.SqlQueryType;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders;
+import org.apache.ignite.internal.sql.engine.prepare.IgnitePlanner;
+import org.apache.ignite.internal.sql.engine.prepare.PlannerHelper;
+import org.apache.ignite.internal.sql.engine.prepare.PrepareServiceImpl;
+import org.apache.ignite.internal.sql.engine.prepare.QueryOptimizer;
+import org.apache.ignite.internal.sql.engine.prepare.ddl.DdlSqlToCommandConverter;
+import org.apache.ignite.internal.sql.engine.rel.IgniteRel;
+import org.apache.ignite.internal.sql.engine.sql.IgniteSqlParser;
+import org.apache.ignite.internal.sql.engine.sql.StatementParseResult;
+import org.apache.ignite.internal.sql.engine.trait.IgniteDistributions;
+import org.apache.ignite.internal.sql.engine.util.BaseQueryContext;
+import org.apache.ignite.sql.SqlException;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.mockito.Mockito;
+
+/**
+ * Prepare service self test.
+ */
+public class PrepareServiceSelfTest extends AbstractPlannerTest {
+ private final QueryContext queryCtx = QueryContext.create(SqlQueryType.ALL);
+ private PrepareServiceImpl service;
+
+ private QueryOptimizer queryPlannerSpy;
+ private DdlSqlToCommandConverter ddlPlannerSpy;
+
+ @BeforeEach
+ void setUp() {
+ queryPlannerSpy = Mockito.spy(new TestQueryOptimizer());
+ ddlPlannerSpy = Mockito.spy(new DdlSqlToCommandConverter(Map.of(), () -> "default"));
+
+ service = new PrepareServiceImpl(
+ "node",
+ 10,
+ ddlPlannerSpy,
+ queryPlannerSpy
+ );
+
+ service.start();
+ }
+
+ @AfterEach
+ void tearDown() throws Exception {
+ service.stop();
+ }
+
+ @SuppressWarnings("WeakerAccess")
+ static Stream<Arguments> queries() {
+ return Stream.of(
+ // Query
+ Arguments.of("SELECT * FROM tbl WHERE id > 0", OBJECT_EMPTY_ARRAY),
+ // Query with args
+ Arguments.of("SELECT * FROM tbl WHERE id > ?", new Object[]{1}),
+ // DML
+ Arguments.of("INSERT INTO tbl VALUES (1, '42')", OBJECT_EMPTY_ARRAY),
+ Arguments.of("UPDATE tbl SET VAL = '42' WHERE id = 1", OBJECT_EMPTY_ARRAY),
+ // TODO IGNITE-19866: uncomment DELETE statement
+ // Arguments.of("DELETE FROM tbl WHERE id = 1", OBJECT_EMPTY_ARRAY),
Review Comment:
This issue will not reproduce if you add PK column to the test table.
--
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 #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov closed pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
URL: https://github.com/apache/ignite-3/pull/2226
--
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 a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1245087309
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestNode.java:
##########
@@ -81,7 +80,7 @@
public class TestNode implements LifecycleAware {
private final String nodeName;
private final SchemaPlus schema;
- private final PrepareService prepareService;
+ private final PrepareServiceImpl prepareService;
Review Comment:
fixed
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -72,10 +84,13 @@ public class PrepareServiceImpl implements PrepareService, SchemaUpdateListener
private final DdlSqlToCommandConverter ddlConverter;
- private final ConcurrentMap<CacheKey, CompletableFuture<QueryPlan>> cache;
+ private final ConcurrentMap<CacheKey, CompletableFuture<QueryPlan>> planCache;
+ private final ConcurrentMap<CacheKey, String> normalizedQueryCache;
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-3] xtern commented on a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1243298133
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PrepareServiceSelfTest.java:
##########
@@ -0,0 +1,273 @@
+/*
+ * 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.planner;
+
+import static org.apache.ignite.internal.sql.engine.prepare.PrepareServiceImpl.validateParsedStatement;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureExceptionMatcher.willThrow;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.List;
+import java.util.Map;
+import java.util.function.BiFunction;
+import java.util.stream.Stream;
+import org.apache.calcite.runtime.CalciteContextException;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.ignite.internal.schema.NativeTypes;
+import org.apache.ignite.internal.sql.engine.QueryContext;
+import org.apache.ignite.internal.sql.engine.SqlQueryType;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders;
+import org.apache.ignite.internal.sql.engine.prepare.IgnitePlanner;
+import org.apache.ignite.internal.sql.engine.prepare.PlannerHelper;
+import org.apache.ignite.internal.sql.engine.prepare.PrepareServiceImpl;
+import org.apache.ignite.internal.sql.engine.prepare.ddl.DdlSqlToCommandConverter;
+import org.apache.ignite.internal.sql.engine.rel.IgniteRel;
+import org.apache.ignite.internal.sql.engine.sql.IgniteSqlParser;
+import org.apache.ignite.internal.sql.engine.sql.StatementParseResult;
+import org.apache.ignite.internal.sql.engine.trait.IgniteDistributions;
+import org.apache.ignite.internal.sql.engine.util.BaseQueryContext;
+import org.apache.ignite.sql.SqlException;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.mockito.Mockito;
+
+/**
+ * Prepare service self test.
+ */
+public class PrepareServiceSelfTest extends AbstractPlannerTest {
+ private final QueryContext queryCtx = QueryContext.create(SqlQueryType.ALL);
+ private PrepareServiceImpl service;
+
+ private BiFunction<SqlNode, IgnitePlanner, IgniteRel> queryPlannerSpy;
+ private DdlSqlToCommandConverter ddlPlannerSpy;
+
+ @BeforeEach
+ void setUp() {
+ queryPlannerSpy = Mockito.spy(new QueryOptimizer());
+ ddlPlannerSpy = Mockito.spy(new DdlSqlToCommandConverter(Map.of(), () -> "default"));
+
+ service = new PrepareServiceImpl(
+ "node",
+ 10,
+ ddlPlannerSpy,
+ queryPlannerSpy
+ );
+
+ service.start();
+ }
+
+ @AfterEach
+ void tearDown() throws Exception {
+ service.stop();
+ }
+
+ static Stream<Arguments> queries() {
+ return Stream.of(
+ // Query
+ Arguments.of("SELECT * FROM tbl WHERE id > 0", "SELECT * /* a comment. */ FROM tbl WHERE id > 0", new Object[]{}),
+ // Query with args
+ Arguments.of("SELECT * FROM tbl WHERE id > ?", "SELECT * /* a comment. */ FROM tbl WHERE id > ?", new Object[]{1}),
+ // DML
+ Arguments.of("INSERT INTO tbl VALUES (1, '42')", "INSERT INTO /* a comment. */ tbl VALUES (1, '42')", new Object[]{}),
+ // DML with args
+ Arguments.of("INSERT INTO tbl VALUES (?, ?)", "INSERT INTO /* a comment. */ tbl VALUES (?, ?)", new Object[]{1, "42"})
+ );
+ }
+
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("queries")
+ public void queryCache(String query1, String query2, Object[] params) {
+ // Preparing query caches plan for both query and normalized query.
+ assertThat(service.prepareAsync(query1, queryCtx, createContext(params)), willBe(notNullValue()));
+ assertEquals(1, service.parseCacheSize());
+ assertEquals(1, service.planCacheSize());
+ Mockito.verify(queryPlannerSpy, Mockito.times(1)).apply(Mockito.any(), Mockito.any());
+
+ // Preparing same query returns plan from cache.
+ assertThat(service.prepareAsync(query1, queryCtx, createContext(params)), willBe(notNullValue()));
+ assertEquals(1, service.parseCacheSize());
+ assertEquals(1, service.planCacheSize());
+ Mockito.verify(queryPlannerSpy, Mockito.times(1)).apply(Mockito.any(), Mockito.any());
+
+ // Preparing similar query returns cached plan and also cache the plan for the query.
+ assertThat(service.prepareAsync(query2, queryCtx, createContext(params)), willBe(notNullValue()));
+ assertEquals(2, service.parseCacheSize());
+ assertEquals(1, service.planCacheSize());
+ Mockito.verify(queryPlannerSpy, Mockito.times(1)).apply(Mockito.any(), Mockito.any());
+ }
+
+
+ private SqlNode parse(String sql, Object... params) {
+ // Parse query
+ StatementParseResult parseResult = IgniteSqlParser.parse(sql, StatementParseResult.MODE);
+ SqlNode sqlNode = parseResult.statement();
+
+ // Validate statement
+ validateParsedStatement(queryCtx, parseResult, sqlNode, params);
+
+ return sqlNode;
+ }
+
+ @Test
+ public void ddlBypassCache() {
+ String query = "CREATE TABLE tbl0(id INTEGER PRIMARY KEY, val VARCHAR);";
+
+ assertThat(service.prepareAsync(query, queryCtx, createContext()), willBe(notNullValue()));
+ assertEquals(0, service.planCacheSize());
+ Mockito.verify(ddlPlannerSpy, Mockito.times(1)).convert(Mockito.any(), Mockito.any());
+ // DDL goes a separate flow via ddl converter.
+ Mockito.verifyNoInteractions(queryPlannerSpy);
+
+ // Prepare DDL query once again.
+ assertThat(service.prepareAsync(query, queryCtx, createContext()), willBe(notNullValue()));
+ assertEquals(0, service.planCacheSize());
+ Mockito.verify(ddlPlannerSpy, Mockito.times(2)).convert(Mockito.any(), Mockito.any());
+ }
+
+ @Test
+ public void errors() {
+ String query = "invalid query"; // Invalid table name.
+
+ assertThat(service.prepareAsync(query, queryCtx, createContext()), willThrow(SqlException.class));
+ assertEquals(0, service.planCacheSize());
+ Mockito.verifyNoInteractions(queryPlannerSpy);
+
+ query = "SELECT * FROM tbl2 WHERE id > 0"; // Invalid table name.
+
+ assertThat(service.prepareAsync(query, queryCtx, createContext()), willThrow(CalciteContextException.class));
+ assertEquals(1, service.planCacheSize());
+ Mockito.verifyNoInteractions(queryPlannerSpy);
+ }
+
+ @Test
+ public void disabledCache() {
+ PrepareServiceImpl service = new PrepareServiceImpl("node", 0, ddlPlannerSpy, queryPlannerSpy);
+
+ service.start();
+
+ String query = "SELECT * FROM tbl WHERE id > 0";
+
+ assertThat(service.prepareAsync(query, queryCtx, createContext()), willBe(notNullValue()));
+ assertEquals(0, service.planCacheSize());
+ Mockito.verify(queryPlannerSpy, Mockito.times(1)).apply(Mockito.any(), Mockito.any());
+
+ assertThat(service.prepareAsync(query, queryCtx, createContext()), willBe(notNullValue()));
+ assertEquals(0, service.planCacheSize());
+ Mockito.verify(queryPlannerSpy, Mockito.times(2)).apply(Mockito.any(), Mockito.any());
+ }
+
+ @Test
+ public void normalizedQuery() {
+ SqlNode queryAst = parse("SELECT NULL");
+
+ String normalizedQuery = queryAst.toString();
+
+ assertThat(service.prepareAsync(normalizedQuery, queryCtx, createContext()), willBe(notNullValue()));
+ assertEquals(1, service.planCacheSize());
+ }
+
+ @Test
+ public void explainUsesCachedPlans() {
+ String query = "SELECT * FROM tbl WHERE id > 0";
+ String explainQuery = "EXPLAIN PLAN FOR SELECT * FROM tbl WHERE id > 0";
+ String explainQuery2 = "EXPLAIN PLAN FOR SELECT * /* comment */ FROM tbl WHERE id > 0";
+
+ // Ensure explain don't cache anything.
Review Comment:
```suggestion
// Ensure explain doesn't cache anything.
```
--
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 a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1245027243
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -296,4 +364,67 @@ private ResultSetMetadata resultSetMetadata(
}
);
}
+
+ private static SqlNode parse(String query, QueryContext queryContext, BaseQueryContext ctx) {
+ StatementParseResult parseResult = IgniteSqlParser.parse(query, StatementParseResult.MODE);
+
+ SqlNode sqlNode = parseResult.statement();
+
+ validateParsedStatement(queryContext, parseResult, sqlNode, ctx.parameters());
+
+ return sqlNode;
+ }
+
+ private static boolean skipCache(SqlNode sqlNode) {
+ SqlKind kind = sqlNode.getKind();
+
+ switch (kind) {
+ case SELECT:
+ case INSERT:
+ return false;
Review Comment:
There is an issue with DELETE statement https://issues.apache.org/jira/browse/IGNITE-19866.
Allow caching for the rest of DML queries.
--
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] xtern commented on a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1241091906
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -141,8 +164,59 @@ public void stop() throws Exception {
planningPool.shutdownNow();
}
+ /** Drop cached query plans. */
+ public void invalidateCachedPlans() {
+ planCache.clear();
+ }
+
+ @TestOnly
+ public void invalidateParserCache() {
+ normalizedQueryCache.clear();
+ }
+
+ @TestOnly
+ public int planCacheSize() {
+ return planCache.size();
+ }
+
+ @TestOnly
+ public int parseCacheSize() {
+ return normalizedQueryCache.size();
+ }
+
/** {@inheritDoc} */
@Override
+ public CompletableFuture<QueryPlan> prepareAsync(String query, QueryContext queryContext, BaseQueryContext ctx) {
+ CacheKey cacheKey = createCacheKey(query, ctx);
+
+ String normalizedQuery = normalizedQueryCache.get(cacheKey);
+
+ if (normalizedQuery == null) {
+ SqlNode sqlNode;
+ try {
+ sqlNode = parse(query, queryContext, ctx);
+ } catch (Exception ex) {
+ return failedFuture(ex);
+ }
+
+ if (skipCache(sqlNode)) {
+ return prepareAsync(sqlNode, ctx);
+ }
+
+ normalizedQueryCache.putIfAbsent(cacheKey, sqlNode.toString());
+
+ return planCache.computeIfAbsent(createCacheKey(sqlNode.toString(), ctx), k -> prepareAsync(sqlNode, ctx))
Review Comment:
It may make sense to save the result of calling `sqlNode.toString()` so that this method is not called twice.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/CacheKey.java:
##########
@@ -74,25 +66,25 @@ 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();
+ int result = Long.hashCode(catalogVersion);
+ result = 32 * result + schemaName.hashCode();
Review Comment:
```suggestion
result = 31 * result + schemaName.hashCode();
```
--
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 a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1247616907
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestNode.java:
##########
@@ -203,7 +200,7 @@ public QueryPlan prepare(String query) {
public QueryPlan prepare(SqlNode queryAst) {
assertThat(queryAst, not(instanceOf(SqlNodeList.class)));
- return await(prepareService.prepareAsync(queryAst, createContext()));
+ return await(((PrepareServiceImpl) prepareService).prepareAsync(queryAst, createContext()));
Review Comment:
Benchmarks need a way to prepare plan from AST, to avoid parsing stage from benchmark.
There are next ways to fix this:
* get prepare service from node (e.g. via 'unwrap' method), then cast to specific implementation inside the benchmark test.
* pull `prepareAsync` method into the interface.
* introduce the interface(s) with separate methods for parsing and optimization stages, introduce handler objects for these interface(s), and pass then to PrepareService constructor. Then either call `unwrap` method on prepare service to get these handlers.
* Rework benchmark to make it uses PrepareServiceImpl directly.
--
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] xtern commented on a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1245634852
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PrepareServiceSelfTest.java:
##########
@@ -0,0 +1,294 @@
+/*
+ * 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.planner;
+
+import static org.apache.ignite.internal.sql.engine.prepare.PrepareServiceImpl.validateParsedStatement;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureExceptionMatcher.willThrow;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static org.apache.ignite.internal.util.ArrayUtils.OBJECT_EMPTY_ARRAY;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Stream;
+import org.apache.calcite.runtime.CalciteContextException;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.ignite.internal.schema.NativeTypes;
+import org.apache.ignite.internal.sql.engine.QueryContext;
+import org.apache.ignite.internal.sql.engine.SqlQueryType;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders;
+import org.apache.ignite.internal.sql.engine.prepare.IgnitePlanner;
+import org.apache.ignite.internal.sql.engine.prepare.PlannerHelper;
+import org.apache.ignite.internal.sql.engine.prepare.PrepareServiceImpl;
+import org.apache.ignite.internal.sql.engine.prepare.QueryOptimizer;
+import org.apache.ignite.internal.sql.engine.prepare.ddl.DdlSqlToCommandConverter;
+import org.apache.ignite.internal.sql.engine.rel.IgniteRel;
+import org.apache.ignite.internal.sql.engine.sql.IgniteSqlParser;
+import org.apache.ignite.internal.sql.engine.sql.StatementParseResult;
+import org.apache.ignite.internal.sql.engine.trait.IgniteDistributions;
+import org.apache.ignite.internal.sql.engine.util.BaseQueryContext;
+import org.apache.ignite.sql.SqlException;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.mockito.Mockito;
+
+/**
+ * Prepare service self test.
+ */
+public class PrepareServiceSelfTest extends AbstractPlannerTest {
+ private final QueryContext queryCtx = QueryContext.create(SqlQueryType.ALL);
+ private PrepareServiceImpl service;
+
+ private QueryOptimizer queryPlannerSpy;
+ private DdlSqlToCommandConverter ddlPlannerSpy;
+
+ @BeforeEach
+ void setUp() {
+ queryPlannerSpy = Mockito.spy(new TestQueryOptimizer());
+ ddlPlannerSpy = Mockito.spy(new DdlSqlToCommandConverter(Map.of(), () -> "default"));
+
+ service = new PrepareServiceImpl(
+ "node",
+ 10,
+ ddlPlannerSpy,
+ queryPlannerSpy
+ );
+
+ service.start();
+ }
+
+ @AfterEach
+ void tearDown() throws Exception {
+ service.stop();
+ }
+
+ @SuppressWarnings("WeakerAccess")
+ static Stream<Arguments> queries() {
+ return Stream.of(
+ // Query
+ Arguments.of("SELECT * FROM tbl WHERE id > 0", OBJECT_EMPTY_ARRAY),
+ // Query with args
+ Arguments.of("SELECT * FROM tbl WHERE id > ?", new Object[]{1}),
+ // DML
+ Arguments.of("INSERT INTO tbl VALUES (1, '42')", OBJECT_EMPTY_ARRAY),
+ Arguments.of("UPDATE tbl SET VAL = '42' WHERE id = 1", OBJECT_EMPTY_ARRAY),
+ // TODO IGNITE-19866: uncomment DELETE statement
+ // Arguments.of("DELETE FROM tbl WHERE id = 1", OBJECT_EMPTY_ARRAY),
Review Comment:
This issue will not reproduce if you add the PK column to the test table.
--
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 #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1247496569
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestNode.java:
##########
@@ -203,7 +200,7 @@ public QueryPlan prepare(String query) {
public QueryPlan prepare(SqlNode queryAst) {
assertThat(queryAst, not(instanceOf(SqlNodeList.class)));
- return await(prepareService.prepareAsync(queryAst, createContext()));
+ return await(((PrepareServiceImpl) prepareService).prepareAsync(queryAst, createContext()));
Review Comment:
once again: TestNode _must_ (I mean even cast like this is no allowed) use interface, not implementation
--
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] xtern commented on a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1243296683
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareService.java:
##########
@@ -29,5 +29,5 @@ public interface PrepareService extends LifecycleAware {
/**
* Prepare query plan.
*/
- CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx);
+ CompletableFuture<QueryPlan> prepareAsync(String query, QueryContext queryContext, BaseQueryContext ctx);
Review Comment:
```suggestion
CompletableFuture<QueryPlan> prepareAsync(String query, QueryContext queryContext, BaseQueryContext ctx);
```
--
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] xtern commented on a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1243470496
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -296,4 +364,67 @@ private ResultSetMetadata resultSetMetadata(
}
);
}
+
+ private static SqlNode parse(String query, QueryContext queryContext, BaseQueryContext ctx) {
+ StatementParseResult parseResult = IgniteSqlParser.parse(query, StatementParseResult.MODE);
+
+ SqlNode sqlNode = parseResult.statement();
+
+ validateParsedStatement(queryContext, parseResult, sqlNode, ctx.parameters());
+
+ return sqlNode;
+ }
+
+ private static boolean skipCache(SqlNode sqlNode) {
+ SqlKind kind = sqlNode.getKind();
+
+ switch (kind) {
+ case SELECT:
+ case INSERT:
+ return false;
Review Comment:
I don't quite understand why we don't caching only 'INSERT' instead of caching all DML queries?
For example, I have the following results in `InsertBenchmark` if we changing `insert` query to something with UPDATE:
`update usertable set field3='a' where ycsb_key=?`
Patch not applied:
```
Benchmark (fsync) Mode Cnt Score Error Units
InsertBenchmark.sqlInsert false avgt 1.288 ms/op
```
Patch applied:
```
Benchmark (fsync) Mode Cnt Score Error Units
InsertBenchmark.sqlInsert false avgt 3.553 ms/op
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -296,4 +364,67 @@ private ResultSetMetadata resultSetMetadata(
}
);
}
+
+ private static SqlNode parse(String query, QueryContext queryContext, BaseQueryContext ctx) {
+ StatementParseResult parseResult = IgniteSqlParser.parse(query, StatementParseResult.MODE);
+
+ SqlNode sqlNode = parseResult.statement();
+
+ validateParsedStatement(queryContext, parseResult, sqlNode, ctx.parameters());
+
+ return sqlNode;
+ }
+
+ private static boolean skipCache(SqlNode sqlNode) {
+ SqlKind kind = sqlNode.getKind();
+
+ switch (kind) {
+ case SELECT:
+ case INSERT:
+ return false;
Review Comment:
I don't quite understand why we caching only 'INSERT' instead of caching all DML queries?
For example, I have the following results in `InsertBenchmark` if we changing `insert` query to something with UPDATE:
`update usertable set field3='a' where ycsb_key=?`
Patch not applied:
```
Benchmark (fsync) Mode Cnt Score Error Units
InsertBenchmark.sqlInsert false avgt 1.288 ms/op
```
Patch applied:
```
Benchmark (fsync) Mode Cnt Score Error Units
InsertBenchmark.sqlInsert false avgt 3.553 ms/op
```
--
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 #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1241659499
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -141,8 +164,59 @@ public void stop() throws Exception {
planningPool.shutdownNow();
}
+ /** Drop cached query plans. */
+ public void invalidateCachedPlans() {
+ planCache.clear();
+ }
+
+ @TestOnly
+ public void invalidateParserCache() {
+ normalizedQueryCache.clear();
+ }
+
+ @TestOnly
+ public int planCacheSize() {
+ return planCache.size();
+ }
+
+ @TestOnly
+ public int parseCacheSize() {
+ return normalizedQueryCache.size();
+ }
+
/** {@inheritDoc} */
@Override
+ public CompletableFuture<QueryPlan> prepareAsync(String query, QueryContext queryContext, BaseQueryContext ctx) {
+ CacheKey cacheKey = createCacheKey(query, ctx);
+
+ String normalizedQuery = normalizedQueryCache.get(cacheKey);
+
+ if (normalizedQuery == null) {
+ SqlNode sqlNode;
+ try {
+ sqlNode = parse(query, queryContext, ctx);
+ } catch (Exception ex) {
+ return failedFuture(ex);
+ }
+
+ if (skipCache(sqlNode)) {
+ return prepareAsync(sqlNode, ctx);
+ }
+
+ normalizedQueryCache.putIfAbsent(cacheKey, sqlNode.toString());
+
+ return planCache.computeIfAbsent(createCacheKey(sqlNode.toString(), ctx), k -> prepareAsync(sqlNode, ctx))
+ .thenApply(QueryPlan::copy);
+ }
+
+ return planCache.computeIfAbsent(createCacheKey(normalizedQuery, ctx), k ->
+ supplyAsync(() -> parse(query, queryContext, ctx)).thenCompose(sqlNode -> prepareAsync(sqlNode, ctx)))
Review Comment:
Here `supplyAsync` is called w/o an executor so it uses common ForkJoinPool.
--
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] xtern commented on a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1243470496
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -296,4 +364,67 @@ private ResultSetMetadata resultSetMetadata(
}
);
}
+
+ private static SqlNode parse(String query, QueryContext queryContext, BaseQueryContext ctx) {
+ StatementParseResult parseResult = IgniteSqlParser.parse(query, StatementParseResult.MODE);
+
+ SqlNode sqlNode = parseResult.statement();
+
+ validateParsedStatement(queryContext, parseResult, sqlNode, ctx.parameters());
+
+ return sqlNode;
+ }
+
+ private static boolean skipCache(SqlNode sqlNode) {
+ SqlKind kind = sqlNode.getKind();
+
+ switch (kind) {
+ case SELECT:
+ case INSERT:
+ return false;
Review Comment:
I don't quite understand why we caching only 'INSERT' instead of caching all DML queries?
For example, I have the following results in `InsertBenchmark` if we changing `insert` query to something with UPDATE:
`update usertable set field3='a' where ycsb_key=?`
This patch not applied:
```
Benchmark (fsync) Mode Cnt Score Error Units
InsertBenchmark.sqlInsert false avgt 1.288 ms/op
```
This patch applied:
```
Benchmark (fsync) Mode Cnt Score Error Units
InsertBenchmark.sqlInsert false avgt 3.553 ms/op
```
--
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 a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1245027243
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -296,4 +364,67 @@ private ResultSetMetadata resultSetMetadata(
}
);
}
+
+ private static SqlNode parse(String query, QueryContext queryContext, BaseQueryContext ctx) {
+ StatementParseResult parseResult = IgniteSqlParser.parse(query, StatementParseResult.MODE);
+
+ SqlNode sqlNode = parseResult.statement();
+
+ validateParsedStatement(queryContext, parseResult, sqlNode, ctx.parameters());
+
+ return sqlNode;
+ }
+
+ private static boolean skipCache(SqlNode sqlNode) {
+ SqlKind kind = sqlNode.getKind();
+
+ switch (kind) {
+ case SELECT:
+ case INSERT:
+ return false;
Review Comment:
There is an issue IGNITE-19866 related to DELETE statement.
Allow caching for the rest of DML queries.
--
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 a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1247616907
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestNode.java:
##########
@@ -203,7 +200,7 @@ public QueryPlan prepare(String query) {
public QueryPlan prepare(SqlNode queryAst) {
assertThat(queryAst, not(instanceOf(SqlNodeList.class)));
- return await(prepareService.prepareAsync(queryAst, createContext()));
+ return await(((PrepareServiceImpl) prepareService).prepareAsync(queryAst, createContext()));
Review Comment:
Benchmarks need a way to prepare plan from AST, to avoid parsing stage from benchmark.
There are next ways to fix this:
* get prepare service from node (e.g. via 'unwrap' method), then cast to specific implementation
* pull `prepareAsync` method into the interface
* introduce the interface(s) with separate methods for parsing and optimization stages, introduce handler objects for these interface(s), and pass then to PrepareService constructor. Then either call `unwrap` method on prepare service to get these handlers.
* Rework benchmark to make it uses PrepareServiceImpl directly.
--
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] xtern commented on a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1243470496
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -296,4 +364,67 @@ private ResultSetMetadata resultSetMetadata(
}
);
}
+
+ private static SqlNode parse(String query, QueryContext queryContext, BaseQueryContext ctx) {
+ StatementParseResult parseResult = IgniteSqlParser.parse(query, StatementParseResult.MODE);
+
+ SqlNode sqlNode = parseResult.statement();
+
+ validateParsedStatement(queryContext, parseResult, sqlNode, ctx.parameters());
+
+ return sqlNode;
+ }
+
+ private static boolean skipCache(SqlNode sqlNode) {
+ SqlKind kind = sqlNode.getKind();
+
+ switch (kind) {
+ case SELECT:
+ case INSERT:
+ return false;
Review Comment:
It seems to me that the problem with SqlNode#toString relates to DDL statements (not to DML), so I don't quite understand why we caching only 'INSERT' instead of caching all DML queries?
For example, I have the following results in `InsertBenchmark` if we changing `insert` query to something with UPDATE:
`update usertable set field3='a' where ycsb_key=?`
This patch not applied:
```
Benchmark (fsync) Mode Cnt Score Error Units
InsertBenchmark.sqlInsert false avgt 1.288 ms/op
```
This patch applied:
```
Benchmark (fsync) Mode Cnt Score Error Units
InsertBenchmark.sqlInsert false avgt 3.553 ms/op
```
--
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 a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1245027243
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -296,4 +364,67 @@ private ResultSetMetadata resultSetMetadata(
}
);
}
+
+ private static SqlNode parse(String query, QueryContext queryContext, BaseQueryContext ctx) {
+ StatementParseResult parseResult = IgniteSqlParser.parse(query, StatementParseResult.MODE);
+
+ SqlNode sqlNode = parseResult.statement();
+
+ validateParsedStatement(queryContext, parseResult, sqlNode, ctx.parameters());
+
+ return sqlNode;
+ }
+
+ private static boolean skipCache(SqlNode sqlNode) {
+ SqlKind kind = sqlNode.getKind();
+
+ switch (kind) {
+ case SELECT:
+ case INSERT:
+ return false;
Review Comment:
There is an issue [1] related to DELETE statement.
Allow caching for the rest of DML queries.
[1] https://issues.apache.org/jira/browse/IGNITE-19866
--
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 #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1244811505
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -72,10 +84,13 @@ public class PrepareServiceImpl implements PrepareService, SchemaUpdateListener
private final DdlSqlToCommandConverter ddlConverter;
- private final ConcurrentMap<CacheKey, CompletableFuture<QueryPlan>> cache;
+ private final ConcurrentMap<CacheKey, CompletableFuture<QueryPlan>> planCache;
+ private final ConcurrentMap<CacheKey, String> normalizedQueryCache;
Review Comment:
What is the reason to have normalizedQueryCache like mapping _CacheKey_ --> String ?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -72,10 +84,13 @@ public class PrepareServiceImpl implements PrepareService, SchemaUpdateListener
private final DdlSqlToCommandConverter ddlConverter;
- private final ConcurrentMap<CacheKey, CompletableFuture<QueryPlan>> cache;
+ private final ConcurrentMap<CacheKey, CompletableFuture<QueryPlan>> planCache;
+ private final ConcurrentMap<CacheKey, String> normalizedQueryCache;
private final String nodeName;
+ private final BiFunction<SqlNode, IgnitePlanner, IgniteRel> queryOptimizer;
Review Comment:
let's introduce interface instead of BiFunction
--
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 #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1246198785
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestNode.java:
##########
@@ -81,7 +80,7 @@
public class TestNode implements LifecycleAware {
private final String nodeName;
private final SchemaPlus schema;
- private final PrepareService prepareService;
+ private final PrepareServiceImpl prepareService;
Review Comment:
well, no. Now it's even worse: interface PrepareService has two similar method, but one of them uses cache while other doesn't. If you don't believe in 0-sized caffeine cache, then introduce an abstraction for cache and provide different implementations in tests and production configuration
--
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 a diff in pull request #2226: IGNITE-17765 Sql. Introduce cache for parsed statements
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2226:
URL: https://github.com/apache/ignite-3/pull/2226#discussion_r1246271834
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PrepareServiceSelfTest.java:
##########
@@ -0,0 +1,294 @@
+/*
+ * 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.planner;
+
+import static org.apache.ignite.internal.sql.engine.prepare.PrepareServiceImpl.validateParsedStatement;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureExceptionMatcher.willThrow;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static org.apache.ignite.internal.util.ArrayUtils.OBJECT_EMPTY_ARRAY;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Stream;
+import org.apache.calcite.runtime.CalciteContextException;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.ignite.internal.schema.NativeTypes;
+import org.apache.ignite.internal.sql.engine.QueryContext;
+import org.apache.ignite.internal.sql.engine.SqlQueryType;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders;
+import org.apache.ignite.internal.sql.engine.prepare.IgnitePlanner;
+import org.apache.ignite.internal.sql.engine.prepare.PlannerHelper;
+import org.apache.ignite.internal.sql.engine.prepare.PrepareServiceImpl;
+import org.apache.ignite.internal.sql.engine.prepare.QueryOptimizer;
+import org.apache.ignite.internal.sql.engine.prepare.ddl.DdlSqlToCommandConverter;
+import org.apache.ignite.internal.sql.engine.rel.IgniteRel;
+import org.apache.ignite.internal.sql.engine.sql.IgniteSqlParser;
+import org.apache.ignite.internal.sql.engine.sql.StatementParseResult;
+import org.apache.ignite.internal.sql.engine.trait.IgniteDistributions;
+import org.apache.ignite.internal.sql.engine.util.BaseQueryContext;
+import org.apache.ignite.sql.SqlException;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.mockito.Mockito;
+
+/**
+ * Prepare service self test.
+ */
+public class PrepareServiceSelfTest extends AbstractPlannerTest {
+ private final QueryContext queryCtx = QueryContext.create(SqlQueryType.ALL);
+ private PrepareServiceImpl service;
+
+ private QueryOptimizer queryPlannerSpy;
+ private DdlSqlToCommandConverter ddlPlannerSpy;
+
+ @BeforeEach
+ void setUp() {
+ queryPlannerSpy = Mockito.spy(new TestQueryOptimizer());
+ ddlPlannerSpy = Mockito.spy(new DdlSqlToCommandConverter(Map.of(), () -> "default"));
+
+ service = new PrepareServiceImpl(
+ "node",
+ 10,
+ ddlPlannerSpy,
+ queryPlannerSpy
+ );
+
+ service.start();
+ }
+
+ @AfterEach
+ void tearDown() throws Exception {
+ service.stop();
+ }
+
+ @SuppressWarnings("WeakerAccess")
+ static Stream<Arguments> queries() {
+ return Stream.of(
+ // Query
+ Arguments.of("SELECT * FROM tbl WHERE id > 0", OBJECT_EMPTY_ARRAY),
+ // Query with args
+ Arguments.of("SELECT * FROM tbl WHERE id > ?", new Object[]{1}),
+ // DML
+ Arguments.of("INSERT INTO tbl VALUES (1, '42')", OBJECT_EMPTY_ARRAY),
+ Arguments.of("UPDATE tbl SET VAL = '42' WHERE id = 1", OBJECT_EMPTY_ARRAY),
+ // TODO IGNITE-19866: uncomment DELETE statement
+ // Arguments.of("DELETE FROM tbl WHERE id = 1", OBJECT_EMPTY_ARRAY),
Review Comment:
I believe this should work without PK.
--
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