zstan commented on code in PR #2047:
URL: https://github.com/apache/ignite-3/pull/2047#discussion_r1201570347
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java:
##########
@@ -557,82 +585,93 @@ private boolean isSystemFieldName(String alias) {
@Override
protected void inferUnknownTypes(RelDataType inferredType,
SqlValidatorScope scope, SqlNode node) {
if (node instanceof SqlDynamicParam) {
- SqlDynamicParam dynamicParam = (SqlDynamicParam) node;
- RelDataType type = inferDynamicParamType(dynamicParam);
+ RelDataType result = inferDynamicParamType(inferredType,
(SqlDynamicParam) node);
- boolean narrowType = inferredType.equals(unknownType) ||
SqlTypeUtil.canCastFrom(inferredType, type, true)
- &&
SqlTypeUtil.comparePrecision(inferredType.getPrecision(), type.getPrecision())
> 0;
-
- if (narrowType) {
- setValidatedNodeType(node, type);
+ this.setValidatedNodeType(node, result);
+ } else {
+ super.inferUnknownTypes(inferredType, scope, node);
+ }
+ }
- return;
- }
+ private RelDataType inferDynamicParamType(RelDataType inferredType,
SqlDynamicParam dynamicParam) {
+ RelDataType type = getDynamicParamType(dynamicParam);
+ RelDataType paramTypeToUse;
+
+ /*
+ * If inferredType is unknown - use a type of dynamic parameter since
there is no other source of type information.
+ *
+ * If parameter's type and the inferredType do not match - use
parameter's type.
+ * This makes CAST operations to work correctly. Otherwise cast's
operand is going to have
+ * the same type as a target type which is not correct as it
+ * makes every CAST operation eligible to redundant type conversion
elimination
+ * at later stages:
+ * E.g: CAST(? AS INTEGER) where ?='hello' operand is going to be
inferred as INTEGER
+ * although it is a string.
+ *
+ * In other cases use the inferredType and we rely on type inference
rules provided by
+ * operator's SqlOperandTypeInference and SqlOperandTypeCheckers.
+ */
+
+ if (inferredType.equals(unknownType) ||
(!SqlTypeUtil.equalSansNullability(type, inferredType))) {
+ paramTypeToUse = type;
+ } else {
+ paramTypeToUse = inferredType;
}
- if (node instanceof SqlCall) {
- SqlValidatorScope newScope = scopes.get(node);
+ return typeFactory.createTypeWithNullability(paramTypeToUse, true);
+ }
- if (newScope != null) {
- scope = newScope;
- }
+ /** Returns type of the given dynamic parameter. */
+ RelDataType getDynamicParamType(SqlDynamicParam dynamicParam) {
+ IgniteTypeFactory typeFactory = (IgniteTypeFactory)
this.getTypeFactory();
+ Object value = getDynamicParamValue(dynamicParam);
- SqlCall call = (SqlCall) node;
- SqlOperandTypeInference operandTypeInference =
call.getOperator().getOperandTypeInference();
- SqlOperandTypeChecker operandTypeChecker =
call.getOperator().getOperandTypeChecker();
- SqlCallBinding callBinding = new SqlCallBinding(this, scope, call);
- List<SqlNode> operands = callBinding.operands();
- RelDataType[] operandTypes = new RelDataType[operands.size()];
+ RelDataType parameterType;
+ // IgniteCustomType: first we must check whether dynamic parameter is
a custom data type.
+ // If so call createCustomType with appropriate arguments.
+ if (value instanceof UUID) {
+ parameterType = typeFactory.createCustomType(UuidType.NAME);
+ } else if (value == null) {
+ parameterType = typeFactory.createSqlType(SqlTypeName.NULL);
+ } else {
+ parameterType =
typeFactory.toSql(typeFactory.createType(value.getClass()));
+ }
- Arrays.fill(operandTypes, unknownType);
+ // Dynamic parameters are always nullable.
+ // Otherwise it seem to cause "Conversion to relational algebra failed
to preserve datatypes" errors
+ // in some cases.
+ return typeFactory.createTypeWithNullability(parameterType, true);
+ }
- if (operandTypeInference != null && !(call instanceof SqlCase)) {
- operandTypeInference.inferOperandTypes(callBinding,
inferredType, operandTypes);
- } else if (operandTypeChecker instanceof FamilyOperandTypeChecker)
{
- // Infer operand types from checker for dynamic parameters if
it's possible.
- FamilyOperandTypeChecker checker = (FamilyOperandTypeChecker)
operandTypeChecker;
+ private Object getDynamicParamValue(SqlDynamicParam dynamicParam) {
+ Object value = parameters[dynamicParam.getIndex()];
+ // save dynamic parameter for later validation.
+ dynamicParams[dynamicParam.getIndex()] = dynamicParam;
+ return value;
+ }
- for (int i = 0; i < checker.getOperandCountRange().getMax();
i++) {
- if (i >= operandTypes.length) {
- break;
- }
+ /**
+ * Post validation check of dynamic parameters.
+ */
+ private void checkDynamicParameters() {
+ // Derived types of dynamic parameters should not change (current type
inference behavior).
- SqlTypeFamily family = checker.getOperandSqlTypeFamily(i);
- RelDataType type =
family.getDefaultConcreteType(typeFactory());
+ for (int i = 0; i < parameters.length; i++) {
+ SqlDynamicParam param = dynamicParams[i];
+ assert param != null : format("Dynamic parameter#{} has not been
checked", i);
- if (type != null && operands.get(i) instanceof
SqlDynamicParam) {
- operandTypes[i] = type;
- }
- }
- }
+ RelDataType paramType = getDynamicParamType(param);
+ RelDataType derivedType = getValidatedNodeType(param);
- for (int i = 0; i < operands.size(); ++i) {
- SqlNode operand = operands.get(i);
+ // We can check for nullability, but it was set to true.
+ if (!SqlTypeUtil.equalSansNullability(derivedType, paramType)) {
Review Comment:
you need to use
org.apache.ignite.internal.util.IgniteUtils#assertionsEnabled or rework this
approach at all
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/type/IgniteTypeSystem.java:
##########
@@ -148,4 +149,37 @@ public RelDataType deriveSumType(RelDataTypeFactory
typeFactory, RelDataType arg
return typeFactory.createTypeWithNullability(sumType,
argumentType.isNullable());
}
+
+ /**
+ * Checks that {@code toType} and {@code fromType} have compatible type
families taking into account custom data types.
+ *
+ * @see SqlTypeUtil#canAssignFrom(RelDataType, RelDataType)
+ */
+ public boolean typeFamiliesAreCompatible(RelDataTypeFactory typeFactory,
RelDataType toType, RelDataType fromType) {
+ // Types T1 and T2 have compatible type families if their types are
assignable.
+
+ // Same types are always compatible.
+ if (SqlTypeUtil.equalSansNullability(typeFactory, toType, fromType)) {
+ return true;
+ }
+
+ // NULL is compatible with all types.
+ if (fromType.getSqlTypeName() == SqlTypeName.NULL ||
toType.getSqlTypeName() == SqlTypeName.NULL) {
+ return true;
+ } else if (fromType instanceof IgniteCustomType && toType instanceof
IgniteCustomType) {
+ IgniteCustomType fromCustom = (IgniteCustomType) fromType;
+ IgniteCustomType toCustom = (IgniteCustomType) toType;
+
+ // IgniteCustomType: different custom data types are not
compatible.
+ return Objects.equals(fromCustom.getCustomTypeName(),
toCustom.getCustomTypeName());
+ } else if (fromType instanceof IgniteCustomType || toType instanceof
IgniteCustomType) {
+ // Custom data types are not compatible with other types.
+ return false;
+ } else {
+ boolean fromTo = SqlTypeUtil.canAssignFrom(toType, fromType);
Review Comment:
whats the difference between 1 time call : SqlTypeUtil.inSameFamily(t1, t2) ?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java:
##########
@@ -103,6 +100,9 @@ public class IgniteSqlValidator extends SqlValidatorImpl {
/** Dynamic parameters. */
private final Object[] parameters;
+ /** Types of dynamic parameters. */
+ private final SqlDynamicParam[] dynamicParams;
Review Comment:
not a type.
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/util/StatementCheckerTest.java:
##########
@@ -0,0 +1,175 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.sql.engine.util;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.when;
+
+import com.google.common.collect.ImmutableList;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.List;
+import java.util.Map;
+import org.apache.calcite.plan.RelTraitSet;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.ignite.internal.sql.engine.rel.IgniteRel;
+import org.apache.ignite.internal.sql.engine.rel.IgniteValues;
+import org.apache.ignite.internal.sql.engine.schema.IgniteSchema;
+import org.apache.ignite.internal.sql.engine.type.IgniteTypeFactory;
+import org.apache.ignite.internal.sql.engine.util.StatementChecker.SqlPrepare;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.DynamicTest;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+import org.opentest4j.AssertionFailedError;
+
+/**
+ * Tests for {@link StatementChecker}.
+ */
+public class StatementCheckerTest {
+
+ private final RelNode dummyNode = Mockito.mock(IgniteRel.class);
+
+ private final SqlPrepare sqlPrepare = Mockito.mock(SqlPrepare.class);
+
+ /** Validation check should pass. */
+ @Test
+ public void testOk() throws Throwable {
+ when(sqlPrepare.prepare(any(IgniteSchema.class), any(String.class),
any(List.class))).thenReturn(dummyNode);
+
+ DynamicTest test = newChecker().sql("SELECT 1").ok();
+ assertEquals("OK SELECT 1", test.getDisplayName(), "display name");
+
+ test.getExecutable().execute();
+ }
+
+ /** Validation check should pass - error any error is accepted. */
Review Comment:
```suggestion
/** Validation check should pass - any error is accepted. */
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/type/IgniteTypeSystem.java:
##########
@@ -148,4 +149,37 @@ public RelDataType deriveSumType(RelDataTypeFactory
typeFactory, RelDataType arg
return typeFactory.createTypeWithNullability(sumType,
argumentType.isNullable());
}
+
+ /**
+ * Checks that {@code toType} and {@code fromType} have compatible type
families taking into account custom data types.
+ *
+ * @see SqlTypeUtil#canAssignFrom(RelDataType, RelDataType)
+ */
+ public boolean typeFamiliesAreCompatible(RelDataTypeFactory typeFactory,
RelDataType toType, RelDataType fromType) {
Review Comment:
why you use keyword "family" here ?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java:
##########
@@ -153,6 +160,48 @@ public void validateUpdate(SqlUpdate call) {
validateUpdateFields(call);
super.validateUpdate(call);
+
+ SqlSelect select = call.getSourceSelect();
+ assert select != null : "Update: SourceSelect has not been set";
+
+ // Update creates a source expression list which is not updated
+ // after type coercion adds CASTs to source expressions.
+ syncSelectList(select, call);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public void validateMerge(SqlMerge call) {
+ super.validateMerge(call);
+
+ SqlSelect select = call.getSourceSelect();
+ SqlUpdate update = call.getUpdateCall();
+
+ if (update != null) {
+ assert select != null : "Merge: SourceSelect has not been set";
Review Comment:
why we need only assertions on validation ?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/sql/fun/IgniteSqlOperatorTable.java:
##########
@@ -105,7 +105,7 @@ public class IgniteSqlOperatorTable extends
ReflectiveSqlOperatorTable {
*/
public static final SqlFunction SUBSTR = new SqlFunction("SUBSTR",
SqlKind.OTHER_FUNCTION,
ReturnTypes.ARG0_NULLABLE_VARYING, null,
- OperandTypes.STRING_INTEGER_OPTIONAL_INTEGER,
+
OperandTypes.STRING_INTEGER.or(OperandTypes.STRING_INTEGER_INTEGER),
Review Comment:
whats the problem with previous implementation ?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/type/IgniteTypeSystem.java:
##########
@@ -148,4 +149,37 @@ public RelDataType deriveSumType(RelDataTypeFactory
typeFactory, RelDataType arg
return typeFactory.createTypeWithNullability(sumType,
argumentType.isNullable());
}
+
+ /**
+ * Checks that {@code toType} and {@code fromType} have compatible type
families taking into account custom data types.
+ *
+ * @see SqlTypeUtil#canAssignFrom(RelDataType, RelDataType)
+ */
+ public boolean typeFamiliesAreCompatible(RelDataTypeFactory typeFactory,
RelDataType toType, RelDataType fromType) {
+ // Types T1 and T2 have compatible type families if their types are
assignable.
+
+ // Same types are always compatible.
+ if (SqlTypeUtil.equalSansNullability(typeFactory, toType, fromType)) {
+ return true;
+ }
+
+ // NULL is compatible with all types.
+ if (fromType.getSqlTypeName() == SqlTypeName.NULL ||
toType.getSqlTypeName() == SqlTypeName.NULL) {
+ return true;
+ } else if (fromType instanceof IgniteCustomType && toType instanceof
IgniteCustomType) {
+ IgniteCustomType fromCustom = (IgniteCustomType) fromType;
+ IgniteCustomType toCustom = (IgniteCustomType) toType;
+
+ // IgniteCustomType: different custom data types are not
compatible.
+ return Objects.equals(fromCustom.getCustomTypeName(),
toCustom.getCustomTypeName());
+ } else if (fromType instanceof IgniteCustomType || toType instanceof
IgniteCustomType) {
+ // Custom data types are not compatible with other types.
+ return false;
Review Comment:
do we have such a tests?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
