You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ygerzhedovich (via GitHub)" <gi...@apache.org> on 2023/06/19 12:53:08 UTC
[GitHub] [ignite-3] ygerzhedovich opened a new pull request, #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
ygerzhedovich opened a new pull request, #2214:
URL: https://github.com/apache/ignite-3/pull/2214
(no comment)
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1240197198
##########
modules/jdbc/src/test/java/org/apache/ignite/internal/jdbc/PreparedStatementParamsTest.java:
##########
@@ -61,25 +62,28 @@
public class PreparedStatementParamsTest {
/** Values for supported JDBC types. */
- private static final Map<JDBCType, Object> SUPPORTED_TYPES = Map.ofEntries(
- Map.entry(JDBCType.BOOLEAN, true),
- Map.entry(JDBCType.TINYINT, (byte) 1),
- Map.entry(JDBCType.SMALLINT, (short) 1),
- Map.entry(JDBCType.INTEGER, 1),
- Map.entry(JDBCType.BIGINT, 1L),
- Map.entry(JDBCType.FLOAT, 1.0f),
- Map.entry(JDBCType.REAL, 1.0f),
- Map.entry(JDBCType.DOUBLE, 1.0d),
- Map.entry(JDBCType.DECIMAL, new BigDecimal("123")),
- Map.entry(JDBCType.CHAR, "123"),
- Map.entry(JDBCType.VARCHAR, "123"),
- Map.entry(JDBCType.BINARY, new byte[]{1, 2, 3}),
- Map.entry(JDBCType.VARBINARY, new byte[]{1, 2, 3}),
- Map.entry(JDBCType.DATE, new Date(1)),
- Map.entry(JDBCType.TIME, Time.valueOf(LocalTime.NOON)),
- Map.entry(JDBCType.TIMESTAMP, Timestamp.valueOf("2000-01-01 00:00:00.000")),
- Map.entry(JDBCType.OTHER, new UUID(1, 1))
- );
+ private static final Map<JDBCType, Object> SUPPORTED_TYPES = new HashMap();
+
+ static {
+ SUPPORTED_TYPES.put(JDBCType.BOOLEAN, true);
+ SUPPORTED_TYPES.put(JDBCType.TINYINT, (byte) 1);
+ SUPPORTED_TYPES.put(JDBCType.SMALLINT, (short) 1);
+ SUPPORTED_TYPES.put(JDBCType.INTEGER, 1);
+ SUPPORTED_TYPES.put(JDBCType.BIGINT, 1L);
+ SUPPORTED_TYPES.put(JDBCType.FLOAT, 1.0f);
+ SUPPORTED_TYPES.put(JDBCType.REAL, 1.0f);
+ SUPPORTED_TYPES.put(JDBCType.DOUBLE, 1.0d);
+ SUPPORTED_TYPES.put(JDBCType.DECIMAL, new BigDecimal("123"));
+ SUPPORTED_TYPES.put(JDBCType.CHAR, "123");
+ SUPPORTED_TYPES.put(JDBCType.VARCHAR, "123");
+ SUPPORTED_TYPES.put(JDBCType.BINARY, new byte[]{1, 2, 3});
+ SUPPORTED_TYPES.put(JDBCType.VARBINARY, new byte[]{1, 2, 3});
+ SUPPORTED_TYPES.put(JDBCType.DATE, new Date(1));
+ SUPPORTED_TYPES.put(JDBCType.TIME, Time.valueOf(LocalTime.NOON));
+ SUPPORTED_TYPES.put(JDBCType.TIMESTAMP, Timestamp.valueOf("2000-01-01 00:00:00.000"));
+ SUPPORTED_TYPES.put(JDBCType.OTHER, new UUID(1, 1));
+ SUPPORTED_TYPES.put(JDBCType.NULL, null);
Review Comment:
Maybe I am missing something, but what's wrong with Map.ofEntries+Map.entry ?
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1239638368
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/api/SessionBuilderImpl.java:
##########
@@ -133,19 +135,20 @@ public SessionBuilder property(String name, @Nullable Object value) {
/** {@inheritDoc} */
@Override
public Session build() {
- PropertiesHolder propsHolder = PropertiesHelper.newBuilder()
+ Builder propBuilder = PropertiesHelper
+ .newBuilder(props)
Review Comment:
Properties are copied twice: here and in constructor.
Maybe propBuilder can be instantiated in constructor?
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1260977665
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PlannerTimeoutTest.java:
##########
@@ -33,22 +35,53 @@
import org.apache.calcite.rel.type.RelDataType;
import org.apache.calcite.rel.type.RelDataTypeFactory;
import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.ignite.internal.schema.NativeTypes;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders.ConfigurationParameter;
+import org.apache.ignite.internal.sql.engine.framework.TestCluster;
+import org.apache.ignite.internal.sql.engine.framework.TestNode;
import org.apache.ignite.internal.sql.engine.prepare.IgnitePlanner;
import org.apache.ignite.internal.sql.engine.prepare.PlanningContext;
import org.apache.ignite.internal.sql.engine.rel.IgniteConvention;
import org.apache.ignite.internal.sql.engine.rel.IgniteRel;
import org.apache.ignite.internal.sql.engine.schema.IgniteSchema;
+import org.apache.ignite.internal.sql.engine.trait.IgniteDistributions;
+import org.apache.ignite.internal.sql.engine.util.SqlTestUtils;
import org.jetbrains.annotations.NotNull;
import org.junit.jupiter.api.Test;
/**
* Test planner timeout.
*/
public class PlannerTimeoutTest extends AbstractPlannerTest {
- private static final long PLANNER_TIMEOUT = 500;
+
+ /** Check correctness of planning timeout. */
+ @Test
+ public void testPlanningTimeout() {
+ TestCluster cluster = TestBuilders.cluster()
+ .nodes("N1")
+ .addTable()
+ .name("TST1")
+ .distribution(IgniteDistributions.hash(List.of(0)))
+ .addColumn("ID", NativeTypes.INT32)
+ .addColumn("VAL", NativeTypes.stringOf(64))
+ .end()
+ .addConfiguration(ConfigurationParameter.PLANNING_TIMEOUT, 1L)
+ .build();
+
+ cluster.start();
+
+ TestNode gatewayNode = cluster.node("N1");
+
+ SqlTestUtils.assertThrowsSqlException(PLANNING_TIMEOUTED_ERR,
+ () -> gatewayNode.prepare("SELECT * FROM TST1 t, TST1 t1, TST1 t2"));
+
+ }
@Test
public void testLongPlanningTimeout() throws Exception {
+ final long PLANNER_TIMEOUT = 500;
Review Comment:
```suggestion
final long plannerTimeout = 500;
```
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261191665
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgnitePlanner.java:
##########
@@ -560,6 +564,11 @@ public void checkCancel() {
long startTs = ctx.startTs();
if (FastTimestamps.coarseCurrentTimeMillis() - startTs > timeout) {
+ LOG.info("Planning of a query aborted due to planner timeout threshold is reached [timeout={}, query={}]",
Review Comment:
> I think user should have ability be aware about the first case too
sounds like we need to expose metric for this
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1239634731
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/property/PropertiesHelper.java:
##########
@@ -42,6 +43,23 @@ public static Builder newBuilder() {
return new BuilderImpl();
}
+ /**
+ * Creates new builder and populates it with properties from the given Map. Keys of the map are names of properties, values are values.
+ *
+ * @param props A holder to populate new builder with.
+ * @return A builder containing properties from given holder.
+ */
+ public static Builder newBuilder(Map<String, Object> props) {
+ Builder builder = newBuilder();
+
+ for (Entry<String, Object> p : props.entrySet()) {
+ Property property = new Property(p.getKey(), p.getValue().getClass());
+ builder.set(property, p.getValue());
+ }
Review Comment:
I don't like the idea to return partially constructed object.
I'd prefer to have one or both methods:
`static Properties fromMap(Map<> props)`
or in Builder class `Builder copyFrom(Map<> props)`
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1259764008
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/api/SessionBuilderImpl.java:
##########
@@ -136,16 +136,14 @@ public Session build() {
PropertiesHolder propsHolder = PropertiesHelper.newBuilder()
.set(SessionProperty.IDLE_TIMEOUT, sessionTimeout)
.set(QueryProperty.QUERY_TIMEOUT, queryTimeout)
- .set(QueryProperty.DEFAULT_SCHEMA, schema)
- .build();
+ .set(QueryProperty.DEFAULT_SCHEMA, schema).build();
Review Comment:
```suggestion
.set(QueryProperty.DEFAULT_SCHEMA, schema)
.build();
```
--
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] ygerzhedovich commented on a diff in pull request #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1239797101
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDmlTest.java:
##########
@@ -384,12 +375,10 @@ public void testMergeKeysConflict() {
sql("CREATE TABLE test2 (k int PRIMARY KEY, a int, b int)");
- IgniteException ex = assertThrows(IgniteException.class, () -> sql(
- "MERGE INTO test2 USING test1 ON test1.a = test2.a "
- + "WHEN MATCHED THEN UPDATE SET b = test1.b + 1 "
- + "WHEN NOT MATCHED THEN INSERT (k, a, b) VALUES (0, a, b)"));
-
- assertEquals(Sql.DUPLICATE_KEYS_ERR, ex.code());
+ assertSqlExceptionThrows(Sql.DUPLICATE_KEYS_ERR, () -> sql(
Review Comment:
agree
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDmlTest.java:
##########
@@ -75,9 +74,7 @@ public void pkConstraintConsistencyTest() {
.check();
{
Review Comment:
sure
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/property/PropertiesHelper.java:
##########
@@ -42,6 +43,23 @@ public static Builder newBuilder() {
return new BuilderImpl();
}
+ /**
+ * Creates new builder and populates it with properties from the given Map. Keys of the map are names of properties, values are values.
+ *
+ * @param props A holder to populate new builder with.
+ * @return A builder containing properties from given holder.
+ */
+ public static Builder newBuilder(Map<String, Object> props) {
+ Builder builder = newBuilder();
+
+ for (Entry<String, Object> p : props.entrySet()) {
+ Property property = new Property(p.getKey(), p.getValue().getClass());
+ builder.set(property, p.getValue());
+ }
Review Comment:
removed
##########
modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/JdbcPreparedStatement.java:
##########
@@ -83,6 +83,7 @@ public class JdbcPreparedStatement extends JdbcStatement implements PreparedStat
JDBCType.VARCHAR,
JDBCType.BINARY,
JDBCType.VARBINARY,
+ JDBCType.NULL,
Review Comment:
the change is not related to the task, but so small and I decided to include into the patch.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PlanningContext.java:
##########
@@ -39,23 +39,31 @@
* Planning context.
*/
public final class PlanningContext implements Context {
+ /** Parent context. */
Review Comment:
yep
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareService.java:
##########
@@ -28,6 +28,12 @@
public interface PrepareService extends LifecycleAware {
/**
* Prepare query plan.
+ *
+ * @param sqlNode AST of a query which need to be planned.
+ * @param ctx Query context.
+ * @param plannerTimeout Timeout in milliseconds to planning.
+ *
+ * @return Future that contains prepared query plan when completes.
*/
- CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx);
+ CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx, long plannerTimeout);
Review Comment:
because it related only to planning phase )
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/sql/util/SqlTestUtils.java:
##########
@@ -32,13 +35,31 @@
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.ignite.internal.sql.engine.type.UuidType;
import org.apache.ignite.sql.ColumnType;
+import org.apache.ignite.sql.SqlException;
+import org.junit.jupiter.api.function.Executable;
/**
* Test utils for SQL.
*/
public class SqlTestUtils {
private static final ThreadLocalRandom RND = ThreadLocalRandom.current();
+
+ /**
+ * <em>Assert</em> that execution of the supplied {@code executable} throws
+ * an {@link SqlException} with expected error code and return the exception.
+ *
+ * @param expectedCode Expected error code of {@link SqlException}
+ * @param executable supplier to execute and check thrown exception.
Review Comment:
thanks
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -143,30 +148,45 @@ public void stop() throws Exception {
/** {@inheritDoc} */
@Override
- public CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx) {
- try {
- assert single(sqlNode);
+ public CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx, long plannerTimeout) {
+ CompletableFuture<QueryPlan> result;
+
+ assert single(sqlNode);
+
+ SqlQueryType queryType = Commons.getQueryType(sqlNode);
+ assert queryType != null : "No query type for query: " + sqlNode;
+
+ PlanningContext planningContext = PlanningContext.builder()
+ .parentContext(ctx)
+ .query(ctx.query())
+ .plannerTimeout(plannerTimeout)
+ .build();
- SqlQueryType queryType = Commons.getQueryType(sqlNode);
- assert queryType != null : "No query type for query: " + sqlNode;
+ result = prepareAsync0(sqlNode, queryType, planningContext);
- PlanningContext planningContext = PlanningContext.builder()
- .parentContext(ctx)
- .build();
+ return result.exceptionally(ex -> {
+ Throwable th = ExceptionUtils.unwrapCause(ex);
+ if (planningContext.timeouted() && th instanceof RelOptPlanner.CannotPlanException) {
+ LOG.info("Query plan is absent due to planing timeout is reached [query={}]", ctx.query());
Review Comment:
no, we will have the second line only for case when plan can't be executed. It's should be rare case and signal that system has incorrect settings.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgnitePlanner.java:
##########
@@ -560,6 +564,11 @@ public void checkCancel() {
long startTs = ctx.startTs();
if (FastTimestamps.coarseCurrentTimeMillis() - startTs > timeout) {
+ LOG.info("Planning of a query aborted due to planner timeout threshold is reached [timeout={}, query={}]",
Review Comment:
I had some doubts about the same. But I don't see right now proper way to inform user about the situation
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/BaseQueryContext.java:
##########
@@ -148,35 +148,35 @@ public List<MetadataHandler<?>> handlers(Class<? extends MetadataHandler<?>> hnd
private final QueryCancel cancel;
+ private final String query;
Review Comment:
Why is it bad? Right now we have query text for PlanningContext and I suppose that BaseQueryContext will be getted rid a little bit later when we make context refactoring
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -143,30 +148,45 @@ public void stop() throws Exception {
/** {@inheritDoc} */
@Override
- public CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx) {
- try {
- assert single(sqlNode);
+ public CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx, long plannerTimeout) {
+ CompletableFuture<QueryPlan> result;
+
+ assert single(sqlNode);
+
+ SqlQueryType queryType = Commons.getQueryType(sqlNode);
+ assert queryType != null : "No query type for query: " + sqlNode;
+
+ PlanningContext planningContext = PlanningContext.builder()
+ .parentContext(ctx)
+ .query(ctx.query())
+ .plannerTimeout(plannerTimeout)
+ .build();
- SqlQueryType queryType = Commons.getQueryType(sqlNode);
- assert queryType != null : "No query type for query: " + sqlNode;
+ result = prepareAsync0(sqlNode, queryType, planningContext);
- PlanningContext planningContext = PlanningContext.builder()
- .parentContext(ctx)
- .build();
+ return result.exceptionally(ex -> {
+ Throwable th = ExceptionUtils.unwrapCause(ex);
+ if (planningContext.timeouted() && th instanceof RelOptPlanner.CannotPlanException) {
+ LOG.info("Query plan is absent due to planing timeout is reached [query={}]", ctx.query());
+ throw new SqlException(ErrorGroups.Sql.PLANNING_TIMEOUTED_ERR);
+ }
+
+ throw new IgniteException(th);
+ }
Review Comment:
yep
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/property/PropertiesHelper.java:
##########
@@ -42,6 +43,23 @@ public static Builder newBuilder() {
return new BuilderImpl();
}
+ /**
+ * Creates new builder and populates it with properties from the given Map. Keys of the map are names of properties, values are values.
+ *
+ * @param props A holder to populate new builder with.
+ * @return A builder containing properties from given holder.
+ */
+ public static Builder newBuilder(Map<String, Object> props) {
Review Comment:
removed
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -143,30 +148,45 @@ public void stop() throws Exception {
/** {@inheritDoc} */
@Override
- public CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx) {
- try {
- assert single(sqlNode);
+ public CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx, long plannerTimeout) {
+ CompletableFuture<QueryPlan> result;
+
+ assert single(sqlNode);
+
+ SqlQueryType queryType = Commons.getQueryType(sqlNode);
+ assert queryType != null : "No query type for query: " + sqlNode;
+
+ PlanningContext planningContext = PlanningContext.builder()
+ .parentContext(ctx)
+ .query(ctx.query())
+ .plannerTimeout(plannerTimeout)
+ .build();
- SqlQueryType queryType = Commons.getQueryType(sqlNode);
- assert queryType != null : "No query type for query: " + sqlNode;
+ result = prepareAsync0(sqlNode, queryType, planningContext);
- PlanningContext planningContext = PlanningContext.builder()
- .parentContext(ctx)
- .build();
+ return result.exceptionally(ex -> {
+ Throwable th = ExceptionUtils.unwrapCause(ex);
+ if (planningContext.timeouted() && th instanceof RelOptPlanner.CannotPlanException) {
+ LOG.info("Query plan is absent due to planing timeout is reached [query={}]", ctx.query());
+ throw new SqlException(ErrorGroups.Sql.PLANNING_TIMEOUTED_ERR);
+ }
+
+ throw new IgniteException(th);
+ }
Review Comment:
yep
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItCommonApiTest.java:
##########
@@ -120,6 +123,21 @@ public void testSessionExpiration() throws Exception {
ses2.execute(null, "SELECT 2 + 2").close();
}
+ /** Check correctness of planning timeout. */
+ @Test
+ public void testPlanningTimeout() {
+ IgniteSql sql = igniteSql();
+
+ sql("CREATE TABLE TST1(id INTEGER PRIMARY KEY, val INTEGER)");
+
+ Session ses = sql.sessionBuilder().property(PLANNING_TIMEOUT.name, 1L).build();
+
+ SqlTestUtils.assertSqlExceptionThrows(PLANNING_TIMEOUTED_ERR,
+ () -> ses.execute(null, "SELECT * FROM TST1 t, TST1 t1, TST1 t2"));
+
+ ses.close();
Review Comment:
we have red test and session will close in some time
##########
modules/jdbc/src/test/java/org/apache/ignite/internal/jdbc/PreparedStatementParamsTest.java:
##########
@@ -61,25 +62,28 @@
public class PreparedStatementParamsTest {
/** Values for supported JDBC types. */
- private static final Map<JDBCType, Object> SUPPORTED_TYPES = Map.ofEntries(
- Map.entry(JDBCType.BOOLEAN, true),
- Map.entry(JDBCType.TINYINT, (byte) 1),
- Map.entry(JDBCType.SMALLINT, (short) 1),
- Map.entry(JDBCType.INTEGER, 1),
- Map.entry(JDBCType.BIGINT, 1L),
- Map.entry(JDBCType.FLOAT, 1.0f),
- Map.entry(JDBCType.REAL, 1.0f),
- Map.entry(JDBCType.DOUBLE, 1.0d),
- Map.entry(JDBCType.DECIMAL, new BigDecimal("123")),
- Map.entry(JDBCType.CHAR, "123"),
- Map.entry(JDBCType.VARCHAR, "123"),
- Map.entry(JDBCType.BINARY, new byte[]{1, 2, 3}),
- Map.entry(JDBCType.VARBINARY, new byte[]{1, 2, 3}),
- Map.entry(JDBCType.DATE, new Date(1)),
- Map.entry(JDBCType.TIME, Time.valueOf(LocalTime.NOON)),
- Map.entry(JDBCType.TIMESTAMP, Timestamp.valueOf("2000-01-01 00:00:00.000")),
- Map.entry(JDBCType.OTHER, new UUID(1, 1))
- );
+ private static final Map<JDBCType, Object> SUPPORTED_TYPES = new HashMap();
+
+ static {
+ SUPPORTED_TYPES.put(JDBCType.BOOLEAN, true);
+ SUPPORTED_TYPES.put(JDBCType.TINYINT, (byte) 1);
+ SUPPORTED_TYPES.put(JDBCType.SMALLINT, (short) 1);
+ SUPPORTED_TYPES.put(JDBCType.INTEGER, 1);
+ SUPPORTED_TYPES.put(JDBCType.BIGINT, 1L);
+ SUPPORTED_TYPES.put(JDBCType.FLOAT, 1.0f);
+ SUPPORTED_TYPES.put(JDBCType.REAL, 1.0f);
+ SUPPORTED_TYPES.put(JDBCType.DOUBLE, 1.0d);
+ SUPPORTED_TYPES.put(JDBCType.DECIMAL, new BigDecimal("123"));
+ SUPPORTED_TYPES.put(JDBCType.CHAR, "123");
+ SUPPORTED_TYPES.put(JDBCType.VARCHAR, "123");
+ SUPPORTED_TYPES.put(JDBCType.BINARY, new byte[]{1, 2, 3});
+ SUPPORTED_TYPES.put(JDBCType.VARBINARY, new byte[]{1, 2, 3});
+ SUPPORTED_TYPES.put(JDBCType.DATE, new Date(1));
+ SUPPORTED_TYPES.put(JDBCType.TIME, Time.valueOf(LocalTime.NOON));
+ SUPPORTED_TYPES.put(JDBCType.TIMESTAMP, Timestamp.valueOf("2000-01-01 00:00:00.000"));
+ SUPPORTED_TYPES.put(JDBCType.OTHER, new UUID(1, 1));
+ SUPPORTED_TYPES.put(JDBCType.NULL, null);
Review Comment:
it doesn't support of null's
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/api/SessionBuilderImpl.java:
##########
@@ -133,19 +135,20 @@ public SessionBuilder property(String name, @Nullable Object value) {
/** {@inheritDoc} */
@Override
public Session build() {
- PropertiesHolder propsHolder = PropertiesHelper.newBuilder()
+ Builder propBuilder = PropertiesHelper
+ .newBuilder(props)
Review Comment:
removed
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItCommonApiTest.java:
##########
@@ -120,6 +122,27 @@ public void testSessionExpiration() throws Exception {
ses2.execute(null, "SELECT 2 + 2").close();
}
+ /** Check correctness of planning timeout. */
+ @Test
+ public void testPlanningTimeout() {
+ IgniteSql sql = igniteSql();
+
+ sql("CREATE TABLE TST1(id INTEGER PRIMARY KEY, val INTEGER)");
+
+ Long oldPlannerTimeout = (Long) IgniteTestUtils.getFieldValue(queryProcessor(), SqlQueryProcessor.class, "plannerTimeout");
Review Comment:
it will be part of configuration in nearest future and the test should be reworked based on configuration
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImplTest.java:
##########
@@ -155,7 +158,7 @@ public class ExecutionServiceImplTest {
public void init() {
testCluster = new TestCluster();
executionServices = nodeNames.stream().map(this::create).collect(Collectors.toList());
- prepareService = new PrepareServiceImpl("test", 0, null);
+ prepareService = new PrepareServiceImpl("test", 0, null, 15000L);
Review Comment:
yes, thanks
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PlannerTimeoutTest.java:
##########
@@ -33,22 +35,53 @@
import org.apache.calcite.rel.type.RelDataType;
import org.apache.calcite.rel.type.RelDataTypeFactory;
import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.ignite.internal.schema.NativeTypes;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders.ConfigurationParameter;
+import org.apache.ignite.internal.sql.engine.framework.TestCluster;
+import org.apache.ignite.internal.sql.engine.framework.TestNode;
import org.apache.ignite.internal.sql.engine.prepare.IgnitePlanner;
import org.apache.ignite.internal.sql.engine.prepare.PlanningContext;
import org.apache.ignite.internal.sql.engine.rel.IgniteConvention;
import org.apache.ignite.internal.sql.engine.rel.IgniteRel;
import org.apache.ignite.internal.sql.engine.schema.IgniteSchema;
+import org.apache.ignite.internal.sql.engine.trait.IgniteDistributions;
+import org.apache.ignite.internal.sql.engine.util.SqlTestUtils;
import org.jetbrains.annotations.NotNull;
import org.junit.jupiter.api.Test;
/**
* Test planner timeout.
*/
public class PlannerTimeoutTest extends AbstractPlannerTest {
- private static final long PLANNER_TIMEOUT = 500;
+
+ /** Check correctness of planning timeout. */
+ @Test
+ public void testPlanningTimeout() {
+ TestCluster cluster = TestBuilders.cluster()
+ .nodes("N1")
+ .addTable()
+ .name("TST1")
+ .distribution(IgniteDistributions.hash(List.of(0)))
+ .addColumn("ID", NativeTypes.INT32)
+ .addColumn("VAL", NativeTypes.stringOf(64))
+ .end()
+ .addConfiguration(ConfigurationParameter.PLANNING_TIMEOUT, 1L)
+ .build();
+
+ cluster.start();
+
+ TestNode gatewayNode = cluster.node("N1");
+
+ SqlTestUtils.assertThrowsSqlException(PLANNING_TIMEOUTED_ERR,
+ () -> gatewayNode.prepare("SELECT * FROM TST1 t, TST1 t1, TST1 t2"));
+
+ }
@Test
public void testLongPlanningTimeout() throws Exception {
+ final long PLANNER_TIMEOUT = 500;
Review Comment:
agree
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestBuilders.java:
##########
@@ -358,12 +392,16 @@ public TestCluster build() {
var schemaManager = new PredefinedSchemaManager(new IgniteSchema("PUBLIC", tableMap, indexMap, SCHEMA_VERSION));
Map<String, TestNode> nodes = nodeNames.stream()
- .map(name -> new TestNode(name, clusterService.forNode(name), schemaManager))
+ .map(name -> new TestNode(name, clusterService.forNode(name), schemaManager, param(ConfigurationParameter.PLANNING_TIMEOUT)))
.collect(Collectors.toMap(TestNode::name, Function.identity()));
return new TestCluster(nodes);
}
+ private <T> T param(ConfigurationParameter parameter) {
+ return (T) configuration.get(parameter);
Review Comment:
thanks
##########
modules/jdbc/src/test/java/org/apache/ignite/internal/jdbc/PreparedStatementParamsTest.java:
##########
@@ -61,25 +62,28 @@
public class PreparedStatementParamsTest {
/** Values for supported JDBC types. */
- private static final Map<JDBCType, Object> SUPPORTED_TYPES = Map.ofEntries(
- Map.entry(JDBCType.BOOLEAN, true),
- Map.entry(JDBCType.TINYINT, (byte) 1),
- Map.entry(JDBCType.SMALLINT, (short) 1),
- Map.entry(JDBCType.INTEGER, 1),
- Map.entry(JDBCType.BIGINT, 1L),
- Map.entry(JDBCType.FLOAT, 1.0f),
- Map.entry(JDBCType.REAL, 1.0f),
- Map.entry(JDBCType.DOUBLE, 1.0d),
- Map.entry(JDBCType.DECIMAL, new BigDecimal("123")),
- Map.entry(JDBCType.CHAR, "123"),
- Map.entry(JDBCType.VARCHAR, "123"),
- Map.entry(JDBCType.BINARY, new byte[]{1, 2, 3}),
- Map.entry(JDBCType.VARBINARY, new byte[]{1, 2, 3}),
- Map.entry(JDBCType.DATE, new Date(1)),
- Map.entry(JDBCType.TIME, Time.valueOf(LocalTime.NOON)),
- Map.entry(JDBCType.TIMESTAMP, Timestamp.valueOf("2000-01-01 00:00:00.000")),
- Map.entry(JDBCType.OTHER, new UUID(1, 1))
- );
+ private static final Map<JDBCType, Object> SUPPORTED_TYPES = new HashMap();
Review Comment:
yep
--
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] ygerzhedovich commented on a diff in pull request #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261189499
##########
modules/core/src/main/java/org/apache/ignite/lang/ErrorGroups.java:
##########
@@ -203,6 +203,9 @@ public static class Sql {
/** Execution cancelled. */
public static final int EXECUTION_CANCELLED_ERR = SQL_ERR_GROUP.registerErrorCode(33);
+
+ /** Planning timeouted without any valid plan. */
+ public static final int PLANNING_TIMEOUTED_ERR = SQL_ERR_GROUP.registerErrorCode(34);
Review Comment:
Ask DB admins to inrease planning timeout or thing about decreasing complexity of the query
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261245115
##########
modules/core/src/main/java/org/apache/ignite/lang/ErrorGroups.java:
##########
@@ -203,6 +203,9 @@ public static class Sql {
/** Execution cancelled. */
public static final int EXECUTION_CANCELLED_ERR = SQL_ERR_GROUP.registerErrorCode(33);
+
+ /** Planning timeouted without any valid plan. */
+ public static final int PLANNING_TIMEOUTED_ERR = SQL_ERR_GROUP.registerErrorCode(34);
Review Comment:
ok, let it be
--
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] ygerzhedovich commented on a diff in pull request #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261240853
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/BaseQueryContext.java:
##########
@@ -148,35 +148,35 @@ public List<MetadataHandler<?>> handlers(Class<? extends MetadataHandler<?>> hnd
private final QueryCancel cancel;
+ private final String query;
Review Comment:
will be removed
--
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] ygerzhedovich commented on a diff in pull request #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261181351
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgnitePlanner.java:
##########
@@ -560,6 +564,11 @@ public void checkCancel() {
long startTs = ctx.startTs();
if (FastTimestamps.coarseCurrentTimeMillis() - startTs > timeout) {
+ LOG.info("Planning of a query aborted due to planner timeout threshold is reached [timeout={}, query={}]",
Review Comment:
It's not true. We can have two cases:
1) was have cancel planning by timeout, however already there is correct plane to execute.
2) Timeout reached, but we don't have a plan at all.
user get exception just in the second case. I think user should have ability be aware about the first case too.
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261186196
##########
modules/core/src/main/java/org/apache/ignite/lang/ErrorGroups.java:
##########
@@ -203,6 +203,9 @@ public static class Sql {
/** Execution cancelled. */
public static final int EXECUTION_CANCELLED_ERR = SQL_ERR_GROUP.registerErrorCode(33);
+
+ /** Planning timeouted without any valid plan. */
+ public static final int PLANNING_TIMEOUTED_ERR = SQL_ERR_GROUP.registerErrorCode(34);
Review Comment:
ok, then what steps should the user take as a response to this exception?
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1239634731
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/property/PropertiesHelper.java:
##########
@@ -42,6 +43,23 @@ public static Builder newBuilder() {
return new BuilderImpl();
}
+ /**
+ * Creates new builder and populates it with properties from the given Map. Keys of the map are names of properties, values are values.
+ *
+ * @param props A holder to populate new builder with.
+ * @return A builder containing properties from given holder.
+ */
+ public static Builder newBuilder(Map<String, Object> props) {
+ Builder builder = newBuilder();
+
+ for (Entry<String, Object> p : props.entrySet()) {
+ Property property = new Property(p.getKey(), p.getValue().getClass());
+ builder.set(property, p.getValue());
+ }
Review Comment:
I don't like the idea to return partially constructed object.
I'd prefer to have one or both methods:
`static Properties fromMap(Map<> props)`
or in Builder class `Builder addFromMap(Map<> props)`
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1239624615
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PlanningContext.java:
##########
@@ -39,23 +39,31 @@
* Planning context.
*/
public final class PlanningContext implements Context {
+ /** Parent context. */
Review Comment:
please don't. Such javadoc's have no value
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDmlTest.java:
##########
@@ -75,9 +74,7 @@ public void pkConstraintConsistencyTest() {
.check();
{
Review Comment:
we don't need this braces anymore
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/sql/util/SqlTestUtils.java:
##########
@@ -32,13 +35,31 @@
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.ignite.internal.sql.engine.type.UuidType;
import org.apache.ignite.sql.ColumnType;
+import org.apache.ignite.sql.SqlException;
+import org.junit.jupiter.api.function.Executable;
/**
* Test utils for SQL.
*/
public class SqlTestUtils {
private static final ThreadLocalRandom RND = ThreadLocalRandom.current();
+
+ /**
+ * <em>Assert</em> that execution of the supplied {@code executable} throws
+ * an {@link SqlException} with expected error code and return the exception.
+ *
+ * @param expectedCode Expected error code of {@link SqlException}
+ * @param executable supplier to execute and check thrown exception.
Review Comment:
```suggestion
* @param executable Supplier to execute and check thrown exception.
```
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItCommonApiTest.java:
##########
@@ -120,6 +123,21 @@ public void testSessionExpiration() throws Exception {
ses2.execute(null, "SELECT 2 + 2").close();
}
+ /** Check correctness of planning timeout. */
+ @Test
+ public void testPlanningTimeout() {
+ IgniteSql sql = igniteSql();
+
+ sql("CREATE TABLE TST1(id INTEGER PRIMARY KEY, val INTEGER)");
+
+ Session ses = sql.sessionBuilder().property(PLANNING_TIMEOUT.name, 1L).build();
+
+ SqlTestUtils.assertSqlExceptionThrows(PLANNING_TIMEOUTED_ERR,
+ () -> ses.execute(null, "SELECT * FROM TST1 t, TST1 t1, TST1 t2"));
+
+ ses.close();
Review Comment:
what if assertion on the line above fail?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -143,30 +148,45 @@ public void stop() throws Exception {
/** {@inheritDoc} */
@Override
- public CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx) {
- try {
- assert single(sqlNode);
+ public CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx, long plannerTimeout) {
+ CompletableFuture<QueryPlan> result;
+
+ assert single(sqlNode);
+
+ SqlQueryType queryType = Commons.getQueryType(sqlNode);
+ assert queryType != null : "No query type for query: " + sqlNode;
+
+ PlanningContext planningContext = PlanningContext.builder()
+ .parentContext(ctx)
+ .query(ctx.query())
+ .plannerTimeout(plannerTimeout)
+ .build();
- SqlQueryType queryType = Commons.getQueryType(sqlNode);
- assert queryType != null : "No query type for query: " + sqlNode;
+ result = prepareAsync0(sqlNode, queryType, planningContext);
- PlanningContext planningContext = PlanningContext.builder()
- .parentContext(ctx)
- .build();
+ return result.exceptionally(ex -> {
+ Throwable th = ExceptionUtils.unwrapCause(ex);
+ if (planningContext.timeouted() && th instanceof RelOptPlanner.CannotPlanException) {
+ LOG.info("Query plan is absent due to planing timeout is reached [query={}]", ctx.query());
Review Comment:
so, we are going to have 2 lines in the log per each timeout?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/BaseQueryContext.java:
##########
@@ -148,35 +148,35 @@ public List<MetadataHandler<?>> handlers(Class<? extends MetadataHandler<?>> hnd
private final QueryCancel cancel;
+ private final String query;
Review Comment:
is it possible to avoid keeping query in the context?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/property/PropertiesHelper.java:
##########
@@ -42,6 +43,23 @@ public static Builder newBuilder() {
return new BuilderImpl();
}
+ /**
+ * Creates new builder and populates it with properties from the given Map. Keys of the map are names of properties, values are values.
+ *
+ * @param props A holder to populate new builder with.
+ * @return A builder containing properties from given holder.
+ */
+ public static Builder newBuilder(Map<String, Object> props) {
Review Comment:
the only difference between PropertiesHolder and Map<String, Object> is that former ensures type safety. By exposing builder like this, you deprive PropertiesHolder of this advantage.
Besides, this particular code is working only because method equals of class Property is implemented in that way. But there is no guarantee that it won't be changed in the future.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -143,30 +148,45 @@ public void stop() throws Exception {
/** {@inheritDoc} */
@Override
- public CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx) {
- try {
- assert single(sqlNode);
+ public CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx, long plannerTimeout) {
+ CompletableFuture<QueryPlan> result;
+
+ assert single(sqlNode);
+
+ SqlQueryType queryType = Commons.getQueryType(sqlNode);
+ assert queryType != null : "No query type for query: " + sqlNode;
+
+ PlanningContext planningContext = PlanningContext.builder()
+ .parentContext(ctx)
+ .query(ctx.query())
+ .plannerTimeout(plannerTimeout)
+ .build();
- SqlQueryType queryType = Commons.getQueryType(sqlNode);
- assert queryType != null : "No query type for query: " + sqlNode;
+ result = prepareAsync0(sqlNode, queryType, planningContext);
- PlanningContext planningContext = PlanningContext.builder()
- .parentContext(ctx)
- .build();
+ return result.exceptionally(ex -> {
+ Throwable th = ExceptionUtils.unwrapCause(ex);
+ if (planningContext.timeouted() && th instanceof RelOptPlanner.CannotPlanException) {
+ LOG.info("Query plan is absent due to planing timeout is reached [query={}]", ctx.query());
+ throw new SqlException(ErrorGroups.Sql.PLANNING_TIMEOUTED_ERR);
+ }
+
+ throw new IgniteException(th);
+ }
Review Comment:
indentation doesn't look right
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgnitePlanner.java:
##########
@@ -560,6 +564,11 @@ public void checkCancel() {
long startTs = ctx.startTs();
if (FastTimestamps.coarseCurrentTimeMillis() - startTs > timeout) {
+ LOG.info("Planning of a query aborted due to planner timeout threshold is reached [timeout={}, query={}]",
Review Comment:
I'm not sure we should log it at INFO level
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261001744
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgnitePlanner.java:
##########
@@ -560,6 +564,11 @@ public void checkCancel() {
long startTs = ctx.startTs();
if (FastTimestamps.coarseCurrentTimeMillis() - startTs > timeout) {
+ LOG.info("Planning of a query aborted due to planner timeout threshold is reached [timeout={}, query={}]",
Review Comment:
Maybe it is better to move logging to Context, because it contains all the required information for logging, and the planner will not bother about what should be logged.
--
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 merged pull request #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 merged PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261180103
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -143,30 +148,45 @@ public void stop() throws Exception {
/** {@inheritDoc} */
@Override
- public CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx) {
- try {
- assert single(sqlNode);
+ public CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx, long plannerTimeout) {
+ CompletableFuture<QueryPlan> result;
+
+ assert single(sqlNode);
+
+ SqlQueryType queryType = Commons.getQueryType(sqlNode);
+ assert queryType != null : "No query type for query: " + sqlNode;
+
+ PlanningContext planningContext = PlanningContext.builder()
+ .parentContext(ctx)
+ .query(ctx.query())
+ .plannerTimeout(plannerTimeout)
+ .build();
- SqlQueryType queryType = Commons.getQueryType(sqlNode);
- assert queryType != null : "No query type for query: " + sqlNode;
+ result = prepareAsync0(sqlNode, queryType, planningContext);
- PlanningContext planningContext = PlanningContext.builder()
- .parentContext(ctx)
- .build();
+ return result.exceptionally(ex -> {
+ Throwable th = ExceptionUtils.unwrapCause(ex);
+ if (planningContext.timeouted() && th instanceof RelOptPlanner.CannotPlanException) {
+ LOG.info("Query plan is absent due to planing timeout is reached [query={}]", ctx.query());
Review Comment:
Why is exception not enough?
--
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] ygerzhedovich commented on a diff in pull request #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261189499
##########
modules/core/src/main/java/org/apache/ignite/lang/ErrorGroups.java:
##########
@@ -203,6 +203,9 @@ public static class Sql {
/** Execution cancelled. */
public static final int EXECUTION_CANCELLED_ERR = SQL_ERR_GROUP.registerErrorCode(33);
+
+ /** Planning timeouted without any valid plan. */
+ public static final int PLANNING_TIMEOUTED_ERR = SQL_ERR_GROUP.registerErrorCode(34);
Review Comment:
Ask DB admins to increase planning timeout or thing about decreasing complexity of the query
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261173076
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/BaseQueryContext.java:
##########
@@ -148,35 +148,35 @@ public List<MetadataHandler<?>> handlers(Class<? extends MetadataHandler<?>> hnd
private final QueryCancel cancel;
+ private final String query;
Review Comment:
> Why is it bad?
the more random stuff we put to the context, the harder refactoring will be
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1239619567
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDmlTest.java:
##########
@@ -384,12 +375,10 @@ public void testMergeKeysConflict() {
sql("CREATE TABLE test2 (k int PRIMARY KEY, a int, b int)");
- IgniteException ex = assertThrows(IgniteException.class, () -> sql(
- "MERGE INTO test2 USING test1 ON test1.a = test2.a "
- + "WHEN MATCHED THEN UPDATE SET b = test1.b + 1 "
- + "WHEN NOT MATCHED THEN INSERT (k, a, b) VALUES (0, a, b)"));
-
- assertEquals(Sql.DUPLICATE_KEYS_ERR, ex.code());
+ assertSqlExceptionThrows(Sql.DUPLICATE_KEYS_ERR, () -> sql(
Review Comment:
```suggestion
assertThrowsSqlException(Sql.DUPLICATE_KEYS_ERR, () -> sql(
```
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1239629356
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareService.java:
##########
@@ -28,6 +28,12 @@
public interface PrepareService extends LifecycleAware {
/**
* Prepare query plan.
+ *
+ * @param sqlNode AST of a query which need to be planned.
+ * @param ctx Query context.
+ * @param plannerTimeout Timeout in milliseconds to planning.
+ *
+ * @return Future that contains prepared query plan when completes.
*/
- CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx);
+ CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx, long plannerTimeout);
Review Comment:
Why is plannerTimeout not a part of BaseQueryContext?
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261149736
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PlannerTimeoutTest.java:
##########
@@ -33,22 +34,51 @@
import org.apache.calcite.rel.type.RelDataType;
import org.apache.calcite.rel.type.RelDataTypeFactory;
import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.ignite.internal.schema.NativeTypes;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders.ConfigurationParameter;
+import org.apache.ignite.internal.sql.engine.framework.TestCluster;
+import org.apache.ignite.internal.sql.engine.framework.TestNode;
import org.apache.ignite.internal.sql.engine.prepare.IgnitePlanner;
import org.apache.ignite.internal.sql.engine.prepare.PlanningContext;
import org.apache.ignite.internal.sql.engine.rel.IgniteConvention;
import org.apache.ignite.internal.sql.engine.rel.IgniteRel;
import org.apache.ignite.internal.sql.engine.schema.IgniteSchema;
+import org.apache.ignite.internal.sql.engine.util.SqlTestUtils;
import org.jetbrains.annotations.NotNull;
import org.junit.jupiter.api.Test;
/**
* Test planner timeout.
*/
public class PlannerTimeoutTest extends AbstractPlannerTest {
- private static final long PLANNER_TIMEOUT = 500;
+
+ /** Check correctness of planning timeout. */
+ @Test
+ public void testPlanningTimeout() {
+ TestCluster cluster = TestBuilders.cluster()
Review Comment:
please don't do this. You don't need cluster to verify behaviour of some service
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItCommonApiTest.java:
##########
Review Comment:
why changes in this file are required?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgnitePlanner.java:
##########
@@ -560,6 +564,11 @@ public void checkCancel() {
long startTs = ctx.startTs();
if (FastTimestamps.coarseCurrentTimeMillis() - startTs > timeout) {
+ LOG.info("Planning of a query aborted due to planner timeout threshold is reached [timeout={}, query={}]",
Review Comment:
the same: conversation is resolved, but nothing has changed
##########
modules/core/src/main/java/org/apache/ignite/lang/ErrorGroups.java:
##########
@@ -203,6 +203,9 @@ public static class Sql {
/** Execution cancelled. */
public static final int EXECUTION_CANCELLED_ERR = SQL_ERR_GROUP.registerErrorCode(33);
+
+ /** Planning timeouted without any valid plan. */
+ public static final int PLANNING_TIMEOUTED_ERR = SQL_ERR_GROUP.registerErrorCode(34);
Review Comment:
Does it make sense to reuse `EXECUTION_CANCELLED_ERR` with message "planning timeout" or something?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -142,24 +156,39 @@ public void stop() throws Exception {
/** {@inheritDoc} */
@Override
public CompletableFuture<QueryPlan> prepareAsync(ParsedResult parsedResult, BaseQueryContext ctx) {
- try {
- PlanningContext planningContext = PlanningContext.builder()
- .parentContext(ctx)
- .build();
+ CompletableFuture<QueryPlan> result;
+
+ PlanningContext planningContext = PlanningContext.builder()
+ .parentContext(ctx)
+ .query(ctx.query())
+ .plannerTimeout(plannerTimeout)
+ .build();
+
+ result = prepareAsync0(parsedResult, planningContext);
+
+ return result.exceptionally(ex -> {
+ Throwable th = ExceptionUtils.unwrapCause(ex);
+ if (planningContext.timeouted() && th instanceof RelOptPlanner.CannotPlanException) {
+ LOG.info("Query plan is absent due to planing timeout is reached [query={}]", ctx.query());
Review Comment:
this is the second line in the log about planning timeout.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/BaseQueryContext.java:
##########
@@ -148,35 +148,35 @@ public List<MetadataHandler<?>> handlers(Class<? extends MetadataHandler<?>> hnd
private final QueryCancel cancel;
+ private final String query;
Review Comment:
@ygerzhedovich you've resolved the conversation, but neither replied nor changed the line in question, why?
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1260983884
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImplTest.java:
##########
@@ -155,7 +158,7 @@ public class ExecutionServiceImplTest {
public void init() {
testCluster = new TestCluster();
executionServices = nodeNames.stream().map(this::create).collect(Collectors.toList());
- prepareService = new PrepareServiceImpl("test", 0, null);
+ prepareService = new PrepareServiceImpl("test", 0, null, 15000L);
Review Comment:
Do you mean?
```suggestion
prepareService = new PrepareServiceImpl("test", 0, null, PLANNING_TIMEOUT_IN_MS);
```
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1240205498
##########
modules/jdbc/src/test/java/org/apache/ignite/internal/jdbc/PreparedStatementParamsTest.java:
##########
@@ -61,25 +62,28 @@
public class PreparedStatementParamsTest {
/** Values for supported JDBC types. */
- private static final Map<JDBCType, Object> SUPPORTED_TYPES = Map.ofEntries(
- Map.entry(JDBCType.BOOLEAN, true),
- Map.entry(JDBCType.TINYINT, (byte) 1),
- Map.entry(JDBCType.SMALLINT, (short) 1),
- Map.entry(JDBCType.INTEGER, 1),
- Map.entry(JDBCType.BIGINT, 1L),
- Map.entry(JDBCType.FLOAT, 1.0f),
- Map.entry(JDBCType.REAL, 1.0f),
- Map.entry(JDBCType.DOUBLE, 1.0d),
- Map.entry(JDBCType.DECIMAL, new BigDecimal("123")),
- Map.entry(JDBCType.CHAR, "123"),
- Map.entry(JDBCType.VARCHAR, "123"),
- Map.entry(JDBCType.BINARY, new byte[]{1, 2, 3}),
- Map.entry(JDBCType.VARBINARY, new byte[]{1, 2, 3}),
- Map.entry(JDBCType.DATE, new Date(1)),
- Map.entry(JDBCType.TIME, Time.valueOf(LocalTime.NOON)),
- Map.entry(JDBCType.TIMESTAMP, Timestamp.valueOf("2000-01-01 00:00:00.000")),
- Map.entry(JDBCType.OTHER, new UUID(1, 1))
- );
+ private static final Map<JDBCType, Object> SUPPORTED_TYPES = new HashMap();
Review Comment:
It should be `HashMap<>()`
##########
modules/jdbc/src/test/java/org/apache/ignite/internal/jdbc/PreparedStatementParamsTest.java:
##########
@@ -61,25 +62,28 @@
public class PreparedStatementParamsTest {
/** Values for supported JDBC types. */
- private static final Map<JDBCType, Object> SUPPORTED_TYPES = Map.ofEntries(
- Map.entry(JDBCType.BOOLEAN, true),
- Map.entry(JDBCType.TINYINT, (byte) 1),
- Map.entry(JDBCType.SMALLINT, (short) 1),
- Map.entry(JDBCType.INTEGER, 1),
- Map.entry(JDBCType.BIGINT, 1L),
- Map.entry(JDBCType.FLOAT, 1.0f),
- Map.entry(JDBCType.REAL, 1.0f),
- Map.entry(JDBCType.DOUBLE, 1.0d),
- Map.entry(JDBCType.DECIMAL, new BigDecimal("123")),
- Map.entry(JDBCType.CHAR, "123"),
- Map.entry(JDBCType.VARCHAR, "123"),
- Map.entry(JDBCType.BINARY, new byte[]{1, 2, 3}),
- Map.entry(JDBCType.VARBINARY, new byte[]{1, 2, 3}),
- Map.entry(JDBCType.DATE, new Date(1)),
- Map.entry(JDBCType.TIME, Time.valueOf(LocalTime.NOON)),
- Map.entry(JDBCType.TIMESTAMP, Timestamp.valueOf("2000-01-01 00:00:00.000")),
- Map.entry(JDBCType.OTHER, new UUID(1, 1))
- );
+ private static final Map<JDBCType, Object> SUPPORTED_TYPES = new HashMap();
+
+ static {
+ SUPPORTED_TYPES.put(JDBCType.BOOLEAN, true);
+ SUPPORTED_TYPES.put(JDBCType.TINYINT, (byte) 1);
+ SUPPORTED_TYPES.put(JDBCType.SMALLINT, (short) 1);
+ SUPPORTED_TYPES.put(JDBCType.INTEGER, 1);
+ SUPPORTED_TYPES.put(JDBCType.BIGINT, 1L);
+ SUPPORTED_TYPES.put(JDBCType.FLOAT, 1.0f);
+ SUPPORTED_TYPES.put(JDBCType.REAL, 1.0f);
+ SUPPORTED_TYPES.put(JDBCType.DOUBLE, 1.0d);
+ SUPPORTED_TYPES.put(JDBCType.DECIMAL, new BigDecimal("123"));
+ SUPPORTED_TYPES.put(JDBCType.CHAR, "123");
+ SUPPORTED_TYPES.put(JDBCType.VARCHAR, "123");
+ SUPPORTED_TYPES.put(JDBCType.BINARY, new byte[]{1, 2, 3});
+ SUPPORTED_TYPES.put(JDBCType.VARBINARY, new byte[]{1, 2, 3});
+ SUPPORTED_TYPES.put(JDBCType.DATE, new Date(1));
+ SUPPORTED_TYPES.put(JDBCType.TIME, Time.valueOf(LocalTime.NOON));
+ SUPPORTED_TYPES.put(JDBCType.TIMESTAMP, Timestamp.valueOf("2000-01-01 00:00:00.000"));
+ SUPPORTED_TYPES.put(JDBCType.OTHER, new UUID(1, 1));
+ SUPPORTED_TYPES.put(JDBCType.NULL, null);
Review Comment:
OK. Never mind, They do not allow null.
--
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] ygerzhedovich commented on a diff in pull request #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1262355131
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -142,24 +156,39 @@ public void stop() throws Exception {
/** {@inheritDoc} */
@Override
public CompletableFuture<QueryPlan> prepareAsync(ParsedResult parsedResult, BaseQueryContext ctx) {
- try {
- PlanningContext planningContext = PlanningContext.builder()
- .parentContext(ctx)
- .build();
+ CompletableFuture<QueryPlan> result;
+
+ PlanningContext planningContext = PlanningContext.builder()
+ .parentContext(ctx)
+ .query(ctx.query())
+ .plannerTimeout(plannerTimeout)
+ .build();
+
+ result = prepareAsync0(parsedResult, planningContext);
+
+ return result.exceptionally(ex -> {
+ Throwable th = ExceptionUtils.unwrapCause(ex);
+ if (planningContext.timeouted() && th instanceof RelOptPlanner.CannotPlanException) {
+ LOG.info("Query plan is absent due to planing timeout is reached [query={}]", ctx.query());
Review Comment:
removed
--
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] ygerzhedovich commented on a diff in pull request #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261166047
##########
modules/core/src/main/java/org/apache/ignite/lang/ErrorGroups.java:
##########
@@ -203,6 +203,9 @@ public static class Sql {
/** Execution cancelled. */
public static final int EXECUTION_CANCELLED_ERR = SQL_ERR_GROUP.registerErrorCode(33);
+
+ /** Planning timeouted without any valid plan. */
+ public static final int PLANNING_TIMEOUTED_ERR = SQL_ERR_GROUP.registerErrorCode(34);
Review Comment:
From my point of view - it different situation and action from user are different too
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1245119135
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItCommonApiTest.java:
##########
@@ -120,6 +122,27 @@ public void testSessionExpiration() throws Exception {
ses2.execute(null, "SELECT 2 + 2").close();
}
+ /** Check correctness of planning timeout. */
+ @Test
+ public void testPlanningTimeout() {
+ IgniteSql sql = igniteSql();
+
+ sql("CREATE TABLE TST1(id INTEGER PRIMARY KEY, val INTEGER)");
+
+ Long oldPlannerTimeout = (Long) IgniteTestUtils.getFieldValue(queryProcessor(), SqlQueryProcessor.class, "plannerTimeout");
Review Comment:
Can we introduce 'plannerTimeout' option and pass it e.g. via session?
Or use system property instead of reflection hack.
--
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] ygerzhedovich commented on a diff in pull request #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261160463
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/api/ItCommonApiTest.java:
##########
Review Comment:
https://github.com/apache/ignite-3/pull/2214/files#r1238629282
--
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] ygerzhedovich commented on a diff in pull request #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261187200
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -143,30 +148,45 @@ public void stop() throws Exception {
/** {@inheritDoc} */
@Override
- public CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx) {
- try {
- assert single(sqlNode);
+ public CompletableFuture<QueryPlan> prepareAsync(SqlNode sqlNode, BaseQueryContext ctx, long plannerTimeout) {
+ CompletableFuture<QueryPlan> result;
+
+ assert single(sqlNode);
+
+ SqlQueryType queryType = Commons.getQueryType(sqlNode);
+ assert queryType != null : "No query type for query: " + sqlNode;
+
+ PlanningContext planningContext = PlanningContext.builder()
+ .parentContext(ctx)
+ .query(ctx.query())
+ .plannerTimeout(plannerTimeout)
+ .build();
- SqlQueryType queryType = Commons.getQueryType(sqlNode);
- assert queryType != null : "No query type for query: " + sqlNode;
+ result = prepareAsync0(sqlNode, queryType, planningContext);
- PlanningContext planningContext = PlanningContext.builder()
- .parentContext(ctx)
- .build();
+ return result.exceptionally(ex -> {
+ Throwable th = ExceptionUtils.unwrapCause(ex);
+ if (planningContext.timeouted() && th instanceof RelOptPlanner.CannotPlanException) {
+ LOG.info("Query plan is absent due to planing timeout is reached [query={}]", ctx.query());
Review Comment:
seems you are right. Will be removed
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1261176386
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgnitePlanner.java:
##########
@@ -560,6 +564,11 @@ public void checkCancel() {
long startTs = ctx.startTs();
if (FastTimestamps.coarseCurrentTimeMillis() - startTs > timeout) {
+ LOG.info("Planning of a query aborted due to planner timeout threshold is reached [timeout={}, query={}]",
Review Comment:
> But I don't see right now proper way to inform user about the situation
the user will be informed by getting an exception
--
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 #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1260980893
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestBuilders.java:
##########
@@ -358,12 +392,16 @@ public TestCluster build() {
var schemaManager = new PredefinedSchemaManager(new IgniteSchema("PUBLIC", tableMap, indexMap, SCHEMA_VERSION));
Map<String, TestNode> nodes = nodeNames.stream()
- .map(name -> new TestNode(name, clusterService.forNode(name), schemaManager))
+ .map(name -> new TestNode(name, clusterService.forNode(name), schemaManager, param(ConfigurationParameter.PLANNING_TIMEOUT)))
.collect(Collectors.toMap(TestNode::name, Function.identity()));
return new TestCluster(nodes);
}
+ private <T> T param(ConfigurationParameter parameter) {
+ return (T) configuration.get(parameter);
Review Comment:
```suggestion
return (T) configuration.getOrDefault(parameter, parameter.defaultValue());
```
--
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] ygerzhedovich commented on a diff in pull request #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1262352232
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/PlannerTimeoutTest.java:
##########
@@ -33,22 +34,51 @@
import org.apache.calcite.rel.type.RelDataType;
import org.apache.calcite.rel.type.RelDataTypeFactory;
import org.apache.calcite.util.ImmutableBitSet;
+import org.apache.ignite.internal.schema.NativeTypes;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders;
+import org.apache.ignite.internal.sql.engine.framework.TestBuilders.ConfigurationParameter;
+import org.apache.ignite.internal.sql.engine.framework.TestCluster;
+import org.apache.ignite.internal.sql.engine.framework.TestNode;
import org.apache.ignite.internal.sql.engine.prepare.IgnitePlanner;
import org.apache.ignite.internal.sql.engine.prepare.PlanningContext;
import org.apache.ignite.internal.sql.engine.rel.IgniteConvention;
import org.apache.ignite.internal.sql.engine.rel.IgniteRel;
import org.apache.ignite.internal.sql.engine.schema.IgniteSchema;
+import org.apache.ignite.internal.sql.engine.util.SqlTestUtils;
import org.jetbrains.annotations.NotNull;
import org.junit.jupiter.api.Test;
/**
* Test planner timeout.
*/
public class PlannerTimeoutTest extends AbstractPlannerTest {
- private static final long PLANNER_TIMEOUT = 500;
+
+ /** Check correctness of planning timeout. */
+ @Test
+ public void testPlanningTimeout() {
+ TestCluster cluster = TestBuilders.cluster()
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] AMashenkov commented on a diff in pull request #2214: IGNITE-19654: remove redundant planing timeout and use correct one.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2214:
URL: https://github.com/apache/ignite-3/pull/2214#discussion_r1239638368
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/api/SessionBuilderImpl.java:
##########
@@ -133,19 +135,20 @@ public SessionBuilder property(String name, @Nullable Object value) {
/** {@inheritDoc} */
@Override
public Session build() {
- PropertiesHolder propsHolder = PropertiesHelper.newBuilder()
+ Builder propBuilder = PropertiesHelper
+ .newBuilder(props)
Review Comment:
Properties are copied twice: here and in constructor.
--
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