asolimando commented on code in PR #5345:
URL: https://github.com/apache/calcite/pull/5345#discussion_r4353685985
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2785,6 +2770,66 @@ && 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
+ if (SqlTypeUtil.isBinary(outer)) {
+ // D.3
+ 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()
Review 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.
--
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]
