diff --git a/core/src/test/java/io/substrait/extension/FunctionBindingResolverTest.java b/core/src/test/java/io/substrait/extension/FunctionBindingResolverTest.java index 6661bb9d0..9d19f6cae 100644 --- a/core/src/test/java/io/substrait/extension/FunctionBindingResolverTest.java +++ b/core/src/test/java/io/substrait/extension/FunctionBindingResolverTest.java @@ -74,13 +74,13 @@ void decimalDivisionDerivesIndependentlyOfTheDeclaredOutputType() { scalar(DefaultExtensionCatalog.FUNCTIONS_ARITHMETIC_DECIMAL, "divide:dec_dec"); List arguments = List.of(ResolvedArgument.value(R.decimal(10, 2)), ResolvedArgument.value(R.decimal(5, 1))); - assertEquals(R.decimal(21, 8), FunctionBindingResolver.deriveOutputType(divide, arguments)); + assertEquals(R.decimal(17, 8), FunctionBindingResolver.deriveOutputType(divide, arguments)); assertEquals( - R.decimal(21, 8), - FunctionBindingResolver.resolveAndValidate(divide, arguments, List.of(), R.decimal(21, 8)) + R.decimal(17, 8), + FunctionBindingResolver.resolveAndValidate(divide, arguments, List.of(), R.decimal(17, 8)) .outputType()); - for (Type declared : List.of(R.decimal(20, 2), R.decimal(21, 7), N.decimal(21, 8))) { + for (Type declared : List.of(R.decimal(21, 8), R.decimal(17, 7), N.decimal(17, 8))) { InvalidFunctionBindingException error = assertThrows( InvalidFunctionBindingException.class, diff --git a/core/src/test/java/io/substrait/type/ReturnProgramTypeTest.java b/core/src/test/java/io/substrait/type/ReturnProgramTypeTest.java index 5b860cc2b..4f3cad9f2 100644 --- a/core/src/test/java/io/substrait/type/ReturnProgramTypeTest.java +++ b/core/src/test/java/io/substrait/type/ReturnProgramTypeTest.java @@ -35,14 +35,15 @@ private static Type resolve(String key, Type... arguments) { "multiply,10,2,5,1,16,3", "multiply,38,10,38,10,38,6", "multiply,30,20,30,20,38,17", - "divide,10,2,5,1,21,8", + "divide,10,2,5,1,17,8", + "divide,10,2,9,0,20,12", + "divide,28,10,10,2,38,18", + "divide,32,0,1,0,38,6", "divide,38,10,38,10,38,6", "modulus,10,2,5,1,6,2" }) void decimalProgramsDeriveTheCatalogFormula( String name, int p1, int s1, int p2, int s2, int precision, int scale) { - // Expected types follow the extension's spec v0.102.0 formulas. In particular, divide's - // precision uses P2, where the separate prose example uses S2. assertEquals( R.decimal(precision, scale), resolve(name + ":dec_dec", R.decimal(p1, s1), R.decimal(p2, s2))); @@ -51,6 +52,9 @@ void decimalProgramsDeriveTheCatalogFormula( @Test void decimalProgramsPreserveMirrorNullabilityAndLiteralConstraints() { assertEquals(N.decimal(11, 2), resolve("add:dec_dec", N.decimal(10, 2), R.decimal(5, 1))); + assertEquals(N.decimal(17, 8), resolve("divide:dec_dec", N.decimal(10, 2), R.decimal(5, 1))); + assertEquals(N.decimal(17, 8), resolve("divide:dec_dec", R.decimal(10, 2), N.decimal(5, 1))); + assertEquals(N.decimal(17, 8), resolve("divide:dec_dec", N.decimal(10, 2), N.decimal(5, 1))); for (String name : List.of("bitwise_and", "bitwise_or", "bitwise_xor")) { assertEquals( R.decimal(20, 0), resolve(name + ":dec_dec", R.decimal(10, 0), R.decimal(20, 0))); diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java index 0e6dcf7f4..9c7018272 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitTypeSystem.java @@ -266,7 +266,7 @@ public RelDataType deriveDecimalDivideType( type2, (p1, s1, p2, s2) -> { int scale = Math.max(6, s1 + p2 + 1); - return new int[] {p1 - s1 + p2 + scale, scale}; + return new int[] {p1 - s1 + s2 + scale, scale}; }); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/DecimalDivideTest.java b/isthmus/src/test/java/io/substrait/isthmus/DecimalDivideTest.java new file mode 100644 index 000000000..5b00e36ca --- /dev/null +++ b/isthmus/src/test/java/io/substrait/isthmus/DecimalDivideTest.java @@ -0,0 +1,90 @@ +package io.substrait.isthmus; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import io.substrait.expression.Expression; +import io.substrait.isthmus.sql.SubstraitCreateStatementParser; +import io.substrait.plan.Plan; +import io.substrait.plan.ProtoPlanConverter; +import io.substrait.relation.Project; +import io.substrait.type.Type; +import java.util.List; +import org.apache.calcite.prepare.CalciteCatalogReader; +import org.apache.calcite.rel.RelRoot; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +class DecimalDivideTest extends PlanTestBase { + @ParameterizedTest + @CsvSource({ + "10, 2, 5, 1, 17, 8", + "10, 2, 9, 0, 20, 12", + "28, 10, 10, 2, 38, 18", + "38, 10, 38, 10, 38, 6", + "32, 0, 1, 0, 38, 6" + }) + void exportUsesDivisorScale(int p1, int s1, int p2, int s2, int precision, int scale) + throws Exception { + CalciteCatalogReader catalog = catalog(p1, s1, p2, s2); + Plan exported = new SqlToSubstrait().convert("SELECT a / b FROM t", catalog); + Project project = (Project) exported.getRoots().get(0).getInput(); + Expression.ScalarFunctionInvocation divide = + (Expression.ScalarFunctionInvocation) project.getExpressions().get(0); + Type expected = N.decimal(precision, scale); + + assertEquals("divide:dec_dec", divide.declaration().key()); + assertEquals(expected, divide.getType()); + assertEquals(List.of(expected), project.getRecordType().fields()); + assertProtoPlanRoundrip("SELECT a / b FROM t", new SqlToSubstrait(), catalog); + } + + @ParameterizedTest + @CsvSource({ + "10, 2, 5, 1, 21, 8", + "10, 2, 9, 0, 29, 12", + "28, 10, 10, 2, 38, 10", + "38, 10, 38, 10, 38, 6" + }) + void importPreservesPreviouslyDeclaredResultType( + int p1, int s1, int p2, int s2, int oldPrecision, int oldScale) throws Exception { + CalciteCatalogReader catalog = catalog(p1, s1, p2, s2); + Plan plan = new SqlToSubstrait().convert("SELECT a / b FROM t", catalog); + Plan.Root root = plan.getRoots().get(0); + Project project = (Project) root.getInput(); + Expression.ScalarFunctionInvocation divide = + (Expression.ScalarFunctionInvocation) project.getExpressions().get(0); + Type oldType = N.decimal(oldPrecision, oldScale); + // Existing plans carry the result type derived before the formula was corrected. + Expression oldDivide = + Expression.ScalarFunctionInvocation.builder().from(divide).outputType(oldType).build(); + Project oldProject = Project.builder().from(project).expressions(List.of(oldDivide)).build(); + Plan.Root oldRoot = Plan.Root.builder().from(root).input(oldProject).build(); + Plan oldPlan = Plan.builder().from(plan).roots(List.of(oldRoot)).build(); + + io.substrait.proto.Plan wire = toProto(oldPlan); + Plan imported = + new ProtoPlanConverter(extensions) + .from(io.substrait.proto.Plan.parseFrom(wire.toByteArray())); + assertEquals(wire, toProto(imported)); + assertEquals(List.of(oldType), imported.getRoots().get(0).getInput().getRecordType().fields()); + + RelRoot calcite = + new SubstraitToCalcite(converterProvider, catalog).convert(imported.getRoots().get(0)); + assertEquals( + oldType, + TypeConverter.DEFAULT.toSubstrait( + calcite.validatedRowType.getFieldList().get(0).getType())); + Plan.Root reexported = SubstraitRelVisitor.convert(calcite, converterProvider); + Project reexportedProject = (Project) reexported.getInput(); + assertEquals(List.of(oldType), reexportedProject.getRecordType().fields()); + Expression.ScalarFunctionInvocation reexportedDivide = + (Expression.ScalarFunctionInvocation) reexportedProject.getExpressions().get(0); + assertEquals("divide:dec_dec", reexportedDivide.declaration().key()); + assertEquals(oldType, reexportedDivide.getType()); + } + + private CalciteCatalogReader catalog(int p1, int s1, int p2, int s2) throws Exception { + return SubstraitCreateStatementParser.processCreateStatementsToCatalog( + String.format("CREATE TABLE t (a DECIMAL(%d,%d), b DECIMAL(%d,%d))", p1, s1, p2, s2)); + } +}