Repository navigation
[CALCITE-7870] RexSimplify removes some casts which change semantics #5345
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2692,52 +2692,37 @@ && sameTypeOrNarrowsNullability(e.getType(), operand.getType()) | |
| } | ||
| if (RexUtil.isLosslessCast(operand)) { | ||
| // x :: y below means cast(x as y) (which is PostgreSQL-specific cast by the way) | ||
| // A) Both casts can be removed: | ||
| // A.1) intExpr :: bigint :: int => intExpr | ||
| // A.2) char2Expr :: char(5) :: char(2) => char2Expr | ||
| // B) Simplification is not possible: | ||
| // B.1) char6expression :: char(3) :: char(5) => must not be changed | ||
| // the input is truncated to 3 chars, so we can't use char6expression :: char(5) | ||
| // B.2) varchar2Expr :: char(5) :: varchar(2) => must not be changed | ||
| // the input has to be padded with spaces (up to 2 chars) | ||
| // B.3) char2expression :: char(4) :: varchar(5) => must not be changed | ||
| // would not have the padding | ||
|
|
||
| RexNode intExpr = ((RexCall) operand).operands.get(0); | ||
| if ((intExpr.getType().getSqlTypeName() == SqlTypeName.CHAR | ||
| || operand.getType().getSqlTypeName() != SqlTypeName.CHAR) | ||
| && sameTypeOrNarrowsNullability(e.getType(), intExpr.getType())) { | ||
| // Remove both casts (A-like cases): the outer type is the source type, | ||
| // so the two casts are a round trip, except through a CHAR type from a | ||
| // non-CHAR source, as in int :: char(11) :: int: not every runtime | ||
| // parses the padded text back | ||
| return intExpr; | ||
| } | ||
| if (RexUtil.isLosslessCast(intExpr.getType(), operand.getType()) | ||
| && canRemoveInnerCast(intExpr.getType(), operand.getType(), e.getType())) { | ||
| return rexBuilder.makeCast(e.getParserPosition(), e.getType(), intExpr); | ||
| } | ||
| } | ||
| final boolean safe = e.getKind() == SqlKind.SAFE_CAST; | ||
| switch (operand.getKind()) { | ||
| case LITERAL: | ||
| final RexLiteral literal = (RexLiteral) operand; | ||
| final Comparable<?> value = literal.getValueAs(Comparable.class); | ||
| final SqlTypeName typeName = literal.getTypeName(); | ||
|
|
||
| // First, try to remove the cast without changing the value. | ||
|
|
@@ -2787,6 +2772,73 @@ && sameTypeOrNarrowsNullability(e.getType(), intExpr.getType())) { | |
| } | ||
| } | ||
|
|
||
| /** Returns whether {@code CAST(CAST(x AS inner) AS outer)} can be simplified to | ||
| * {@code CAST(x AS outer)}, where x has type {@code source} and the cast from | ||
| * {@code source} to {@code inner} is lossless. */ | ||
| private static boolean canRemoveInnerCast(RelDataType source, RelDataType inner, | ||
| RelDataType outer) { | ||
| // x :: y below means cast(x as y) | ||
| // Removing the inner cast changes the type of the operand of the outer cast | ||
| // from inner to source, but not its value. The result of the outer cast can | ||
| // still change, because a cast to a character or binary type formats its | ||
| // operand according to the operand's type, and a cast of a character operand | ||
| // parses its text. | ||
| // Cases A and B are in simplifyCast. | ||
| // C) The inner cast can be removed: | ||
| // C.1) char2expression :: char(4) :: char(5) => char2expression :: char(5) | ||
| // C.2) char2expression :: char(10) :: char(5) => char2expression :: char(5) | ||
| // C.3) char2expression :: varchar(10) :: char(5) => char2expression :: char(5) | ||
| // C.4) char6expression :: varchar(10) :: char(5) => char6expression :: char(5) | ||
| // C.5) smallintExpr :: int :: varchar => smallintExpr :: varchar | ||
| // 20 => '20' in both cases | ||
| // C.6) intExpr :: varchar :: int => intExpr :: int | ||
| // 20 => 20 in both cases | ||
| // C.7) intExpr :: varchar :: bigint => intExpr :: bigint | ||
| // 7369 => 7369 in both cases; the two casts to tinyint give an error in both cases | ||
| // D) The inner cast cannot be removed (see also B.2 and B.3 in simplifyCast): | ||
| // D.1) intExpr :: double :: varchar => must not be changed | ||
| // 20 => right: '20.0', wrong: '20' | ||
| // realExpr :: double :: varchar => must not be changed | ||
| // 266.66666 => right: '266.6666564941406', wrong: '266.66666' | ||
| // D.2) intExpr :: decimal(12, 2) :: varchar => must not be changed | ||
| // 20 => right: '20.00', wrong: '20' | ||
| // D.3) smallintExpr :: bigint :: varbinary => must not be changed | ||
| // 20 => right: X'0000000000000014', wrong: X'0014' | ||
| // D.4) intExpr :: varchar :: timestamp => must not be changed | ||
| // 7369 => right: error, wrong: 1970-01-01 00:00:07 | ||
| // D.5) tinyintExpr :: smallint :: variant => must not be changed | ||
| // a VARIANT keeps the type of its value; TYPEOF gives | ||
| // right: SMALLINT, wrong: TINYINT | ||
| if (SqlTypeUtil.isBinary(outer)) { | ||
| // D.3 | ||
| return false; | ||
| } | ||
| if (outer.getSqlTypeName() == SqlTypeName.VARIANT) { | ||
| // D.5 | ||
| return false; | ||
| } | ||
| if (SqlTypeUtil.inCharFamily(inner)) { | ||
| if (inner.getSqlTypeName() == SqlTypeName.CHAR | ||
| && outer.getSqlTypeName() != SqlTypeName.CHAR) { | ||
| // B.2 and B.3 from simplifyCast | ||
| return false; | ||
| } | ||
| if (!SqlTypeUtil.inCharFamily(source) | ||
| && !SqlTypeUtil.inCharFamily(outer) | ||
| && outer.getSqlTypeName() != source.getSqlTypeName() | ||
|
Member
There was a problem hiding this comment. Since branch A already handles
Contributor
Author
There was a problem hiding this comment. Are you saying we are missing a case which we should reject? This code runs in the simplifier, so it should be dialect-independent. The simplifier generally should not make assumptions about how the runtime will do something. It should remove casts only when it's guaranteed they will not change results under any reasonable runtime. If we are not sure, we should not remove the cast. The compiler does depend on the runtime in the reduce_* functions used in the rewrite passes, but the simplifier should not.
Member
There was a problem hiding this comment. That's basically my worry, yes, I wouldn't know how to effectively check if some cases like the one I mentioned wouldn't cause problem for any possible runtime system. To be clear, I think this PR is a net improvement, it's more a general "testability/validation" question, it's easier when you have a reference target system, supporting any system is trickier.
Contributor
Author
There was a problem hiding this comment. I cannot guarantee that we won't find more exceptions in the future. But I think the ones added now are justified. Calcite's flexibility makes this difficult to test.
Member
There was a problem hiding this comment. That would be equal to ask "no bugs" in code, that wasn't my point, and as I said the PR is already a solid improvement that I will take. I was interested to hear if you had opinions on how to test this more broadly, possibly with the help of agents. I know that in your stack you have DataFusion (Arrow types) and a Rust DataFlow runtime, I was wondering if there was a way to come up with a test harness for an agent to test all combinations with runtime values and spot more exceptions, like trying to find a counter example (something like https://arxiv.org/pdf/2403.03193 is doing to "validate" rewrite rules, finding a minimal counter example). If we can solve this for one given runtime (say even DF and Arrow), we can write down what was done and users with different runtimes can do the same and open tickets/PRs to refine what we have. It's a way broader discussion beyond this PR, feel free to disregard if you don't have time, I will approve the PR regardless as it's a solid improvement, just curious to hear your thoughts on this.
Contributor
Author
There was a problem hiding this comment. Despite what that paper claims, I think that formal verification is rather hopeless for real implementations of of real SQL because the SQL spec is rather loose, and each dialect makes different choices. Perhaps if you fix ONE dialect then you can do something by proving that some rewrites do not produce plans with equivalent semantics, but Calcite is supposed to be very flexible and can emulate features from many dialects based on various configuration flags. Agents do change the equation, since they never get tired writing test cases or debugging test failures; they are seemingly also good at writing proofs; tying proofs to the actual code is another matter, though. I don't think Java has good enough tooling for this; many people are using Verus these days for Rust. In my recent testing I have been asking agents to generate "all possible combinations" of tests for some reduced test matrices and to compare our results against postgres. This works very well when the Calcite semantics aligns with postgres. I have also been comparing our Rust runtime with the calcite constant folding implementation -- by evaluating the same expression on constants or on runtime data -- by turning on and off the REDUCE_* passes in our Calcite optimizer. Any discrepancy requires investigation: could be our bug or a Calcite bug. In the case of this PR the agent figured out that these rewrites happen even without the constant folding... so these are really bugs. I am not sure how useful this answer is. I am still learning how to best do things. |
||
| && !(SqlTypeUtil.isIntType(source) && SqlTypeUtil.isNumeric(outer))) { | ||
| // D.4; C.6 and C.7 are the exceptions | ||
| return false; | ||
| } | ||
| } else if (SqlTypeUtil.inCharFamily(outer) | ||
| && !(SqlTypeUtil.isIntType(source) && SqlTypeUtil.isIntType(inner))) { | ||
| // D.1 and D.2; C.5 is the exception | ||
| return false; | ||
| } | ||
| return SqlTypeCoercionRule.instance() | ||
| .canApplyFrom(outer.getSqlTypeName(), source.getSqlTypeName()); | ||
| } | ||
|
|
||
| /** Tries to simplify CEIL/FLOOR function on top of CEIL/FLOOR. | ||
| * | ||
| * <p>Examples: | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.