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 72f6d8f425 IGNITE-18422: Sql. Match number of dynamic parameters with
given parameters (#1528)
72f6d8f425 is described below
commit 72f6d8f4258d99beb4dd3b3fd1edb668dd0190e0
Author: Max Zhuravkov <[email protected]>
AuthorDate: Mon Jan 30 13:37:16 2023 +0400
IGNITE-18422: Sql. Match number of dynamic parameters with given parameters
(#1528)
Co-authored-by: Pavel Tupitsyn <[email protected]>
---
.../apache/ignite/jdbc/ItJdbcBatchSelfTest.java | 2 +-
.../dotnet/Apache.Ignite.Tests/Sql/SqlTests.cs | 6 +-
.../internal/sql/api/ItSqlAsynchronousApiTest.java | 2 +-
.../internal/sql/api/ItSqlSynchronousApiTest.java | 2 +-
.../sql/engine/ItDynamicParameterTest.java | 88 ++++++++++++++++++++++
.../internal/sql/engine/ItLimitOffsetTest.java | 14 ++--
.../internal/sql/engine/ItSecondaryIndexTest.java | 1 -
.../sql/engine/prepare/IgniteSqlValidator.java | 81 ++++++++++++++++----
.../sql/engine/prepare/PrepareServiceImpl.java | 9 ++-
.../internal/sql/engine/util/IgniteResource.java | 3 +
.../internal/sql/engine/planner/PlannerTest.java | 1 -
11 files changed, 177 insertions(+), 32 deletions(-)
diff --git
a/modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcBatchSelfTest.java
b/modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcBatchSelfTest.java
index 07304eab9d..54fa194843 100644
---
a/modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcBatchSelfTest.java
+++
b/modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcBatchSelfTest.java
@@ -158,7 +158,7 @@ public class ItJdbcBatchSelfTest extends
AbstractJdbcSelfTest {
assertEquals(0, updCnts.length, "Invalid update counts size");
- if (!e.getMessage().contains("Given statement type does not match
that declared by JDBC driver")) {
+ if (!e.getMessage().contains("Unexpected number of query
parameters. Provided 1 but there is only 0 dynamic parameter(s).")) {
log.error("Invalid exception: ", e);
fail();
diff --git a/modules/platforms/dotnet/Apache.Ignite.Tests/Sql/SqlTests.cs
b/modules/platforms/dotnet/Apache.Ignite.Tests/Sql/SqlTests.cs
index 60ee7a72b1..b6fd540032 100644
--- a/modules/platforms/dotnet/Apache.Ignite.Tests/Sql/SqlTests.cs
+++ b/modules/platforms/dotnet/Apache.Ignite.Tests/Sql/SqlTests.cs
@@ -56,7 +56,7 @@ namespace Apache.Ignite.Tests.Sql
[Test]
public async Task TestSimpleQuery()
{
- await using IResultSet<IIgniteTuple> resultSet = await
Client.Sql.ExecuteAsync(null, "select 1 as num, 'hello' as str", 1);
+ await using IResultSet<IIgniteTuple> resultSet = await
Client.Sql.ExecuteAsync(null, "select 1 as num, 'hello' as str");
var rows = await resultSet.ToListAsync();
Assert.AreEqual(-1, resultSet.AffectedRows);
@@ -203,7 +203,7 @@ namespace Apache.Ignite.Tests.Sql
public async Task TestMultipleEnumerationThrows()
{
// GetAll -> GetAsyncEnumerator.
- await using var resultSet = await Client.Sql.ExecuteAsync(null,
"SELECT 1", 1);
+ await using var resultSet = await Client.Sql.ExecuteAsync(null,
"SELECT 1");
await resultSet.ToListAsync();
var ex = Assert.Throws<IgniteClientException>(() =>
resultSet.GetAsyncEnumerator());
@@ -211,7 +211,7 @@ namespace Apache.Ignite.Tests.Sql
Assert.ThrowsAsync<IgniteClientException>(async () => await
resultSet.ToListAsync());
// GetAsyncEnumerator -> GetAll.
- await using var resultSet2 = await Client.Sql.ExecuteAsync(null,
"SELECT 1", 1);
+ await using var resultSet2 = await Client.Sql.ExecuteAsync(null,
"SELECT 1");
_ = resultSet2.GetAsyncEnumerator();
Assert.ThrowsAsync<IgniteClientException>(async () => await
resultSet2.ToListAsync());
diff --git
a/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlAsynchronousApiTest.java
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlAsynchronousApiTest.java
index 83cc3e872e..40b9ce51be 100644
---
a/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlAsynchronousApiTest.java
+++
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlAsynchronousApiTest.java
@@ -658,7 +658,7 @@ public class ItSqlAsynchronousApiTest extends
AbstractBasicIntegrationTest {
assertThrowsWithCause(
() -> ses.executeBatchAsync(null, "SELECT * FROM TEST",
args).get(),
SqlException.class,
- "Invalid SQL statement type in the batch"
+ "Unexpected number of query parameters. Provided 2 but there
is only 0 dynamic parameter(s)"
);
assertThrowsWithCause(
diff --git
a/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlSynchronousApiTest.java
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlSynchronousApiTest.java
index 308b2a6920..bd59046349 100644
---
a/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlSynchronousApiTest.java
+++
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItSqlSynchronousApiTest.java
@@ -308,7 +308,7 @@ public class ItSqlSynchronousApiTest extends
AbstractBasicIntegrationTest {
assertThrowsWithCause(
() -> ses.executeBatch(null, "SELECT * FROM TEST", args),
SqlException.class,
- "Invalid SQL statement type in the batch"
+ "Unexpected number of query parameters. Provided 2 but there
is only 0 dynamic parameter(s)"
);
assertThrowsWithCause(
diff --git
a/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDynamicParameterTest.java
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDynamicParameterTest.java
index f38f8fa6d1..d3771baf65 100644
---
a/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDynamicParameterTest.java
+++
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDynamicParameterTest.java
@@ -18,6 +18,9 @@
package org.apache.ignite.internal.sql.engine;
import static
org.apache.ignite.internal.sql.engine.util.SqlTypeUtils.toSqlType;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import java.math.BigDecimal;
import java.math.BigInteger;
@@ -30,8 +33,11 @@ import java.time.LocalTime;
import java.time.Period;
import java.util.UUID;
import java.util.concurrent.ThreadLocalRandom;
+import org.apache.calcite.runtime.CalciteContextException;
import org.apache.ignite.internal.sql.engine.util.MetadataMatcher;
import org.apache.ignite.sql.ColumnType;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
@@ -42,6 +48,16 @@ import org.junit.jupiter.params.provider.EnumSource.Mode;
public class ItDynamicParameterTest extends AbstractBasicIntegrationTest {
private static final ThreadLocalRandom RND = ThreadLocalRandom.current();
+ @BeforeEach
+ public void createTable() {
+ sql("CREATE TABLE t1 (id INTEGER PRIMARY KEY, val1 INTEGER NOT NULL,
val2 INTEGER)");
+ }
+
+ @AfterEach
+ public void dropTables() {
+ sql("DROP TABLE IF EXISTS t1");
+ }
+
@ParameterizedTest
@EnumSource(value = ColumnType.class,
// https://issues.apache.org/jira/browse/IGNITE-18258
@@ -118,6 +134,70 @@ public class ItDynamicParameterTest extends
AbstractBasicIntegrationTest {
assertQuery("SELECT COALESCE(?, ?)").withParams(12,
"b").returns(12).check();
}
+ @Test
+ public void testUnspecifiedDynamicParameterInExplain() {
+ assertUnexpectedNumberOfParameters("EXPLAIN PLAN FOR SELECT * FROM t1
WHERE id > ?");
+ }
+
+ @Test
+ public void testDynamicParametersInExplain() {
+ sql("EXPLAIN PLAN FOR SELECT * FROM t1 WHERE id > ?", 1);
+ }
+
+ @Test
+ public void testUnspecifiedDynamicParameterInSelectList() {
+ assertUnexpectedNumberOfParameters("SELECT COALESCE(?)");
+ assertUnexpectedNumberOfParameters("SELECT * FROM (VALUES(1, 2, ?))
t1");
+ }
+
+ @Test
+ public void testUnspecifiedDynamicParameterInInsert() {
+ assertUnexpectedNumberOfParameters("INSERT INTO t1 VALUES(1, 2, ?)");
+ }
+
+ @Test
+ public void testUnspecifiedDynamicParameterInUpdate() {
+ // column value
+ assertUnexpectedNumberOfParameters("UPDATE t1 SET val1=? WHERE id =
1");
+ // predicate
+ assertUnexpectedNumberOfParameters("UPDATE t1 SET val1=10 WHERE id =
?");
+ }
+
+ @Test
+ public void testUnspecifiedDynamicParameterInDelete() {
+ assertUnexpectedNumberOfParameters("DELETE FROM t1 WHERE id = ? AND
val1=1");
+ }
+
+ @Test
+ public void testUnexpectedNumberOfParametersInSelectList() {
+ assertUnexpectedNumberOfParameters("SELECT 1", 1);
+ assertUnexpectedNumberOfParameters("SELECT ?", 1, 2);
+ }
+
+ @Test
+ public void testUnexpectedNumberOfParametersInSelectInInsert() {
+ assertUnexpectedNumberOfParameters("INSERT INTO t1 VALUES(1, 2, 3)",
1);
+ assertUnexpectedNumberOfParameters("INSERT INTO t1 VALUES(1, 2, ?)",
1, 2);
+ }
+
+ @Test
+ public void testUnexpectedNumberOfParametersInDelete() {
+ assertUnexpectedNumberOfParameters("DELETE FROM t1 WHERE id = 1 AND
val1=1", 1);
+ assertUnexpectedNumberOfParameters("DELETE FROM t1 WHERE id = ? AND
val1=1", 1, 2);
+ }
+
+ @Test
+ public void testUnspecifiedDynamicParameterInLimitOffset() {
+ assertUnexpectedNumberOfParameters("SELECT * FROM t1 LIMIT 1", 1);
+ assertUnexpectedNumberOfParameters("SELECT * FROM t1 LIMIT ?", 1, 2);
+
+ assertUnexpectedNumberOfParameters("SELECT * FROM t1 LIMIT 1 OFFSET
1", 1);
+ assertUnexpectedNumberOfParameters("SELECT * FROM t1 LIMIT 1 OFFSET
?", 1, 2);
+
+ assertUnexpectedNumberOfParameters("SELECT * FROM t1 OFFSET 1", 1);
+ assertUnexpectedNumberOfParameters("SELECT * FROM t1 OFFSET ?", 1, 2);
+ }
+
private Object generateValueByType(int i, ColumnType type) {
switch (type) {
case BOOLEAN:
@@ -167,4 +247,12 @@ public class ItDynamicParameterTest extends
AbstractBasicIntegrationTest {
throw new IllegalArgumentException("unsupported type " + type);
}
}
+
+ private static void assertUnexpectedNumberOfParameters(String query,
Object... params) {
+ CalciteContextException err =
assertThrows(CalciteContextException.class, () -> {
+ assertQuery(query).withParams(params).check();
+ }, "query: " + query);
+
+ assertThat("query: " + query, err.getMessage(),
containsString("Unexpected number of query parameters"));
+ }
}
diff --git
a/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItLimitOffsetTest.java
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItLimitOffsetTest.java
index 0867f61114..19a2f7a655 100644
---
a/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItLimitOffsetTest.java
+++
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItLimitOffsetTest.java
@@ -145,12 +145,14 @@ public class ItLimitOffsetTest extends
AbstractBasicIntegrationTest {
String request = createSql(lim, off, param, sorted);
Object[] params = null;
- if (lim >= 0 && off >= 0) {
- params = new Object[]{off, lim};
- } else if (lim >= 0) {
- params = new Object[]{lim};
- } else if (off >= 0) {
- params = new Object[]{off};
+ if (param) {
+ if (lim >= 0 && off >= 0) {
+ params = new Object[]{off, lim};
+ } else if (lim >= 0) {
+ params = new Object[]{lim};
+ } else if (off >= 0) {
+ params = new Object[]{off};
+ }
}
log.info("SQL: " + request + (param ? "params=" +
Arrays.toString(params) : ""));
diff --git
a/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItSecondaryIndexTest.java
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItSecondaryIndexTest.java
index 320b91e786..880f1e1cbf 100644
---
a/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItSecondaryIndexTest.java
+++
b/modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItSecondaryIndexTest.java
@@ -261,7 +261,6 @@ public class ItSecondaryIndexTest extends
AbstractBasicIntegrationTest {
@Test
public void testIndexedFieldGreaterThanFilter() {
assertQuery("SELECT * FROM Developer WHERE depId>21")
- .withParams(3)
.matches(containsIndexScan("PUBLIC", "DEVELOPER", DEPID_IDX))
.returns(23, "Musorgskii", 22, "", -1)
.check();
diff --git
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java
index 13eed68884..20468cdc86 100644
---
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java
+++
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java
@@ -18,7 +18,6 @@
package org.apache.ignite.internal.sql.engine.prepare;
import static org.apache.calcite.util.Static.RESOURCE;
-import static org.apache.ignite.internal.util.ArrayUtils.nullOrEmpty;
import java.math.BigDecimal;
import java.util.Arrays;
@@ -38,6 +37,7 @@ import org.apache.calcite.sql.SqlCall;
import org.apache.calcite.sql.SqlCallBinding;
import org.apache.calcite.sql.SqlDelete;
import org.apache.calcite.sql.SqlDynamicParam;
+import org.apache.calcite.sql.SqlExplain;
import org.apache.calcite.sql.SqlIdentifier;
import org.apache.calcite.sql.SqlInsert;
import org.apache.calcite.sql.SqlJoin;
@@ -96,7 +96,9 @@ public class IgniteSqlValidator extends SqlValidatorImpl {
}
/** Dynamic parameters. */
- Object[] parameters;
+ private final Object[] parameters;
+
+ private int dynamicParameterCount;
/**
* Creates a validator.
@@ -114,6 +116,40 @@ public class IgniteSqlValidator extends SqlValidatorImpl {
this.parameters = parameters;
}
+ /** {@inheritDoc} */
+ @Override
+ public SqlNode validate(SqlNode topNode) {
+ this.dynamicParameterCount = 0;
+ try {
+ SqlNode topNodeToValidate;
+ // Calcite fails to validate a query when its top node is EXPLAIN
PLAN FOR
+ // java.lang.NullPointerException: namespace for <query>
+ // at
org.apache.calcite.sql.validate.SqlValidatorImpl.getNamespaceOrThrow(SqlValidatorImpl.java:1280)
+ boolean explain = topNode instanceof SqlExplain;
+ if (explain) {
+ SqlExplain explainNode = (SqlExplain) topNode;
+ topNodeToValidate = explainNode.getExplicandum();
+ } else {
+ topNodeToValidate = topNode;
+ }
+
+ SqlNode validatedNode = super.validate(topNodeToValidate);
+ if (parameters.length != dynamicParameterCount) {
+ throw newValidationError(topNode,
IgniteResource.INSTANCE.unexpectedParameter(parameters.length,
dynamicParameterCount));
+ }
+
+ if (explain) {
+ SqlExplain explainNode = (SqlExplain) topNode;
+ explainNode.setOperand(0, validatedNode);
+ return explainNode;
+ } else {
+ return validatedNode;
+ }
+ } finally {
+ this.dynamicParameterCount = 0;
+ }
+ }
+
/** {@inheritDoc} */
@Override
public void validateInsert(SqlInsert insert) {
@@ -230,12 +266,10 @@ public class IgniteSqlValidator extends SqlValidatorImpl {
throw newValidationError(n,
IgniteResource.INSTANCE.correctIntegerLimit(nodeName));
}
} else if (n instanceof SqlDynamicParam) {
- // will fail in params check.
- if (nullOrEmpty(parameters)) {
- return;
- }
-
- int idx = ((SqlDynamicParam) n).getIndex();
+ SqlDynamicParam dynamicParam = (SqlDynamicParam) n;
+ int idx = dynamicParam.getIndex();
+ // validateDynamicParam is not called by a validator we must call
it ourselves.
+ validateDynamicParam(dynamicParam);
if (idx < parameters.length) {
Object param = parameters[idx];
@@ -456,18 +490,37 @@ public class IgniteSqlValidator extends SqlValidatorImpl {
return Commons.implicitPkEnabled() &&
Commons.IMPLICIT_PK_COL_NAME.equals(alias);
}
+ /** {@inheritDoc} */
+ @Override
+ public void validateDynamicParam(SqlDynamicParam dynamicParam) {
+ this.dynamicParameterCount = Integer.max(dynamicParameterCount,
dynamicParam.getIndex() + 1);
+ }
+
/** {@inheritDoc} */
@Override
protected void inferUnknownTypes(RelDataType inferredType,
SqlValidatorScope scope, SqlNode node) {
+
if (node instanceof SqlDynamicParam &&
inferredType.equals(unknownType)) {
- Object param = parameters[((SqlDynamicParam) node).getIndex()];
- RelDataType type;
- if (param == null) {
- type = typeFactory().createSqlType(SqlTypeName.NULL);
+ SqlDynamicParam dynamicParam = (SqlDynamicParam) node;
+
+ // We explicitly call validateDynamicParam because the validator
does not call it.
+ validateDynamicParam(dynamicParam);
+
+ if (dynamicParam.getIndex() >= parameters.length) {
+ RelDataType type =
typeFactory().createSqlType(SqlTypeName.NULL);
+ setValidatedNodeType(node, type);
} else {
- type =
typeFactory().toSql(typeFactory().createType(param.getClass()));
+ RelDataType type;
+ // Parameter's index is always valid because otherwise
validateDynamicParam throws an exception.
+ Object param = parameters[dynamicParam.getIndex()];
+ if (param == null) {
+ type = typeFactory().createSqlType(SqlTypeName.NULL);
+ } else {
+ type =
typeFactory().toSql(typeFactory().createType(param.getClass()));
+ }
+
+ setValidatedNodeType(node, type);
}
- setValidatedNodeType(node, type);
} else if (node instanceof SqlCall) {
SqlValidatorScope newScope = scopes.get(node);
diff --git
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java
index d07c5590e3..4611036835 100644
---
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java
+++
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java
@@ -199,13 +199,14 @@ public class PrepareServiceImpl implements
PrepareService, SchemaUpdateListener
return CompletableFuture.supplyAsync(() -> {
IgnitePlanner planner = ctx.planner();
- SqlNode sql = ((SqlExplain) explain).getExplicandum();
-
// Validate
- sql = planner.validate(sql);
+ // We extract query subtree inside the validator.
+ SqlNode explainNode = planner.validate(explain);
+ // Extract validated query.
+ SqlNode validNode = ((SqlExplain) explainNode).getExplicandum();
// Convert to Relational operators graph
- IgniteRel igniteRel = optimize(sql, planner);
+ IgniteRel igniteRel = optimize(validNode, planner);
String plan = RelOptUtil.toString(igniteRel,
SqlExplainLevel.ALL_ATTRIBUTES);
diff --git
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteResource.java
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteResource.java
index dc1c263b05..024c872e2a 100644
---
a/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteResource.java
+++
b/modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteResource.java
@@ -37,4 +37,7 @@ public interface IgniteResource {
@Resources.BaseMessage("Illegal value of {0}. The value must be positive
and less than Integer.MAX_VALUE (" + Integer.MAX_VALUE + ").")
Resources.ExInst<SqlValidatorException> correctIntegerLimit(String a0);
+
+ @Resources.BaseMessage("Unexpected number of query parameters. Provided
{0} but there is only {1} dynamic parameter(s).")
+ Resources.ExInst<SqlValidatorException> unexpectedParameter(int provided,
int expected);
}
diff --git
a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PlannerTest.java
b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PlannerTest.java
index b40172b112..61c2a4bd28 100644
---
a/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PlannerTest.java
+++
b/modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PlannerTest.java
@@ -161,7 +161,6 @@ public class PlannerTest extends AbstractPlannerTest {
PlanningContext ctx = PlanningContext.builder()
.parentContext(BaseQueryContext.builder()
.logger(log)
- .parameters(2)
.frameworkConfig(newConfigBuilder(FRAMEWORK_CONFIG)
.defaultSchema(schema)
.build())
