You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ignite.apache.org by ko...@apache.org on 2025/02/26 15:32:14 UTC
(ignite-3) branch main updated: IGNITE-24560 Sql. Result of script parsing with single statement is cached incorrectly (#5301)
This is an automated email from the ASF dual-hosted git repository.
korlov pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/ignite-3.git
The following commit(s) were added to refs/heads/main by this push:
new d38ff172596 IGNITE-24560 Sql. Result of script parsing with single statement is cached incorrectly (#5301)
d38ff172596 is described below
commit d38ff172596f24ddd4d25f85c88d2219fe8eaade
Author: korlov42 <ko...@apache.org>
AuthorDate: Wed Feb 26 17:32:09 2025 +0200
IGNITE-24560 Sql. Result of script parsing with single statement is cached incorrectly (#5301)
---
.../cli/commands/sql/ItSqlCommandTest.java | 21 ++++++
.../internal/sql/engine/sql/ParserServiceImpl.java | 83 +++++++++++++---------
.../sql/engine/sql/ParserServiceImplTest.java | 30 +++++++-
3 files changed, 99 insertions(+), 35 deletions(-)
diff --git a/modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/sql/ItSqlCommandTest.java b/modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/sql/ItSqlCommandTest.java
index 47a00872696..988facab0ea 100644
--- a/modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/sql/ItSqlCommandTest.java
+++ b/modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/sql/ItSqlCommandTest.java
@@ -52,6 +52,27 @@ class ItSqlCommandTest extends CliSqlCommandTestBase {
);
}
+ @Test
+ @DisplayName("Should execute sequence of statements without error")
+ void createSelectAlterSelect() {
+ String[] statements = {
+ "CREATE TABLE test(id INT PRIMARY KEY, val1 INT, val2 INT)",
+ "SELECT * FROM test",
+ "ALTER TABLE test DROP COLUMN val2",
+ "SELECT * FROM test",
+ };
+
+ for (String statement : statements) {
+ execute("sql", statement, "--jdbc-url", JDBC_URL);
+
+ assertAll(
+ this::assertExitCodeIsZero,
+ this::assertOutputIsNotEmpty,
+ this::assertErrOutputIsEmpty
+ );
+ }
+ }
+
@Test
@DisplayName("Should display readable error when wrong jdbc is given")
void wrongJdbcUrl() {
diff --git a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/sql/ParserServiceImpl.java b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/sql/ParserServiceImpl.java
index f806088007a..511d9ef5bb0 100644
--- a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/sql/ParserServiceImpl.java
+++ b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/sql/ParserServiceImpl.java
@@ -19,6 +19,7 @@ package org.apache.ignite.internal.sql.engine.sql;
import java.util.ArrayList;
import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Supplier;
import org.apache.calcite.sql.SqlNode;
@@ -48,6 +49,52 @@ public class ParserServiceImpl implements ParserService {
SqlNode parsedTree = parsedStatement.statement();
+ return prepareSingleResult(query, parsedTree, parsedStatement.dynamicParamsCount());
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public List<ParsedResult> parseScript(String query) {
+ ScriptParseResult parsedStatement = IgniteSqlParser.parse(query, ScriptParseResult.MODE);
+
+ if (parsedStatement.results().size() == 1) {
+ StatementParseResult parseResult = parsedStatement.results().get(0);
+
+ return List.of(
+ prepareSingleResult(query, parseResult.statement(), parseResult.dynamicParamsCount())
+ );
+ }
+
+ List<ParsedResult> results = new ArrayList<>(parsedStatement.results().size());
+
+ for (StatementParseResult result : parsedStatement.results()) {
+ SqlNode parsedTree = result.statement();
+ SqlQueryType queryType = Commons.getQueryType(parsedTree);
+ String normalizedQuery = parsedTree.toString();
+
+ assert queryType != null : normalizedQuery;
+
+ AtomicBoolean used = new AtomicBoolean();
+
+ results.add(new ParsedResultImpl(
+ queryType,
+ normalizedQuery,
+ normalizedQuery,
+ result.dynamicParamsCount(),
+ () -> {
+ if (!used.compareAndSet(false, true)) {
+ throw new IllegalStateException("Parsed result of script is not reusable.");
+ }
+
+ return parsedTree;
+ }
+ ));
+ }
+
+ return results;
+ }
+
+ private static ParsedResult prepareSingleResult(String originalQuery, SqlNode parsedTree, int dynamicParamsCount) {
SqlQueryType queryType = Commons.getQueryType(parsedTree);
SqlPrettyWriter w = new SqlPrettyWriter(NORMALIZED_SQL_WRITER_CONFIG);
@@ -58,12 +105,11 @@ public class ParserServiceImpl implements ParserService {
AtomicReference<SqlNode> holder = new AtomicReference<>(parsedTree);
- @SuppressWarnings("UnnecessaryLocalVariable")
- ParsedResult result = new ParsedResultImpl(
+ return new ParsedResultImpl(
queryType,
- query,
+ originalQuery,
normalizedQuery,
- parsedStatement.dynamicParamsCount(),
+ dynamicParamsCount,
() -> {
// Descendants of SqlNode class are mutable, thus we must use every
// syntax node only once to avoid problem. But we already parsed the
@@ -76,36 +122,9 @@ public class ParserServiceImpl implements ParserService {
return ast;
}
- return IgniteSqlParser.parse(query, StatementParseResult.MODE).statement();
+ return IgniteSqlParser.parse(originalQuery, StatementParseResult.MODE).statement();
}
);
-
- return result;
- }
-
- /** {@inheritDoc} */
- @Override
- public List<ParsedResult> parseScript(String query) {
- ScriptParseResult parsedStatement = IgniteSqlParser.parse(query, ScriptParseResult.MODE);
- List<ParsedResult> results = new ArrayList<>(parsedStatement.results().size());
-
- for (StatementParseResult result : parsedStatement.results()) {
- SqlNode parsedTree = result.statement();
- SqlQueryType queryType = Commons.getQueryType(parsedTree);
- String normalizedQuery = parsedTree.toString();
-
- assert queryType != null : normalizedQuery;
-
- results.add(new ParsedResultImpl(
- queryType,
- normalizedQuery,
- normalizedQuery,
- result.dynamicParamsCount(),
- () -> parsedTree
- ));
- }
-
- return results;
}
static class ParsedResultImpl implements ParsedResult {
diff --git a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/sql/ParserServiceImplTest.java b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/sql/ParserServiceImplTest.java
index 38f9e0bb4c7..3391ca2f9f2 100644
--- a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/sql/ParserServiceImplTest.java
+++ b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/sql/ParserServiceImplTest.java
@@ -18,6 +18,7 @@
package org.apache.ignite.internal.sql.engine.sql;
import static org.apache.ignite.internal.sql.engine.util.SqlTestUtils.assertThrowsSqlException;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
@@ -37,6 +38,7 @@ import org.junit.jupiter.params.provider.EnumSource;
/**
* Tests to verify {@link ParserServiceImpl}.
*/
+@SuppressWarnings("ThrowableNotThrown")
public class ParserServiceImplTest {
enum Statement {
QUERY("SELECT * FROM my_table", SqlQueryType.QUERY),
@@ -78,6 +80,20 @@ public class ParserServiceImplTest {
assertThat(firstCall.toString(), is(secondCall.toString()));
}
+ @ParameterizedTest
+ @EnumSource(Statement.class)
+ void scriptResultReturnedByServiceCreateNewInstanceOfTreeForSingleStatementQueries(Statement statement) {
+ ParserServiceImpl service = new ParserServiceImpl();
+
+ ParsedResult result = service.parseScript(statement.text).get(0);
+
+ SqlNode firstCall = result.parsedTree();
+ SqlNode secondCall = result.parsedTree();
+
+ assertNotSame(firstCall, secondCall);
+ assertThat(firstCall.toString(), is(secondCall.toString()));
+ }
+
/**
* Checks the parsing of a query containing multiple statements.
*
@@ -100,7 +116,6 @@ public class ParserServiceImplTest {
String multiStatementQuery = buf.toString();
- //noinspection ThrowableNotThrown
assertThrowsSqlException(
Sql.STMT_VALIDATION_ERR,
"Multiple statements are not allowed",
@@ -115,8 +130,17 @@ public class ParserServiceImplTest {
ParsedResult singleStatementResult = service.parse(statements.get(i).text);
assertThat(result.queryType(), equalTo(statements.get(i).type));
- assertThat(result.parsedTree(), notNullValue());
- assertThat(result.parsedTree().toString(), equalTo(singleStatementResult.parsedTree().toString()));
+
+ SqlNode parsedTree = result.parsedTree();
+
+ assertThrowsWithCause(
+ result::parsedTree,
+ IllegalStateException.class,
+ "Parsed result of script is not reusable"
+ );
+
+ assertThat(parsedTree, notNullValue());
+ assertThat(parsedTree.toString(), equalTo(singleStatementResult.parsedTree().toString()));
assertThat(result.normalizedQuery(), equalTo(singleStatementResult.normalizedQuery()));
assertThat(result.originalQuery(), equalTo(singleStatementResult.normalizedQuery()));
}