From b158d2fe18068cff183dd58a1f05382ae4944b69 Mon Sep 17 00:00:00 2001 From: Aleksey Plekhanov Date: Fri, 19 Jul 2024 10:36:38 +0300 Subject: [PATCH] IGNITE-22716 SQL Calcite: Fix implicit conversion to DECIMAL for some functions - Fixes #11441. Signed-off-by: Aleksey Plekhanov --- .../processors/query/calcite/RootQuery.java | 8 ++++++- .../calcite/exec/exp/ConverterUtils.java | 17 ++++++++++++++ .../query/calcite/exec/exp/RexImpTable.java | 2 +- .../calcite/exec/exp/RexToLixTranslator.java | 22 ++++++++++-------- .../query/calcite/QueryChecker.java | 18 +++++++++++++-- .../calcite/integration/DataTypesTest.java | 23 +++++++++++++++++++ 6 files changed, 76 insertions(+), 14 deletions(-) diff --git a/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/RootQuery.java b/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/RootQuery.java index 31fafc96eee6c..e67e7f98435b6 100644 --- a/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/RootQuery.java +++ b/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/RootQuery.java @@ -29,6 +29,7 @@ import java.util.stream.Collectors; import org.apache.calcite.plan.Context; import org.apache.calcite.schema.SchemaPlus; +import org.apache.calcite.tools.FrameworkConfig; import org.apache.calcite.tools.Frameworks; import org.apache.calcite.util.CancelFlag; import org.apache.ignite.IgniteCheckedException; @@ -139,10 +140,15 @@ public RootQuery( Context parent = Commons.convert(qryCtx); + FrameworkConfig frameworkCfg = qryCtx != null ? qryCtx.unwrap(FrameworkConfig.class) : null; + + if (frameworkCfg == null) + frameworkCfg = FRAMEWORK_CONFIG; + ctx = BaseQueryContext.builder() .parentContext(parent) .frameworkConfig( - Frameworks.newConfigBuilder(FRAMEWORK_CONFIG) + Frameworks.newConfigBuilder(frameworkCfg) .defaultSchema(schema) .build() ) diff --git a/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java b/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java index 0beb119701174..90c0c2af06bec 100644 --- a/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java +++ b/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java @@ -34,8 +34,10 @@ import org.apache.calcite.rex.RexNode; import org.apache.calcite.runtime.SqlFunctions; import org.apache.calcite.sql.type.SqlTypeName; +import org.apache.calcite.sql.type.SqlTypeUtil; import org.apache.calcite.util.BuiltInMethod; import org.apache.calcite.util.Util; +import org.apache.ignite.internal.processors.query.calcite.util.Commons; /** */ public class ConverterUtils { @@ -166,6 +168,21 @@ static List internalTypes(List operandList) { return Util.transform(operandList, node -> toInternal(node.getType())); } + /** + * Convert {@code operand} to {@code targetType}. + * + * @param operand The expression to convert + * @param targetType Target type + * @return A new expression with java type corresponding to {@code targetType} + * or original expression if there is no need to convert. + */ + public static Expression convert(Expression operand, RelDataType targetType) { + if (SqlTypeUtil.isDecimal(targetType)) + return convertToDecimal(operand, targetType); + else + return convert(operand, Commons.typeFactory().getJavaClass(targetType)); + } + /** * Convert {@code operand} to target type {@code toType}. * diff --git a/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/RexImpTable.java b/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/RexImpTable.java index e52342a432027..433fb9d29637a 100644 --- a/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/RexImpTable.java +++ b/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/RexImpTable.java @@ -1963,7 +1963,7 @@ private ParameterExpression genValueStatement( final Expression convertedCallVal = noConvert ? callVal - : ConverterUtils.convert(callVal, returnType); + : ConverterUtils.convert(callVal, call.getType()); final Expression valExpression = Expressions.condition(condition, diff --git a/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/RexToLixTranslator.java b/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/RexToLixTranslator.java index 43ed1dbd6380c..e697948cc0639 100644 --- a/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/RexToLixTranslator.java +++ b/modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/RexToLixTranslator.java @@ -62,7 +62,6 @@ import org.apache.calcite.sql.SqlOperator; import org.apache.calcite.sql.fun.SqlStdOperatorTable; import org.apache.calcite.sql.parser.SqlParserPos; -import org.apache.calcite.sql.type.SqlTypeName; import org.apache.calcite.sql.type.SqlTypeUtil; import org.apache.calcite.sql.validate.SqlConformance; import org.apache.calcite.util.BuiltInMethod; @@ -552,11 +551,9 @@ Expression translateCast( } break; } - if (targetType.getSqlTypeName() == SqlTypeName.DECIMAL) - convert = ConverterUtils.convertToDecimal(operand, targetType); if (convert == null) - convert = ConverterUtils.convert(operand, typeFactory.getJavaClass(targetType)); + convert = ConverterUtils.convert(operand, targetType); // Going from anything to CHAR(n) or VARCHAR(n), make sure value is no // longer than n. @@ -1073,7 +1070,7 @@ private Result implementCaseWhen(RexCall call) { list.newName("case_when_value")); list.add(Expressions.declare(0, valVariable, null)); final List operandList = call.getOperands(); - implementRecursively(this, operandList, valVariable, 0); + implementRecursively(this, operandList, valVariable, call.getType(), 0); final Expression isNullExpression = checkNull(valVariable); final ParameterExpression isNullVariable = Expressions.parameter( @@ -1108,8 +1105,13 @@ private Result implementCaseWhen(RexCall call) { * } * */ - private void implementRecursively(final RexToLixTranslator currentTranslator, - final List operandList, final ParameterExpression valueVariable, int pos) { + private void implementRecursively( + final RexToLixTranslator currentTranslator, + final List operandList, + final ParameterExpression valueVariable, + final RelDataType valueType, + int pos + ) { final BlockBuilder curBlockBuilder = currentTranslator.getBlockBuilder(); final List storageTypes = ConverterUtils.internalTypes(operandList); // [ELSE] clause @@ -1119,7 +1121,7 @@ private void implementRecursively(final RexToLixTranslator currentTranslator, curBlockBuilder.add( Expressions.statement( Expressions.assign(valueVariable, - ConverterUtils.convert(res, valueVariable.getType())))); + ConverterUtils.convert(res, valueType)))); return; } // Condition code: !a_isNull && a_value @@ -1141,7 +1143,7 @@ private void implementRecursively(final RexToLixTranslator currentTranslator, ifTrueBlockBuilder.add( Expressions.statement( Expressions.assign(valueVariable, - ConverterUtils.convert(ifTrueRes, valueVariable.getType())))); + ConverterUtils.convert(ifTrueRes, valueType)))); final BlockStatement ifTrue = ifTrueBlockBuilder.toBlock(); // There is no [ELSE] clause if (pos + 1 == operandList.size() - 1) { @@ -1154,7 +1156,7 @@ private void implementRecursively(final RexToLixTranslator currentTranslator, new BlockBuilder(true, curBlockBuilder); final RexToLixTranslator ifFalseTranslator = currentTranslator.setBlock(ifFalseBlockBuilder); - implementRecursively(ifFalseTranslator, operandList, valueVariable, pos + 2); + implementRecursively(ifFalseTranslator, operandList, valueVariable, valueType, pos + 2); final BlockStatement ifFalse = ifFalseBlockBuilder.toBlock(); curBlockBuilder.add( Expressions.ifThenElse(tester, ifTrue, ifFalse)); diff --git a/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/QueryChecker.java b/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/QueryChecker.java index 19f12244783f4..2d8f936919d18 100644 --- a/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/QueryChecker.java +++ b/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/QueryChecker.java @@ -28,12 +28,14 @@ import java.util.stream.Collectors; import java.util.stream.IntStream; +import org.apache.calcite.tools.FrameworkConfig; import org.apache.ignite.Ignite; import org.apache.ignite.cache.query.FieldsQueryCursor; import org.apache.ignite.internal.IgniteEx; import org.apache.ignite.internal.IgniteInterruptedCheckedException; import org.apache.ignite.internal.processors.cache.distributed.dht.atomic.GridDhtAtomicCache; import org.apache.ignite.internal.processors.cache.distributed.dht.topology.GridDhtLocalPartition; +import org.apache.ignite.internal.processors.query.QueryContext; import org.apache.ignite.internal.processors.query.QueryEngine; import org.apache.ignite.internal.processors.query.schema.management.SchemaManager; import org.apache.ignite.internal.util.typedef.F; @@ -296,6 +298,9 @@ public static Matcher containsAnyScan(final String schema, final String /** */ private String exactPlan; + /** */ + private FrameworkConfig frameworkCfg; + /** */ public QueryChecker(String qry) { this.qry = qry; @@ -322,6 +327,13 @@ public QueryChecker withParams(Object... params) { return this; } + /** */ + public QueryChecker withFrameworkConfig(FrameworkConfig frameworkCfg) { + this.frameworkCfg = frameworkCfg; + + return this; + } + /** */ public QueryChecker returns(Object... res) { if (expectedResult == null) @@ -370,8 +382,10 @@ public void check() { // Check plan. QueryEngine engine = getEngine(); + QueryContext ctx = frameworkCfg != null ? QueryContext.of(frameworkCfg) : null; + List>> explainCursors = - engine.query(null, "PUBLIC", "EXPLAIN PLAN FOR " + qry, params); + engine.query(ctx, "PUBLIC", "EXPLAIN PLAN FOR " + qry, params); FieldsQueryCursor> explainCursor = explainCursors.get(0); List> explainRes = explainCursor.getAll(); @@ -387,7 +401,7 @@ public void check() { // Check result. List>> cursors = - engine.query(null, "PUBLIC", qry, params); + engine.query(ctx, "PUBLIC", qry, params); FieldsQueryCursor> cur = cursors.get(0); diff --git a/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/DataTypesTest.java b/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/DataTypesTest.java index b6dd0aa7b82af..23474698fe533 100644 --- a/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/DataTypesTest.java +++ b/modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/DataTypesTest.java @@ -24,6 +24,8 @@ import java.util.stream.Collectors; import com.google.common.collect.ImmutableSet; import org.apache.calcite.runtime.CalciteException; +import org.apache.calcite.tools.FrameworkConfig; +import org.apache.calcite.tools.Frameworks; import org.apache.ignite.IgniteCache; import org.apache.ignite.cache.QueryEntity; import org.apache.ignite.configuration.CacheConfiguration; @@ -31,6 +33,8 @@ import org.apache.ignite.internal.util.typedef.F; import org.junit.Test; +import static org.apache.ignite.internal.processors.query.calcite.CalciteQueryProcessor.FRAMEWORK_CONFIG; + /** * Test SQL data types. */ @@ -467,6 +471,25 @@ public void testNumericConversion() { .check(); } + /** */ + @Test + public void testFunctionArgsToNumericImplicitConversion() { + assertQuery("select decode(?, 0, 0, 1, 1.0)").withParams(0).returns(new BigDecimal("0.0")).check(); + assertQuery("select decode(?, 0, 0, 1, 1.0)").withParams(1).returns(new BigDecimal("1.0")).check(); + assertQuery("select decode(?, 0, 0, 1, 1.000)").withParams(0).returns(new BigDecimal("0.000")).check(); + assertQuery("select decode(?, 0, 0, 1, 1.000)").withParams(1).returns(new BigDecimal("1.000")).check(); + assertQuery("select decode(?, 0, 0.0, 1, 1.000)").withParams(0).returns(new BigDecimal("0.000")).check(); + assertQuery("select decode(?, 0, 0.000, 1, 1.0)").withParams(1).returns(new BigDecimal("1.000")).check(); + + // With callRewrite==true function COALESCE is rewritten to CASE and CoalesceImplementor can't be checked. + FrameworkConfig frameworkCfg = Frameworks.newConfigBuilder(FRAMEWORK_CONFIG) + .sqlValidatorConfig(FRAMEWORK_CONFIG.getSqlValidatorConfig().withCallRewrite(false)) + .build(); + + assertQuery("select coalesce(?, 1.000)").withParams(0).withFrameworkConfig(frameworkCfg) + .returns(new BigDecimal("0.000")).check(); + } + /** */ @Test public void testArithmeticOverflow() {