You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2023/01/16 12:35:31 UTC
[GitHub] [ignite-3] lowka opened a new pull request, #1528: IGNITE-18422: Sql. Match number of dynamic parameters with given parameters
lowka opened a new pull request, #1528:
URL: https://github.com/apache/ignite-3/pull/1528
…meters
--
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 #1528: IGNITE-18422: Sql. Match number of dynamic parameters with given parameters
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #1528:
URL: https://github.com/apache/ignite-3/pull/1528#discussion_r1082957978
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java:
##########
@@ -459,15 +459,23 @@ private boolean isSystemFieldName(String alias) {
/** {@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);
+ if (node instanceof SqlDynamicParam) {
+ int index = ((SqlDynamicParam) node).getIndex();
+ if (index >= parameters.length) {
+ throw newValidationError(node, RESOURCE.dynamicParamIllegal());
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 #1528: IGNITE-18422: Sql. Match number of dynamic parameters with given parameters
Posted by GitBox <gi...@apache.org>.
AMashenkov commented on code in PR #1528:
URL: https://github.com/apache/ignite-3/pull/1528#discussion_r1082316721
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/util/QueryChecker.java:
##########
@@ -363,27 +365,54 @@ public QueryChecker planEquals(String plan) {
return this;
}
+ /**
+ * Specifies whether to execute {@code EXPLAIN PLAN FOR} statement before running a query or not.
+ * The default value is {@code true}.
+ * <p>
+ * If the value is set to {@code false} but either plan or at least one plan matcher is specified {@link #check()}
+ * throws {@link IllegalStateException}.
+ * </p>
+ *
+ * @param value whether to execute EXPLAIN PLAN statement or not.
+ * @return this
Review Comment:
```suggestion
*
* @param value whether to execute EXPLAIN PLAN statement or not.
* @return this
* @throws IllegalStateException If the value is set to {@code false} but either plan or at least one plan matcher is specified {@link #check()}
```
--
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 #1528: IGNITE-18422: Sql. Match number of dynamic parameters with given parameters
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #1528:
URL: https://github.com/apache/ignite-3/pull/1528#discussion_r1082915985
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java:
##########
@@ -114,6 +119,35 @@ public IgniteSqlValidator(SqlOperatorTable opTab, CalciteCatalogReader catalogRe
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)
+ if (topNode instanceof SqlExplain) {
+ topNodeToValidate = ((SqlExplain) topNode).getExplicandum();
+ // We do not validate dynamic parameters in case of EXPLAIN FOR queries since they may be omitted.
+ // TODO: We must enable this check for EXPLAIN ANALYSE flavour for EXPLAIN FOR queries.
Review Comment:
We should open an issue for this one.
--
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 #1528: IGNITE-18422: Sql. Match number of dynamic parameters with given parameters
Posted by GitBox <gi...@apache.org>.
lowka commented on code in PR #1528:
URL: https://github.com/apache/ignite-3/pull/1528#discussion_r1082340129
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/util/QueryChecker.java:
##########
@@ -251,6 +251,8 @@ public static Matcher<String> containsAnyScan(final String schema, final String
private String exactPlan;
+ private boolean executeExplainPlan = true;
Review Comment:
Yep. Going to fix this since it does not work anyway.
--
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 #1528: IGNITE-18422: Sql. Match number of dynamic parameters with given parameters
Posted by GitBox <gi...@apache.org>.
ygerzhedovich commented on code in PR #1528:
URL: https://github.com/apache/ignite-3/pull/1528#discussion_r1081461103
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java:
##########
@@ -459,15 +459,23 @@ private boolean isSystemFieldName(String alias) {
/** {@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);
+ if (node instanceof SqlDynamicParam) {
+ int index = ((SqlDynamicParam) node).getIndex();
+ if (index >= parameters.length) {
+ throw newValidationError(node, RESOURCE.dynamicParamIllegal());
Review Comment:
I think we should also throw error in case user pass concrete values more than query has dynamics parameters. And seems it could be done a little bit earlier.
--
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 #1528: IGNITE-18422: Sql. Match number of dynamic parameters with given parameters
Posted by GitBox <gi...@apache.org>.
AMashenkov commented on code in PR #1528:
URL: https://github.com/apache/ignite-3/pull/1528#discussion_r1082316721
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/util/QueryChecker.java:
##########
@@ -363,27 +365,54 @@ public QueryChecker planEquals(String plan) {
return this;
}
+ /**
+ * Specifies whether to execute {@code EXPLAIN PLAN FOR} statement before running a query or not.
+ * The default value is {@code true}.
+ * <p>
+ * If the value is set to {@code false} but either plan or at least one plan matcher is specified {@link #check()}
+ * throws {@link IllegalStateException}.
+ * </p>
+ *
+ * @param value whether to execute EXPLAIN PLAN statement or not.
+ * @return this
Review Comment:
```suggestion
*
* @param value whether to execute EXPLAIN PLAN statement or not.
* @return this
* @throws If the value is set to {@code false} but either plan or at least one plan matcher is specified {@link #check()}
```
--
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 #1528: IGNITE-18422: Sql. Match number of dynamic parameters with given parameters
Posted by GitBox <gi...@apache.org>.
AMashenkov commented on code in PR #1528:
URL: https://github.com/apache/ignite-3/pull/1528#discussion_r1082320651
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/util/QueryChecker.java:
##########
@@ -251,6 +251,8 @@ public static Matcher<String> containsAnyScan(final String schema, final String
private String exactPlan;
+ private boolean executeExplainPlan = true;
Review Comment:
It looks unclear for me.
If `executeExplainPlan` is true, then will the query itself be excecuted, or just EXPLAIN for the query?
Maybe `skipExplan` will be more precise name.
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/util/QueryChecker.java:
##########
@@ -251,6 +251,8 @@ public static Matcher<String> containsAnyScan(final String schema, final String
private String exactPlan;
+ private boolean executeExplainPlan = true;
Review Comment:
It looks unclear for me.
If `executeExplainPlan` is true, then will the query itself be excecuted, or just EXPLAIN for the query?
Maybe `skipExplain` will be more precise name.
--
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 #1528: IGNITE-18422: Sql. Match number of dynamic parameters with given parameters
Posted by GitBox <gi...@apache.org>.
AMashenkov commented on code in PR #1528:
URL: https://github.com/apache/ignite-3/pull/1528#discussion_r1082316721
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/util/QueryChecker.java:
##########
@@ -363,27 +365,54 @@ public QueryChecker planEquals(String plan) {
return this;
}
+ /**
+ * Specifies whether to execute {@code EXPLAIN PLAN FOR} statement before running a query or not.
+ * The default value is {@code true}.
+ * <p>
+ * If the value is set to {@code false} but either plan or at least one plan matcher is specified {@link #check()}
+ * throws {@link IllegalStateException}.
+ * </p>
+ *
+ * @param value whether to execute EXPLAIN PLAN statement or not.
+ * @return this
Review Comment:
```suggestion
*
* @param value whether to execute EXPLAIN PLAN statement or not.
* @return {@code this}
* @throws IllegalStateException If the value is set to {@code false} but either plan or at least one plan matcher is specified {@link #check()}
```
--
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 #1528: IGNITE-18422: Sql. Match number of dynamic parameters with given parameters
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #1528:
URL: https://github.com/apache/ignite-3/pull/1528#discussion_r1082958726
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/sql/engine/util/QueryChecker.java:
##########
@@ -363,27 +365,54 @@ public QueryChecker planEquals(String plan) {
return this;
}
+ /**
+ * Specifies whether to execute {@code EXPLAIN PLAN FOR} statement before running a query or not.
+ * The default value is {@code true}.
+ * <p>
+ * If the value is set to {@code false} but either plan or at least one plan matcher is specified {@link #check()}
+ * throws {@link IllegalStateException}.
+ * </p>
+ *
+ * @param value whether to execute EXPLAIN PLAN statement or not.
+ * @return this
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] korlov42 merged pull request #1528: IGNITE-18422: Sql. Match number of dynamic parameters with given parameters
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 merged PR #1528:
URL: https://github.com/apache/ignite-3/pull/1528
--
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